On Thu, Aug 14, 2025 at 3:41 AM Al Viro viro@zeniv.linux.org.uk 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/116e56ba46382c05066d33a8bbad...
+-< 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).