cmd/snap-confine: unshare per-user mount ns once #6846
Conversation
The function mount-support.c, sc_make_slave_mount_ns unshared the per-user mount namespace again, even though it is explicitly done in snap-confine.c, inside enter_non_classic_execution_environment. Both unshare calls are close to each other for clarity but a third one stay unnoticed in mount-support.c The second mount namespace was unshared before any modifications took place so this bug was unobservable apart from the increments in mount namespace allocation numbers. Fixes: https://bugs.launchpad.net/snapd/+bug/1828352 Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
|
Thanks for tracking this down! You say its unobservable - does that mean we cannot write any sort of this for this? |
|
@mvo5 I can write a test but it would be unreliable. You can look at the number that the kernel assigns to the mount namespace (it's a number like 402653184), assuming that the numbers are allocated sequentially you can unshare and see 402653185, unshare again and get 402653186. If kernel changes the allocator (e.g. starts to randomise the numbers more) the test would break. |
| @@ -647,9 +647,6 @@ void sc_ensure_shared_snap_mount(void) | |||
|
|
|||
| static void sc_make_slave_mount_ns(void) | |||
| { | |||
| if (unshare(CLONE_NEWNS) < 0) { | |||
| die("can not unshare mount namespace"); | |||
| } | |||
There was a problem hiding this comment.
Removing this from here makes this function a one line function, called from only one place (sc_setup_user_mounts). As such, I think I'd prefer this patch to move this:
// In our new mount namespace, recursively change all mounts
// to slave mode, so we see changes from the parent namespace
// but don't propagate our own changes.
sc_do_mount("none", "/", NULL, MS_REC | MS_SLAVE, NULL);
down into sc_setup_user_mounts() and then removing sc_make_slave_mount_ns() entirely.
There was a problem hiding this comment.
I agree but I wanted to keep it separate. In another branch I simplified this whole chunk. I was after a psychological effect where a patch that removes code without adding any code is easier to review.
I will open a follow up.
|
Maybe we could try |
The function mount-support.c, sc_make_slave_mount_ns unshared the
per-user mount namespace again, even though it is explicitly done in
snap-confine.c, inside enter_non_classic_execution_environment. Both
unshare calls are close to each other for clarity but a third one stay
unnoticed in mount-support.c
The second mount namespace was unshared before any modifications took
place so this bug was unobservable apart from the increments in mount
namespace allocation numbers.
Fixes: https://bugs.launchpad.net/snapd/+bug/1828352
Signed-off-by: Zygmunt Krynicki me@zygoon.pl