On Thu, Aug 14, 2025 at 3:41 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > On Wed, Aug 13, 2025 at 01:09:27PM -0600, Tycho Andersen wrote: > > On Wed, Aug 13, 2025 at 07:56:01PM +0100, Al Viro wrote: > > > @@ -3347,18 +3360,11 @@ static int do_set_group(struct path *from_path, struct path *to_path) > > > > > > namespace_lock(); > > > > > > - err = -EINVAL; > > > - /* To and From must be mounted */ > > > - if (!is_mounted(&from->mnt)) > > > - goto out; > > > - if (!is_mounted(&to->mnt)) > > > - goto out; > > > - > > > - err = -EPERM; > > > - /* We should be allowed to modify mount namespaces of both mounts */ > > > - if (!ns_capable(from->mnt_ns->user_ns, CAP_SYS_ADMIN)) > > > + err = may_change_propagation(from); > > > + if (err) > > > goto out; > > > - if (!ns_capable(to->mnt_ns->user_ns, CAP_SYS_ADMIN)) > > > + err = may_change_propagation(from); > > > > Just driving by, but I guess you mean "to" here. > > D'oh... Yes, of course. Fun question: would our selftests have caught > that? > [checks] > move_mount_set_group_test.c doesn't have anything in that area, nothing in > LTP or xfstests either, AFAICS... Yes, selftest is very simple and is not covering userns checks. > And I don't see anything in > https://github.com/checkpoint-restore/criu > either - there are uses of MOVE_MOUNT_SET_GROUP, but they are well-buried > and I don't see anything in their tests that would even try to poke into > that thing... > > Before we go and try to cobble something up, does anybody know of a place > where regression tests for MOVE_MOUNT_SET_GROUP could be picked from? > Basically each CRIU test that is run by zdtm (if it is in ns/uns flavor (which are most of them)), tests mounts checkpoint/restore. And each test which has shared/slave moutns leads to MOVE_MOUNT_SET_GROUP being used and thus tested. We have a mountinfo comparison in zdtm which checks that propagation is topologically the same after c/r. But, yes, we do not cover userns checks, as in CRIU case, CRIU is expected to run in userns which has all capabilities over restored container, and should always pass those checks. JFYI: The use of MOVE_MOUNT_SET_GROUP in CRIU is well-buried in: https://github.com/checkpoint-restore/criu/blob/116e56ba46382c05066d33a8bbadcc495dbdb644/criu/mount-v2.c#L896 +-< move_mount_set_group +-< restore_one_sharing +-< restore_one_sharing_group +-< restore_mount_sharing_options +-< prepare_mnt_ns_v2 This stack already has a set of precreated mounts and walks over their sharing groups saved in CRIU image files and assigns them accordingly. And we have a bunch of tests with different sharing configurations to test propagation c/r specifically: git grep -l "SHARING\|SLAVE" test/zdtm/static test/zdtm/static/mnt_ext_auto.c test/zdtm/static/mnt_ext_master.c test/zdtm/static/mnt_ext_multiple.c test/zdtm/static/mnt_root_ext.c test/zdtm/static/mntns_overmount.c test/zdtm/static/mntns_shared_bind03.c test/zdtm/static/mount_complex_sharing.c test/zdtm/static/mountpoints.c test/zdtm/static/shared_slave_mount_children.c It should be enough to run a zdtm test-suit to check that change does not break something for CRIU (will do).
