[PATCH v2] move_mount: allow to add a mount into an existing group

[PATCH v2] move_mount: allow to add a mount into an existing group

[Pavel Tikhomirov]

Previously a sharing group (shared and master ids pair) can be only
inherited when mount is created via bindmount. This patch adds an
ability to add an existing private mount into an existing sharing group.

With this functionality one can first create the desired mount tree from
only private mounts (without the need to care about undesired mount
propagation or mount creation order implied by sharing group
dependencies), and next then setup any desired mount sharing between
those mounts in tree as needed.

This allows CRIU to restore any set of mount namespaces, mount trees and
sharing group trees for a container.

We have many issues with restoring mounts in CRIU related to sharing
groups and propagation:
- reverse sharing groups vs mount tree order requires complex mounts
  reordering which mostly implies also using some temporary mounts
(please see https://lkml.org/lkml/2021/3/23/569 for more info)

- mount() syscall creates tons of mounts due to propagation
- mount re-parenting due to propagation
- "Mount Trap" due to propagation
- "Non Uniform" propagation, meaning that with different tricks with
  mount order and temporary children-"lock" mounts one can create mount
  trees which can't be restored without those tricks
(see https://www.linuxplumbersconf.org/event/7/contributions/640/)

With this new functionality we can resolve all the problems with
propagation at once.

Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Mattias Nissler <mnissler@chromium.org>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: lkml <linux-kernel@vger.kernel.org>
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

---
This is a rework of "mnt: allow to add a mount into an existing group"
patch from Andrei. https://lkml.org/lkml/2017/4/28/20

New do_set_group is similar to do_move_mount, but with many restrictions
of do_move_mount removed and that's why:

1) Allow "cross-namespace" sharing group set. If we allow operation only
with mounts from current+anon mount namespace one would still be able to
setns(from_mntns) + open_tree(from, OPEN_TREE_CLONE) + setns(to_mntns) +
move_mount(anon, to, MOVE_MOUNT_SET_GROUP) to set sharing group to mount
in different mount namespace with source mount. But with this approach
we would need to create anon mount namespace and mount copy each time,
which is just a waste of resources. So instead lets just check if we are
allowed to modify both mount namespaces (which looks equivalent to what
setns-es and open_tree check).

2) Allow operating on non-root dentry of the mount. As if we prohibit it
this would require extra care from CRIU side in places where we wan't to
copy sharing group from mount on host (for external mounts) and user
gives us path to non-root dentry. I don't see any problem with
referencing mount with any dentry for sharing group setting. Also there
is no problem with referencing one by file and one by directory.

3) Also checks wich only apply to actually moving mount which we have in
do_move_mount and open_tree are skipped. We don't need to check
MNT_LOCKED, unbindable, nsfs loops and ancestor relation as we don't
move mounts.

Also let's add some new checks (offered by Andrei):

1) Don't allow to copy sharing from mount with narrow root to a wider
root, so that user does not have power to receive more propagations when
user already has.

2) Don't allow to copy sharing from mount with locked children for the
same reason, as user shouldn't see propagations to areas overmounted by
locked mounts (if the user could not already do it before sharing
adjustment).

Security note: there would be no (new) loops in sharing groups tree,
because this new move_mount(MOVE_MOUNT_SET_GROUP) operation only adds
one _private_ mount to one group (without moving between groups), the
sharing groups tree itself stays unchanged after it.

In Virtuozzo we have "mount-v2" implementation, based with the original
kernel patch from Andrei, tested for almost a year and it actually
decreased number of bugs with mounts a lot. One can take a look on the
implementation of sharing group restore in CRIU in "mount-v2" here:

https://src.openvz.org/projects/OVZ/repos/criu/browse/criu/mount-v2.c#898

This works almost the same with current version of patch if we replace
mount(MS_SET_GROUP) to move_mount(MOVE_MOUNT_SET_GROUP), please see
super-draft port for mainstream criu, this at least passes
non-user-namespaced mount tests (zdtm.py --mounts-v2 -f ns).

https://github.com/Snorch/criu/commits/mount-v2-poc

v2: Solve the problem mentioned by Andrei:
- check mnt_root of "to" is in the sub-tree of mnt_root of "from"
- also check "from" has no locked mounts in subroot of "to"

---
 fs/namespace.c             | 65 +++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/mount.h |  3 +-
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index ab4174a3c802..521cfd400d06 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2684,6 +2684,66 @@ static bool check_for_nsfs_mounts(struct mount *subtree)
 	return ret;
 }
 
+static int do_set_group(struct path *from_path, struct path *to_path)
+{
+	struct mount *from, *to;
+	int err;
+
+	from = real_mount(from_path->mnt);
+	to = real_mount(to_path->mnt);
+
+	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))
+		goto out;
+	if (!ns_capable(to->mnt_ns->user_ns, CAP_SYS_ADMIN))
+		goto out;
+
+	err = -EINVAL;
+	/* Setting sharing groups is only allowed across same superblock */
+	if (from->mnt.mnt_sb != to->mnt.mnt_sb)
+		goto out;
+
+	/* From mount root should be wider than To mount root */
+	if (!is_subdir(to->mnt.mnt_root, from->mnt.mnt_root))
+		goto out;
+
+	/* From mount should not have locked children in place of To's root */
+	if (has_locked_children(from, to->mnt.mnt_root))
+		goto out;
+
+	/* Setting sharing groups is only allowed on private mounts */
+	if (IS_MNT_SHARED(to) || IS_MNT_SLAVE(to))
+		goto out;
+
+	if (IS_MNT_SLAVE(from)) {
+		struct mount *m = from->mnt_master;
+
+		list_add(&to->mnt_slave, &m->mnt_slave_list);
+		to->mnt_master = m;
+	}
+
+	if (IS_MNT_SHARED(from)) {
+		to->mnt_group_id = from->mnt_group_id;
+		list_add(&to->mnt_share, &from->mnt_share);
+		set_mnt_shared(to);
+	}
+
+	err = 0;
+out:
+	namespace_unlock();
+	return err;
+}
+
 static int do_move_mount(struct path *old_path, struct path *new_path)
 {
 	struct mnt_namespace *ns;
@@ -3669,7 +3729,10 @@ SYSCALL_DEFINE5(move_mount,
 	if (ret < 0)
 		goto out_to;
 
-	ret = do_move_mount(&from_path, &to_path);
+	if (flags & MOVE_MOUNT_SET_GROUP)
+		ret = do_set_group(&from_path, &to_path);
+	else
+		ret = do_move_mount(&from_path, &to_path);
 
 out_to:
 	path_put(&to_path);
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index dd7a166fdf9c..4d93967f8aea 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -73,7 +73,8 @@
 #define MOVE_MOUNT_T_SYMLINKS		0x00000010 /* Follow symlinks on to path */
 #define MOVE_MOUNT_T_AUTOMOUNTS		0x00000020 /* Follow automounts on to path */
 #define MOVE_MOUNT_T_EMPTY_PATH		0x00000040 /* Empty to path permitted */
-#define MOVE_MOUNT__MASK		0x00000077
+#define MOVE_MOUNT_SET_GROUP		0x00000100 /* Set sharing group instead */
+#define MOVE_MOUNT__MASK		0x00000177
 
 /*
  * fsopen() flags.
-- 
2.31.1

Re: [PATCH v2] move_mount: allow to add a mount into an existing group

[Christian Brauner]

On Tue, Jul 13, 2021 at 02:56:36PM +0300, Pavel Tikhomirov wrote:
> Previously a sharing group (shared and master ids pair) can be only
> inherited when mount is created via bindmount. This patch adds an
> ability to add an existing private mount into an existing sharing group.
> 
> With this functionality one can first create the desired mount tree from
> only private mounts (without the need to care about undesired mount
> propagation or mount creation order implied by sharing group
> dependencies), and next then setup any desired mount sharing between
> those mounts in tree as needed.
> 
> This allows CRIU to restore any set of mount namespaces, mount trees and
> sharing group trees for a container.
> 
> We have many issues with restoring mounts in CRIU related to sharing
> groups and propagation:
> - reverse sharing groups vs mount tree order requires complex mounts
>   reordering which mostly implies also using some temporary mounts
> (please see https://lkml.org/lkml/2021/3/23/569 for more info)

Thanks for working on this. We can make good use of this flag as well
when setting up mount layouts and so can systemd so I'm happy to drive
this.

> 
> - mount() syscall creates tons of mounts due to propagation
> - mount re-parenting due to propagation
> - "Mount Trap" due to propagation
> - "Non Uniform" propagation, meaning that with different tricks with
>   mount order and temporary children-"lock" mounts one can create mount
>   trees which can't be restored without those tricks
> (see https://www.linuxplumbersconf.org/event/7/contributions/640/)
> 
> With this new functionality we can resolve all the problems with
> propagation at once.
> 
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Mattias Nissler <mnissler@chromium.org>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Andrei Vagin <avagin@gmail.com>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-api@vger.kernel.org
> Cc: lkml <linux-kernel@vger.kernel.org>
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> 
> ---
> This is a rework of "mnt: allow to add a mount into an existing group"
> patch from Andrei. https://lkml.org/lkml/2017/4/28/20
> 
> New do_set_group is similar to do_move_mount, but with many restrictions
> of do_move_mount removed and that's why:
> 
> 1) Allow "cross-namespace" sharing group set. If we allow operation only
> with mounts from current+anon mount namespace one would still be able to
> setns(from_mntns) + open_tree(from, OPEN_TREE_CLONE) + setns(to_mntns) +
> move_mount(anon, to, MOVE_MOUNT_SET_GROUP) to set sharing group to mount

That's similar to how we can do limited delegated mounting.

> in different mount namespace with source mount. But with this approach
> we would need to create anon mount namespace and mount copy each time,
> which is just a waste of resources. So instead lets just check if we are
> allowed to modify both mount namespaces (which looks equivalent to what
> setns-es and open_tree check).
> 
> 2) Allow operating on non-root dentry of the mount. As if we prohibit it
> this would require extra care from CRIU side in places where we wan't to
> copy sharing group from mount on host (for external mounts) and user
> gives us path to non-root dentry. I don't see any problem with
> referencing mount with any dentry for sharing group setting. Also there
> is no problem with referencing one by file and one by directory.

I would prefer to not do this as it doesn't line up with any other
mount modifying syscalls.

> 
> 3) Also checks wich only apply to actually moving mount which we have in
> do_move_mount and open_tree are skipped. We don't need to check
> MNT_LOCKED, unbindable, nsfs loops and ancestor relation as we don't
> move mounts.
> 
> Also let's add some new checks (offered by Andrei):
> 
> 1) Don't allow to copy sharing from mount with narrow root to a wider
> root, so that user does not have power to receive more propagations when
> user already has.
> 
> 2) Don't allow to copy sharing from mount with locked children for the
> same reason, as user shouldn't see propagations to areas overmounted by
> locked mounts (if the user could not already do it before sharing
> adjustment).
> 
> Security note: there would be no (new) loops in sharing groups tree,
> because this new move_mount(MOVE_MOUNT_SET_GROUP) operation only adds
> one _private_ mount to one group (without moving between groups), the
> sharing groups tree itself stays unchanged after it.
> 
> In Virtuozzo we have "mount-v2" implementation, based with the original
> kernel patch from Andrei, tested for almost a year and it actually
> decreased number of bugs with mounts a lot. One can take a look on the
> implementation of sharing group restore in CRIU in "mount-v2" here:
> 
> https://src.openvz.org/projects/OVZ/repos/criu/browse/criu/mount-v2.c#898
> 
> This works almost the same with current version of patch if we replace
> mount(MS_SET_GROUP) to move_mount(MOVE_MOUNT_SET_GROUP), please see
> super-draft port for mainstream criu, this at least passes
> non-user-namespaced mount tests (zdtm.py --mounts-v2 -f ns).
> 
> https://github.com/Snorch/criu/commits/mount-v2-poc
> 
> v2: Solve the problem mentioned by Andrei:
> - check mnt_root of "to" is in the sub-tree of mnt_root of "from"
> - also check "from" has no locked mounts in subroot of "to"
> 
> ---

Can you please add a simple test for this to selftests?

>  fs/namespace.c             | 65 +++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/mount.h |  3 +-
>  2 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index ab4174a3c802..521cfd400d06 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2684,6 +2684,66 @@ static bool check_for_nsfs_mounts(struct mount *subtree)
>  	return ret;
>  }
>  
> +static int do_set_group(struct path *from_path, struct path *to_path)
> +{
> +	struct mount *from, *to;
> +	int err;
> +
> +	from = real_mount(from_path->mnt);
> +	to = real_mount(to_path->mnt);
> +
> +	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))
> +		goto out;
> +	if (!ns_capable(to->mnt_ns->user_ns, CAP_SYS_ADMIN))
> +		goto out;
> +
> +	err = -EINVAL;
> +	/* Setting sharing groups is only allowed across same superblock */
> +	if (from->mnt.mnt_sb != to->mnt.mnt_sb)
> +		goto out;
> +
> +	/* From mount root should be wider than To mount root */
> +	if (!is_subdir(to->mnt.mnt_root, from->mnt.mnt_root))
> +		goto out;
> +
> +	/* From mount should not have locked children in place of To's root */
> +	if (has_locked_children(from, to->mnt.mnt_root))
> +		goto out;
> +
> +	/* Setting sharing groups is only allowed on private mounts */
> +	if (IS_MNT_SHARED(to) || IS_MNT_SLAVE(to))
> +		goto out;
> +
> +	if (IS_MNT_SLAVE(from)) {
> +		struct mount *m = from->mnt_master;
> +
> +		list_add(&to->mnt_slave, &m->mnt_slave_list);
> +		to->mnt_master = m;
> +	}
> +
> +	if (IS_MNT_SHARED(from)) {
> +		to->mnt_group_id = from->mnt_group_id;
> +		list_add(&to->mnt_share, &from->mnt_share);
> +		set_mnt_shared(to);
> +	}

Should we report EINVAL if a private mount is passed? Though that would
require you to know in advance whether this is one so might actually be
worth doing it like you do now.

> +
> +	err = 0;
> +out:
> +	namespace_unlock();
> +	return err;
> +}
> +
>  static int do_move_mount(struct path *old_path, struct path *new_path)

Technically this could also be part of mount_setattr() (You'd need a new
struct member though.) but it's fine here too.

>  {
>  	struct mnt_namespace *ns;
> @@ -3669,7 +3729,10 @@ SYSCALL_DEFINE5(move_mount,
>  	if (ret < 0)
>  		goto out_to;
>  
> -	ret = do_move_mount(&from_path, &to_path);
> +	if (flags & MOVE_MOUNT_SET_GROUP)
> +		ret = do_set_group(&from_path, &to_path);
> +	else
> +		ret = do_move_mount(&from_path, &to_path);
>  
>  out_to:
>  	path_put(&to_path);
> diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
> index dd7a166fdf9c..4d93967f8aea 100644
> --- a/include/uapi/linux/mount.h
> +++ b/include/uapi/linux/mount.h
> @@ -73,7 +73,8 @@
>  #define MOVE_MOUNT_T_SYMLINKS		0x00000010 /* Follow symlinks on to path */
>  #define MOVE_MOUNT_T_AUTOMOUNTS		0x00000020 /* Follow automounts on to path */
>  #define MOVE_MOUNT_T_EMPTY_PATH		0x00000040 /* Empty to path permitted */
> -#define MOVE_MOUNT__MASK		0x00000077
> +#define MOVE_MOUNT_SET_GROUP		0x00000100 /* Set sharing group instead */
> +#define MOVE_MOUNT__MASK		0x00000177
>  
>  /*
>   * fsopen() flags.
> -- 
> 2.31.1
> 

Re: [PATCH v2] move_mount: allow to add a mount into an existing group

[Pavel Tikhomirov]

On 13.07.2021 17:40, Christian Brauner wrote:
> On Tue, Jul 13, 2021 at 02:56:36PM +0300, Pavel Tikhomirov wrote:
>> Previously a sharing group (shared and master ids pair) can be only
>> inherited when mount is created via bindmount. This patch adds an
>> ability to add an existing private mount into an existing sharing group.
>>
>> With this functionality one can first create the desired mount tree from
>> only private mounts (without the need to care about undesired mount
>> propagation or mount creation order implied by sharing group
>> dependencies), and next then setup any desired mount sharing between
>> those mounts in tree as needed.
>>
>> This allows CRIU to restore any set of mount namespaces, mount trees and
>> sharing group trees for a container.
>>
>> We have many issues with restoring mounts in CRIU related to sharing
>> groups and propagation:
>> - reverse sharing groups vs mount tree order requires complex mounts
>>    reordering which mostly implies also using some temporary mounts
>> (please see https://lkml.org/lkml/2021/3/23/569 for more info)
> 
> Thanks for working on this. We can make good use of this flag as well
> when setting up mount layouts and so can systemd so I'm happy to drive
> this.

I really appreciate it, Thanks Christian!

> 
>>
>> - mount() syscall creates tons of mounts due to propagation
>> - mount re-parenting due to propagation
>> - "Mount Trap" due to propagation
>> - "Non Uniform" propagation, meaning that with different tricks with
>>    mount order and temporary children-"lock" mounts one can create mount
>>    trees which can't be restored without those tricks
>> (see https://www.linuxplumbersconf.org/event/7/contributions/640/)
>>
>> With this new functionality we can resolve all the problems with
>> propagation at once.
>>
>> Cc: Eric W. Biederman <ebiederm@xmission.com>
>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> Cc: Christian Brauner <christian.brauner@ubuntu.com>
>> Cc: Mattias Nissler <mnissler@chromium.org>
>> Cc: Aleksa Sarai <cyphar@cyphar.com>
>> Cc: Andrei Vagin <avagin@gmail.com>
>> Cc: linux-fsdevel@vger.kernel.org
>> Cc: linux-api@vger.kernel.org
>> Cc: lkml <linux-kernel@vger.kernel.org>
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>>
>> ---
>> This is a rework of "mnt: allow to add a mount into an existing group"
>> patch from Andrei. https://lkml.org/lkml/2017/4/28/20
>>
>> New do_set_group is similar to do_move_mount, but with many restrictions
>> of do_move_mount removed and that's why:
>>
>> 1) Allow "cross-namespace" sharing group set. If we allow operation only
>> with mounts from current+anon mount namespace one would still be able to
>> setns(from_mntns) + open_tree(from, OPEN_TREE_CLONE) + setns(to_mntns) +
>> move_mount(anon, to, MOVE_MOUNT_SET_GROUP) to set sharing group to mount
> 
> That's similar to how we can do limited delegated mounting.

Hope it means that we can allow it then. Because (1) looks like the most 
expensive thing from CRIU point of view to disallow.

> 
>> in different mount namespace with source mount. But with this approach
>> we would need to create anon mount namespace and mount copy each time,
>> which is just a waste of resources. So instead lets just check if we are
>> allowed to modify both mount namespaces (which looks equivalent to what
>> setns-es and open_tree check).
>>
>> 2) Allow operating on non-root dentry of the mount. As if we prohibit it
>> this would require extra care from CRIU side in places where we wan't to
>> copy sharing group from mount on host (for external mounts) and user
>> gives us path to non-root dentry. I don't see any problem with
>> referencing mount with any dentry for sharing group setting. Also there
>> is no problem with referencing one by file and one by directory.
> 
> I would prefer to not do this as it doesn't line up with any other
> mount modifying syscalls.

Actually open_tree(from, OPEN_TREE_CLONE) does not check "from" to be 
root dentry. And also when we do bind-mounts "from" can also be any 
non-root file/directory. But that's a bit different case of mount 
creation, not configuration. So yes, it would probably become more 
secure with those checks. -> Will add them.

> 
>>
>> 3) Also checks wich only apply to actually moving mount which we have in
>> do_move_mount and open_tree are skipped. We don't need to check
>> MNT_LOCKED, unbindable, nsfs loops and ancestor relation as we don't
>> move mounts.
>>
>> Also let's add some new checks (offered by Andrei):
>>
>> 1) Don't allow to copy sharing from mount with narrow root to a wider
>> root, so that user does not have power to receive more propagations when
>> user already has.
>>
>> 2) Don't allow to copy sharing from mount with locked children for the
>> same reason, as user shouldn't see propagations to areas overmounted by
>> locked mounts (if the user could not already do it before sharing
>> adjustment).
>>
>> Security note: there would be no (new) loops in sharing groups tree,
>> because this new move_mount(MOVE_MOUNT_SET_GROUP) operation only adds
>> one _private_ mount to one group (without moving between groups), the
>> sharing groups tree itself stays unchanged after it.
>>
>> In Virtuozzo we have "mount-v2" implementation, based with the original
>> kernel patch from Andrei, tested for almost a year and it actually
>> decreased number of bugs with mounts a lot. One can take a look on the
>> implementation of sharing group restore in CRIU in "mount-v2" here:
>>
>> https://src.openvz.org/projects/OVZ/repos/criu/browse/criu/mount-v2.c#898
>>
>> This works almost the same with current version of patch if we replace
>> mount(MS_SET_GROUP) to move_mount(MOVE_MOUNT_SET_GROUP), please see
>> super-draft port for mainstream criu, this at least passes
>> non-user-namespaced mount tests (zdtm.py --mounts-v2 -f ns).
>>
>> https://github.com/Snorch/criu/commits/mount-v2-poc
>>
>> v2: Solve the problem mentioned by Andrei:
>> - check mnt_root of "to" is in the sub-tree of mnt_root of "from"
>> - also check "from" has no locked mounts in subroot of "to"
>>
>> ---
> 
> Can you please add a simple test for this to selftests?

Ok.

> 
>>   fs/namespace.c             | 65 +++++++++++++++++++++++++++++++++++++-
>>   include/uapi/linux/mount.h |  3 +-
>>   2 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/namespace.c b/fs/namespace.c
>> index ab4174a3c802..521cfd400d06 100644
>> --- a/fs/namespace.c
>> +++ b/fs/namespace.c
>> @@ -2684,6 +2684,66 @@ static bool check_for_nsfs_mounts(struct mount *subtree)
>>   	return ret;
>>   }
>>   
>> +static int do_set_group(struct path *from_path, struct path *to_path)
>> +{
>> +	struct mount *from, *to;
>> +	int err;
>> +
>> +	from = real_mount(from_path->mnt);
>> +	to = real_mount(to_path->mnt);
>> +
>> +	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))
>> +		goto out;
>> +	if (!ns_capable(to->mnt_ns->user_ns, CAP_SYS_ADMIN))
>> +		goto out;
>> +
>> +	err = -EINVAL;
>> +	/* Setting sharing groups is only allowed across same superblock */
>> +	if (from->mnt.mnt_sb != to->mnt.mnt_sb)
>> +		goto out;
>> +
>> +	/* From mount root should be wider than To mount root */
>> +	if (!is_subdir(to->mnt.mnt_root, from->mnt.mnt_root))
>> +		goto out;
>> +
>> +	/* From mount should not have locked children in place of To's root */
>> +	if (has_locked_children(from, to->mnt.mnt_root))
>> +		goto out;
>> +
>> +	/* Setting sharing groups is only allowed on private mounts */
>> +	if (IS_MNT_SHARED(to) || IS_MNT_SLAVE(to))
>> +		goto out;
>> +
>> +	if (IS_MNT_SLAVE(from)) {
>> +		struct mount *m = from->mnt_master;
>> +
>> +		list_add(&to->mnt_slave, &m->mnt_slave_list);
>> +		to->mnt_master = m;
>> +	}
>> +
>> +	if (IS_MNT_SHARED(from)) {
>> +		to->mnt_group_id = from->mnt_group_id;
>> +		list_add(&to->mnt_share, &from->mnt_share);
>> +		set_mnt_shared(to);
>> +	}
> 
> Should we report EINVAL if a private mount is passed? Though that would
> require you to know in advance whether this is one so might actually be
> worth doing it like you do now.

Yes, sounds reasonable. In CRIU case we always know in advance. If we 
try to restore some sharing it's good to know that it was actually 
restored and not private by some mistake. -> Will do.

> 
>> +
>> +	err = 0;
>> +out:
>> +	namespace_unlock();
>> +	return err;
>> +}
>> +
>>   static int do_move_mount(struct path *old_path, struct path *new_path)
> 
> Technically this could also be part of mount_setattr() (You'd need a new
> struct member though.) but it's fine here too.

I've looked at mount_setattr(), I saw that I would need to add to_dfd + 
to_path into mount_attr there while in move_mount we already have 
everything at hand, that's why I chose move_mount.

> 
>>   {
>>   	struct mnt_namespace *ns;
>> @@ -3669,7 +3729,10 @@ SYSCALL_DEFINE5(move_mount,
>>   	if (ret < 0)
>>   		goto out_to;
>>   
>> -	ret = do_move_mount(&from_path, &to_path);
>> +	if (flags & MOVE_MOUNT_SET_GROUP)
>> +		ret = do_set_group(&from_path, &to_path);
>> +	else
>> +		ret = do_move_mount(&from_path, &to_path);
>>   
>>   out_to:
>>   	path_put(&to_path);
>> diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
>> index dd7a166fdf9c..4d93967f8aea 100644
>> --- a/include/uapi/linux/mount.h
>> +++ b/include/uapi/linux/mount.h
>> @@ -73,7 +73,8 @@
>>   #define MOVE_MOUNT_T_SYMLINKS		0x00000010 /* Follow symlinks on to path */
>>   #define MOVE_MOUNT_T_AUTOMOUNTS		0x00000020 /* Follow automounts on to path */
>>   #define MOVE_MOUNT_T_EMPTY_PATH		0x00000040 /* Empty to path permitted */
>> -#define MOVE_MOUNT__MASK		0x00000077
>> +#define MOVE_MOUNT_SET_GROUP		0x00000100 /* Set sharing group instead */
>> +#define MOVE_MOUNT__MASK		0x00000177
>>   
>>   /*
>>    * fsopen() flags.
>> -- 
>> 2.31.1
>>

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.