many: make per-snap mount namespace MS_SHARED - #6891
Conversation
846ad75 to
b47246d
Compare
70a9487 to
e81e2cd
Compare
When snap-confine creates the initial layout of the per-snap mount namespace it was using MS_SLAVE in order not to propagate mount events from that mount namespace to the initial mount namespace of the system. This was working fine, including for updates via snap-update-ns, which was always changing the per-snap mount namespace. It didn't fully work for per-snap, per-user mount namespaces. This was mostly affecting desktop applications that inhabit said namespace, but to a lesser degree some of the other mount constructs established by snap-update-ns. This patch changes snap-confine, so that each MS_SLAVE, is followed up by MS_SHARED. This seems counter-intuitive, is it shared or not? The behavior of the kernel is well documented but a little bit misleading at first. When we first change to MS_SLAVE we are closing the propagation channel between a mount point in the per-snap mount namespace and the same location in the initial mount namespace. When we are subsequently using MS_SHARED we are opening the channel to mount namespaces unshared from our own. Once sealed, propagation between mount namespaces cannot be re-established. This is also visible when we carefully set up /media and /mnt so that bi-directional sharing between those places is preserved. We do that by never using MS_SLAVE there. Because propagation settings are inherited by descendant mount points, this change in snap-confine propagates to various other constructs, specifically to the changes done by snap-update-ns, for both initial construction as well as on-the-fly updates. All the mount points established under shared locations remain shared. There's one downside: this actively harms us in the code path that constructs the writable mimic. That code recursively bind-mounts a directory to a stub directory in /tmp/.snap/, so that we can still see the original content, and then proceeds to re-create the original content on top of a freshly mounted tmpfs. With the propagation settings changed that tmpfs would also show up in the stash directory in /tmp, therefore breaking the whole setup. To counter that, the writable mimic construction code in snap-update-ns switched the stash mount point to recursively private, so that prior semantics is retained. With this change we can now observe mount changes performed in the per-snap mount namespace propagate to the derived per-snap, per-user mount namespace. A new spread regression test measures that for the use case of layouts and writable mimics. Somewhat surprisingly, an existing spread test that used to show how layout changes from revision to revision can fail no longer fails! It was, after all, related to propagation changes between layout elements and content interface connections. That test is adjusted to show how the application works in all the revisions used in the test. Another test that was affected was performing cleanup, in the restore section, to unmount a FUSE filesystem mounted in the per-instance view of $SNAP_DATA, followed by the non-instance mount in the $SNAP_DATA. With the propagation changes fixed, the unmount in either of the two locations propagates to the other and a single unmount is sufficient. Apart from the aforementioned changes there are some adjustments to the apparmor policy, to allow the new uses of mount with MS_SHARED and MS_PRIVATE. The associated unit tests are adjusted to reflect that. All of this is coupled with a new regression test, that is reproducing the case of a application running in the per-snap, per-user mount namespace correctly observing all changes performed to the per-snap mount namespace. The test uses persistent per-snap, per-user mount namespaces to observe that as otherwise, since per-user mount namespace persistence is not enabled by default yet, each invocation of the test application would see a new mount namespace, constructed out of the per-snap mount namespace, which never was affected by this issue to begin with. https://bugs.launchpad.net/snapd/+bug/1828354 Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
jdstrand
left a comment
jdstrand
left a comment
There was a problem hiding this comment.
Couple of minor things inline.
At a high level, the tests and apparmor changes look good as does the reasoning in the commit message. The subtlety and complexity of the myriad of mount propagation adjustments throughout the codebase make this difficult to confidently and empirically verify the changes for correctness, which makes me uncomfortable (at one point snap-confine/README.mount_namespace described the setup but it is out of date and not updated by this PR). Granted, snapd is doing some rather novel things with mounts, so the complexity is understandable....
I can say that using the gnome-calculator snap, I compared the strace mount calls after a discard of master with the strace mount calls after a discard with this PR applied and it looks like MS_SHARED is placed after MS_SLAVE except with:
- mount("none", "/tmp/snap.rootfs_5fTIlC/etc/ssl", NULL, MS_SLAVE, NULL) = 0
- mount("none", "/tmp/snap.rootfs_5fTIlC/etc/nsswitch.conf", NULL, MS_SLAVE, NULL) = 0
These come from mount-support.c, 'dirs_from_core'. Since these are not in the 'per-user' area, it is unclear (to me) if these should also be MS_SHARED. The commit message doesn't mention these either AFAICS, so if this is intended behavior, there probably needs to be a comment somewhere.
One thing that was missing from this PR that I was hoping to see were tests that very clearly show that the addition of MS_SHARED did not change the behavior of the host system or other snaps (eg, the content provider) and proving that it coming after MS_SLAVE works as documented against the myriad of kernels out there. This might be further reason to not rely on python 3.7 for mountinfo-tool (see inline comment).
| # XXX: this seems unused | ||
| mount options=(rw bind) /usr/bin/snapctl -> /tmp/snap.rootfs_*/usr/bin/snapctl, | ||
| mount options=(rw slave) -> /tmp/snap.rootfs_*/usr/bin/snapctl, | ||
| mount options=(rw slave) -> /tmp/snap.rootfs_*/usr/bin/snapctl, |
There was a problem hiding this comment.
If it is unused, we should remove it, no?
There was a problem hiding this comment.
Yeah, I just didn't want to mix up other changes in this patch. I have a few cleanups for the profile in a separate patch.
| mount options=(rw rshared) -> /var/lib/snapd/hostfs/, | ||
| # cleanup | ||
| umount /var/lib/snapd/hostfs/tmp/snap.rootfs_*/, | ||
| umount /var/lib/snapd/hostfs/sys/, |
There was a problem hiding this comment.
The changes to the apparmor policy look fine.
| // umount2(2) system call. | ||
| Name: dir, Dir: safeKeepingDir, Options: []string{"rbind"}}, | ||
| // | ||
| // The rprivate is there to ensure that changes made to the |
| // | ||
| // The rprivate is there to ensure that changes made to the | ||
| // original directory do not propagate into this view. | ||
| Name: dir, Dir: safeKeepingDir, Options: []string{"rprivate", "rbind"}}, |
There was a problem hiding this comment.
Based on man 2 mount, this comment is incomplete. It says: "Make this mount point private. Mount and unmount events do not propagate into or out of this mount point." so the comment here should s/into/into or out of/, no?
There was a problem hiding this comment.
Yes, you are correct. I simply expressed that the part we care about is inward propagation (because we don't mount anything in this view the outward propagation does not matter). I will adjust the comment.
| # Unmount the filesystem mounted inside the mount namespace of the | ||
| # application. Due to propagation settings this is sufficient even if | ||
| # instances are used, where the mount point was visible under two | ||
| # locations, because the unmount event propagates across. |
There was a problem hiding this comment.
Based on the umount man page, the propagation doesn't happen when 'MS_REC|MS_PRIVATE' is used, which was added in change.go. You are probably fine for this since the test doesn't have snap-update-ns change events.
There was a problem hiding this comment.
This requires some more explaining. The mount propagation is actually shared here. I will iterate and explain tomorrow.
There was a problem hiding this comment.
Oh right, because it was done earlier and not undone in between the calls:
mount("/etc", "/tmp/snap.rootfs_5fTIlC//etc", NULL, MS_BIND|MS_REC, NULL) = 0
mount("none", "/tmp/snap.rootfs_5fTIlC//etc", NULL, MS_REC|MS_SLAVE, NULL) = 0
mount("none", "/tmp/snap.rootfs_5fTIlC//etc", NULL, MS_REC|MS_SHARED, NULL) = 0
...
mount("/snap/core18/current/etc/ssl", "/tmp/snap.rootfs_5fTIlC/etc/ssl", NULL, MS_BIND, NULL) = 0
mount("none", "/tmp/snap.rootfs_5fTIlC/etc/ssl", NULL, MS_SLAVE, NULL) = 0
mount("/snap/core18/current/etc/nsswitch.conf", "/tmp/snap.rootfs_5fTIlC/etc/nsswitch.conf", NULL, MS_BIND, NULL) = 0
mount("none", "/tmp/snap.rootfs_5fTIlC/etc/nsswitch.conf", NULL, MS_SLAVE, NULL) = 0
There was a problem hiding this comment.
Oh right, because it was done earlier and not undone in between the calls:
mount("/etc", "/tmp/snap.rootfs_5fTIlC//etc", NULL, MS_BIND|MS_REC, NULL) = 0 mount("none", "/tmp/snap.rootfs_5fTIlC//etc", NULL, MS_REC|MS_SLAVE, NULL) = 0 mount("none", "/tmp/snap.rootfs_5fTIlC//etc", NULL, MS_REC|MS_SHARED, NULL) = 0 ... mount("/snap/core18/current/etc/ssl", "/tmp/snap.rootfs_5fTIlC/etc/ssl", NULL, MS_BIND, NULL) = 0 mount("none", "/tmp/snap.rootfs_5fTIlC/etc/ssl", NULL, MS_SLAVE, NULL) = 0 mount("/snap/core18/current/etc/nsswitch.conf", "/tmp/snap.rootfs_5fTIlC/etc/nsswitch.conf", NULL, MS_BIND, NULL) = 0 mount("none", "/tmp/snap.rootfs_5fTIlC/etc/nsswitch.conf", NULL, MS_SLAVE, NULL) = 0
We probably still need a comment that while we made this MS_SLAVE here, that's ok cause we aren't doing anything else below.
| test "$(as_snap_root /bin/cat "/snap/$snap/x1/dir/canary")" = "app:dir/canary" | ||
| test "$(as_snap_root /bin/cat "/snap/$snap/x1/meta/canary")" = "app:meta/canary" | ||
|
|
||
| # Same as above but for snap user in per-user mount namespace. |
There was a problem hiding this comment.
Since per-user mount namespaces are meant to deal with things not in $SNAP, I expected some additional asserts here or possibly below...
There was a problem hiding this comment.
Since all read only areas those relate to the writable mimic where sharing was broken. I agree it is worth adding tests that show how /run or /usr behaves.
| @@ -0,0 +1,206 @@ | |||
| summary: mount event propagation works inside snapd-created tmpfs | |||
| # NOTE: The limitation comes from availability of Python 3.7 for mountinfo-tool | |||
There was a problem hiding this comment.
Considering the complexity of the mount namespace setup and our reliance on mount propagation rules, I feel like limiting the regression tests to just these systems is risky. Briefly looking at mountinfo-tool, it isn't clear why it relies on 3.7. Why does it?
There was a problem hiding this comment.
It is temporary. I will make it compatible with more systems tomorrow. It simply started with 3.7 features because I had not realized those are 3.7 features. It should work everywhere and will do so.
There was a problem hiding this comment.
This is now done in #6940 -- curiously it did find an issue in 16.04 core systems. I will investigate why next week.
Note, I'm not talking about things introduced by this PR, which are pretty contained, I'm talking about how our mount namespace setup is (understandably) complex to start with which makes things difficult to verify. My larger point is essentially more tests and (eventually?) more docs. |
|
The apparmor denial I see on 14.04 makes no sense to me yet. It smells like a kernel bug. I'm investigating what is going on. I don't know what's causing this. You can try to debug this: Just remember to set SNAP_REEXEC=0 to run snap-confine with a locally altered profile. |
When repacking the snapd snap ensure that any "snap-confine.real" apparmor profiles are removed before applying the update.
Codecov Report
@@ Coverage Diff @@
## master #6891 +/- ##
=========================================
Coverage ? 79.89%
=========================================
Files ? 614
Lines ? 47485
Branches ? 0
=========================================
Hits ? 37939
Misses ? 6544
Partials ? 3002
Continue to review full report at Codecov.
|
chipaca
left a comment
chipaca
left a comment
There was a problem hiding this comment.
+1 assuming you address the issues jamie has pointed out :-)
|
I've expanded tests and noticed that on core16 there's something unexpected about mount setup in the mimic test. It seems we have one-too-many mounts there. I will investigate this today. |
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
|
This is now failing, as I mentioned, on core16, it seems to be related to legacy mount layout. I'm debugging this now. EDIT: Having merged with master we are now using |
|
I understand why the change to detach is causing the regression in the change to shared mount points. I need to think about this some more. With the propagation changes one unmount may invalidate another. I'm worried that it will cause consistency issues in what the mount namespace actually is like. |
When snap-update-ns is unmounting something that can host additional
mount points inside, it is now using MNT_DETACH to detach the mount
point (recursively) and lazily unmount anything that is no longer used.
It can be though about like removing a reference in a garbage collected
memory management system. Unlike such systems, however, the kernel also
implements mount and unmount event *propagation*. This can replicate the
change in peer groups that receive propagation events. This can, oddly,
cause unmount loops where the kernel returns EBUSY, because the unmount
operation (even with MNT_DETACH) affects itself, in a way.
One such example is added as a test case in this patch, a fragment of which is
presented here:
// The mount profile contains but two entries:
//
// 1) recursive-bind $SNAP/b/c -> $SNAP/a
// 2) recursive-bind $SNAP/d -> $SNAP/b
//
// NB: The notation "foo -> bar" can be read as "foo is a view of bar",
// and can be implemented by mount "--rbind bar foo".
//
// Both mount operations are performed under a substrate that is MS_SHARED.
// Therefore, due to the rules that decide upon propagation of bind mounts
// the propagation of the new mount entries is also shared. This is
// documented in https://lwn.net/Articles/159092/ (section 5b).
//
// Interactive experimentation shows that the following three mount points exist
// after this operation, as illustrated by findmnt:
//
// └─/snap/test-snapd-layout/x1 /dev/loop1 squashfs ro,nodev,relatime
// ├─/snap/test-snapd-layout/x1/b/c /dev/loop1[/a] squashfs ro,nodev,relatime
// └─/snap/test-snapd-layout/x1/d /dev/loop1[/b] squashfs ro,nodev,relatime
// └─/snap/test-snapd-layout/x1/d/c /dev/loop1[/a] squashfs ro,nodev,relatime
//
// Note that after the first mount operation only one mount point is created, namely
// $SNAP/b/c -> $SNAP/a. The second recursive bind mount not only creates
// $SNAP/d -> $SNAP/b, but also replicates $SNAP/b/c -> $SNAP/a as
// $SNAP/d/c -> $SNAP/a.
When this mount profile is undone, the unmount --lazy (aka MNT_DETACH)
operation on x1/d fails with EBUSY, as explained above. The patch solves that
by changing event propagation, in the unmounted tree, to private. This idea is
hinted at in the umount(2) manual page.
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
|
I've fixed the regression and returned this patch-set to one failing test: mimic on core16. I will resume investigation next week. |
|
I'm back looking at this. EDIT, enter-happy comment I think I understand what the last problem is now. I will propose a separate test that measures the configuration of the mount namespace, similar to what we had before in ancient snap-confine spread tests. With that test in place it will be 1) easier to understand how things behave today 2) measurably different each time we make changes 3) easier to reason about the changes made here. Therefore I'll fork off a small branch that illustrates this and only then merge master into this branch to address the final issue. EDIT2: The prerequisites for the new test have mostly landed. I still have #7071 open and about two more changes to |
|
I'm working on merging #7091 here locally to reflect the changes to mount namespace measurements. |
|
Now that #7091 is merged this can be adjusted and changed to green :) |
Adjust /lib/firmware apparmor permissions to match other changes. Adjust expected mount tables that shows what the mount namespaces are like.
|
I need to close this for now as it is not in a laudable state. I will reopen the branch or try to reopen a smaller part of this branch that can effectively land. |
|
I will close this branch for now and look at re-opening smaller part of it that can land effectively. Thank you for all the review time on it, I will try to not make that go to waste. |
6 participants
When snap-confine creates the initial layout of the per-snap mount
namespace it was using MS_SLAVE in order not to propagate mount events
from that mount namespace to the initial mount namespace of the system.
This was working fine, including for updates via snap-update-ns, which
was always changing the per-snap mount namespace. It didn't fully work
for per-snap, per-user mount namespaces. This was mostly affecting
desktop applications that inhabit said namespace, but to a lesser degree
some of the other mount constructs established by snap-update-ns.
This patch changes snap-confine, so that each MS_SLAVE, is followed up
by MS_SHARED. This seems counter-intuitive, is it shared or not? The
behavior of the kernel is well documented but a little bit misleading at
first. When we first change to MS_SLAVE we are closing the propagation
channel between a mount point in the per-snap mount namespace and the
same location in the initial mount namespace. When we are subsequently
using MS_SHARED we are opening the channel to mount namespaces unshared
from our own.
Once sealed, propagation between mount namespaces cannot be
re-established. This is also visible when we carefully set up /media and
/mnt so that bi-directional sharing between those places is preserved.
We do that by never using MS_SLAVE there.
Because propagation settings are inherited by descendant mount points,
this change in snap-confine propagates to various other constructs,
specifically to the changes done by snap-update-ns, for both initial
construction as well as on-the-fly updates. All the mount points
established under shared locations remain shared.
There's one downside: this actively harms us in the code path that
constructs the writable mimic. That code recursively bind-mounts a
directory to a stub directory in /tmp/.snap/, so that we can still see
the original content, and then proceeds to re-create the original
content on top of a freshly mounted tmpfs. With the propagation settings
changed that tmpfs would also show up in the stash directory in /tmp,
therefore breaking the whole setup. To counter that, this change switches
the writable mimic construction code in snap-update-ns the make the
stash mount point to recursively private, so that prior semantics is retained.
With this change we can now observe mount changes performed in the
per-snap mount namespace propagate to the derived per-snap, per-user
mount namespace. A new spread regression test measures that for the use
case of layouts and writable mimics.
Somewhat surprisingly, an existing spread test that used to show how
layout changes from revision to revision can fail no longer fails! It
was, after all, related to propagation changes between layout elements
and content interface connections. That test is adjusted to show how the
application works in all the revisions used in the test.
Another test that was affected was performing cleanup, in the restore
section, to unmount a FUSE filesystem mounted in the per-instance view
of $SNAP_DATA, followed by the non-instance mount in the $SNAP_DATA.
With the propagation changes fixed, the unmount in either of the two
locations propagates to the other and a single unmount is sufficient.
Apart from the aforementioned changes there are some adjustments to the
apparmor policy, to allow the new uses of mount with MS_SHARED and
MS_PRIVATE. The associated unit tests are adjusted to reflect that.
All of this is coupled with a new regression test, that is reproducing
the case of a application running in the per-snap, per-user mount
namespace correctly observing all changes performed to the per-snap
mount namespace. The test uses persistent per-snap, per-user mount
namespaces to observe that as otherwise, since per-user mount namespace
persistence is not enabled by default yet, each invocation of the test
application would see a new mount namespace, constructed out of the
per-snap mount namespace, which never was affected by this issue to
begin with.
Fixes: https://bugs.launchpad.net/snapd/+bug/1828354
Signed-off-by: Zygmunt Krynicki me@zygoon.pl