Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap-update-ns: add planWritableMimic #4170
Conversation
Some checks were not successful
zyga
requested a review
from
jdstrand
Nov 7, 2017
codecov-io
commented
Nov 7, 2017
•
Edited 9 times
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 20, 2017
-
codecov-io
Nov 20, 2017
-
codecov-io
Nov 17, 2017
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 10, 2017
-
codecov-io
Nov 10, 2017
-
Nov 23, 2017 -
Nov 20, 2017 -
Nov 20, 2017 -
Nov 17, 2017 -
Nov 16, 2017 -
Nov 16, 2017 -
Nov 16, 2017 -
Nov 10, 2017 -
Nov 10, 2017
Codecov Report
@@ Coverage Diff @@
## master #4170 +/- ##
=========================================
+ Coverage 75.96% 76% +0.03%
=========================================
Files 442 442
Lines 38490 38545 +55
=========================================
+ Hits 29240 29295 +55
Misses 7234 7234
Partials 2016 2016
Continue to review full report at Codecov.
|
| + Action: Mount, Entry: mount.Entry{Name: "none", Dir: dir, Type: "tmpfs"}, | ||
| + }) | ||
| + // Iterate over the items in the original directory (nothing is mounted _yet_). | ||
| + entries, err := ioutilReadDir(dir) |
bboozzoo
Nov 8, 2017
Contributor
I'm not sure about using ioutil.ReadDir here. I think a better approach would be to f, err := os.Open(dir) and then range over f.Readdir(<some-sane-limit)). IIRC iotuil.ReadDir() does the same, but it will go and load FileInfo for all entries in directory. This may (but does not need to) be a problem if there are lots of files or dirs inside.
zyga
Nov 8, 2017
Contributor
I'm happy to tweak this though I don't think it will be a real problem. This is also nicer for mocking (one vs two + stateful helper).
| @@ -108,6 +115,64 @@ func secureMkdirAll(name string, perm os.FileMode, uid, gid int) error { | ||
| return nil | ||
| } | ||
| +func designWritableMimic(dir string) ([]*Change, error) { |
pedronis
Nov 10, 2017
Contributor
this function name sounds a bit strange to me... alternatives could be: planWritableMimic, writableMimicPlan, writableMimicChanges
zyga
added some commits
Nov 10, 2017
zyga
changed the title from
cmd/snap-update-ns: add designWritableMimic
to
cmd/snap-update-ns: add planWritableMimic
Nov 10, 2017
| + return nil, err | ||
| + } | ||
| + for _, fi := range entries { | ||
| + // Skip non-directory elements as we cannot handle those yet. |
mvo5
Nov 13, 2017
Collaborator
This comment says non-dir elements are skipped but some lines below the code handles m.IsRegular. What does that mean?
zyga
Nov 13, 2017
•
Edited 1 time
-
zyga
Nov 13, 2017
Contributor
-
Nov 13, 2017
That I didn't remove the comment :-) I'll fix this shortly.
EDIT: Done now
zyga
added some commits
Nov 13, 2017
| + // Mount tmpfs over the original directory, hiding its contents. | ||
| + changes = append(changes, &Change{ | ||
| + Action: Mount, Entry: mount.Entry{Name: "none", Dir: dir, Type: "tmpfs"}, | ||
| + }) |
jdstrand
Nov 20, 2017
Contributor
This isn't the extent of my review, but thought I'd mention that it doesn't seem like we'll ever need the contents of the mounted over directory ever again, so I think it would be good to try to not be wasteful with these mounts and cleanup. Right now you are doing:
- bind current on safe
- mount tmpfs on current
- for each in safe, mount in tmpfs
However, it seems we could do this instead:
- create a safe directory
- for each in current, mount in safe
- for each in current, unmount in current
- mount tmpfs on current
- for each in safe, mount in tmpfs
- for each in safe, unmount in safe
While this a few more steps, it means that when down we don't have any hidden mount under the tmpfs, taking up kernel resources, space in the mount table, etc
zyga
Nov 20, 2017
Contributor
22:55 < zyga-ubuntu> jdstrand: so I maybe agree if your proposal is undo-safe
22:56 < zyga-ubuntu> jdstrand: note that we may be asked to completely undo any of this
22:56 < zyga-ubuntu> jdstrand: and that the typical situation, nothing is hidden under the tmpfs mount
22:56 < zyga-ubuntu> jdstrand: (apart from a fragment of a squashfs mount)
22:56 < zyga-ubuntu> jdstrand: the simple approach I took guarantees (I hope, prove me wrong please) that I can always undo
22:56 < zyga-ubuntu> jdstrand: and create another layout
22:58 < jdstrand> zyga-ubuntu: that might be a case for needing the contents of the mounted over directory
23:00 < zyga-ubuntu> jdstrand: I also toyed with one more idea
23:00 < zyga-ubuntu> jdstrand: iff it's a good assumption that we can look at hostfs, we can fish the snap mounts there
23:00 < zyga-ubuntu> jdstrand: but it's not generic as the same code should be useful for poking holes in various places that are not just /snap
23:01 < zyga-ubuntu> jdstrand: and while it's possible to find the same place in hostfs it may have been changed by the hard-coded mount commands in snap-confine
jdstrand
approved these changes
Nov 20, 2017
I'm going to approve as is, but please consider the open questions inline and add the requested comments.
| @@ -286,6 +291,63 @@ func secureMkfileAll(name string, perm os.FileMode, uid, gid int) error { | ||
| return err | ||
| } | ||
| +func planWritableMimic(dir string) ([]*Change, error) { |
jdstrand
Nov 20, 2017
Contributor
I think a comment above here describing what the function does and why would be handy (eg, describing that the algorithm is intentionally simple so that the changes can be undone be simple going backwards, justifying mounting over 'dir' if you decide to keep the current implementation (eg, so the undo works as expected, there isn't much in there, etc), ....
| + // Bind mount the original directory elsewhere for safe-keeping. | ||
| + changes = append(changes, &Change{ | ||
| + Action: Mount, Entry: mount.Entry{ | ||
| + // XXX: should we rbind here? |
| + Action: Mount, Entry: mount.Entry{Name: "none", Dir: dir, Type: "tmpfs"}, | ||
| + }) | ||
| + // Iterate over the items in the original directory (nothing is mounted _yet_). | ||
| + entries, err := ioutilReadDir(dir) |
jdstrand
Nov 20, 2017
Contributor
Not a big deal (or a blocker), cause, like you said, nothing is mounted yet and this is just recording changes, but I think it might be easier to read if you did:
entries, err := ioutilReadDir(safeKeepingDir)
zyga
Nov 23, 2017
Contributor
Well, we cannot because that's just us planning and not really doing stuff. The safe keeping directory doesn't even exist yet, let alone is bound mounted.
| + ch := &Change{Action: Mount, Entry: mount.Entry{ | ||
| + Name: filepath.Join(safeKeepingDir, fi.Name()), | ||
| + Dir: filepath.Join(dir, fi.Name()), | ||
| + // XXX: should we rbind here? |
| + Name: filepath.Join(safeKeepingDir, fi.Name()), | ||
| + Dir: filepath.Join(dir, fi.Name()), | ||
| + // XXX: should we rbind here? | ||
| + Options: []string{"bind", "ro"}, |
jdstrand
Nov 20, 2017
Contributor
Soo... the function is called planWritableMimic, but we are only mounting read-only here. AIUI, the read-only is still hugely useful for making files accessible to where the snap expects them. That said, maybe just call this planMimic()?
zyga
Nov 23, 2017
Contributor
I was thinking about it and I think the extra "ro" is not needed. Changed.
| + case m.IsRegular(): | ||
| + ch.Entry.Options = append(ch.Entry.Options, "x-snapd.kind=file") | ||
| + changes = append(changes, ch) | ||
| + case m&os.ModeSymlink != 0: |
| + changes = append(changes, ch) | ||
| + } | ||
| + default: | ||
| + logger.Noticef("skipping unsupported thing %s", fi) |
jdstrand
Nov 20, 2017
Contributor
Maybe "skipping unsupported file %s"? Bonus points for showing the mode would be good (pipe, etc).
| + changes, err := update.PlanWritableMimic("/foo") | ||
| + c.Assert(err, IsNil) | ||
| + c.Assert(changes, DeepEquals, []*update.Change{ | ||
| + // Store /foo in /tmp.snap/foo while we set things up |
| + // Bind mount files and directories over. Note that files are identified by x-snapd.kind=file option. | ||
| + {Entry: mount.Entry{Name: "/tmp/.snap/foo/file", Dir: "/foo/file", Options: []string{"bind", "ro", "x-snapd.kind=file"}}, Action: update.Mount}, | ||
| + {Entry: mount.Entry{Name: "/tmp/.snap/foo/dir", Dir: "/foo/dir", Options: []string{"bind", "ro"}}, Action: update.Mount}, | ||
| + // Create symlinks, if we cannot readlink just skip that entry. |
jdstrand
Nov 20, 2017
Contributor
The comment is unclear. I suggest:
// Create symlink
{Entry: mount.Entry{Name: "/tmp/.snap/foo/symlink", Dir: "/foo/symlink", Options: []string{"bind", "ro", "x-snapd.kind=symlink", "x-snapd.symlink=target"}}, Action: update.Mount},
// Bad symlinks and all other file types are skipped and not recoarded in mount changes
// Unmount the safe-keeping directory
...
pedronis
added
the
Blocked
label
Nov 23, 2017
|
marking blocked because it seems it has +2 but a last batch of comments from jdstrand for consideration |
|
@pedronis yes, I'll update everything that Jamie requested soon. |
zyga
removed
the
Blocked
label
Nov 23, 2017
|
All the points should be addressed now, I removed the blocking label and will merge when green. |
zyga commentedNov 7, 2017
This patch adds a helper function that inspects a directory and designs
changes that will create a "writable mimic" for that directory.
The writable mimic is done largely the same way we already do this in
the snap-confine source code (quirks.c) where we use a tmpfs that is
bind mounted over the original read-only place and a series of bind
mounts (a bind-mount farm essentially).
The solution presented here is more generic. The original directory is
"stashed" (bind mounted to a temporary directory), a tmpfs is mounted
and a series of bind mounts are created. In addition to supporting just
directories the code also supports creating files and even symlinks.
Files are bind-mounted just as directories but symlinks are created
directly, since the tmpfs is writable and symlinks cannot participate in
mounting.
The code only designs the necessary changes, the rest of the features
will come once prerequisite branches land.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com