Re: [PATCH] fs: allow clone_private_mount() for a path on real rootfs

["KONDO KAZUMA(近藤 和真)" <kazuma-kondo@nec.com>]

On 2025/05/15 4:02, Al Viro wrote:
> On Wed, May 14, 2025 at 08:37:54AM +0000, KONDO KAZUMA(近藤 和真) wrote:
>> On 2025/05/14 11:43, Al Viro wrote:
>>> On Wed, May 14, 2025 at 12:25:58AM +0000, KONDO KAZUMA(近藤 和真) wrote:
>>>
>>>> @@ -2482,17 +2482,13 @@ struct vfsmount *clone_private_mount(const struct path *path)
>>>>  	if (IS_MNT_UNBINDABLE(old_mnt))
>>>>  		return ERR_PTR(-EINVAL);
>>>>  
>>>> -	if (mnt_has_parent(old_mnt)) {
>>>> +	if (!is_mounted(&old_mnt->mnt))
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>> +	if (mnt_has_parent(old_mnt) || !is_anon_ns(old_mnt->mnt_ns)) {
>>>>  		if (!check_mnt(old_mnt))
>>>>  			return ERR_PTR(-EINVAL);
>>>>  	} else {
>>>> -		if (!is_mounted(&old_mnt->mnt))
>>>> -			return ERR_PTR(-EINVAL);
>>>> -
>>>> -		/* Make sure this isn't something purely kernel internal. */
>>>> -		if (!is_anon_ns(old_mnt->mnt_ns))
>>>> -			return ERR_PTR(-EINVAL);
>>>> -
>>>>  		/* Make sure we don't create mount namespace loops. */
>>>>  		if (!check_for_nsfs_mounts(old_mnt))
>>>>  			return ERR_PTR(-EINVAL);
>>>
>>> Not the right way to do that.  What we want is
>>>
>>> 	/* ours are always fine */
>>> 	if (!check_mnt(old_mnt)) {
>>> 		/* they'd better be mounted _somewhere */
>>> 		if (!is_mounted(old_mnt))
>>> 			return -EINVAL;
>>> 		/* no other real namespaces; only anon */
>>> 		if (!is_anon_ns(old_mnt->mnt_ns))
>>> 			return -EINVAL;
>>> 		/* ... and root of that anon */
>>> 		if (mnt_has_parent(old_mnt))
>>> 			return -EINVAL;
>>> 		/* Make sure we don't create mount namespace loops. */
>>> 		if (!check_for_nsfs_mounts(old_mnt))
>>> 			return ERR_PTR(-EINVAL);
>>> 	}
>>
>> Hello Al Viro,
>>
>> Thank you for your comment.
>> That code can solve my problem, and it seems to be better!
> 
> BTW, see https://lore.kernel.org/all/20250506194849.GT2023217@ZenIV/ for
> discussion about a week ago when that got noticed:
> 
> || In case of clone_private_mount(), though, there's nothing wrong
> || with "clone me a subtree of absolute root", so it has to be
> || done other way round - check if it's ours first, then in "not
> || ours" case check that it's a root of anon namespace.
> ||
> || Failing btrfs mount has ended up with upper layer pathname
> || pointing to initramfs directory where btrfs would've been
> || mounted, which had walked into that corner case.  In your
> || case the problem has already happened by that point, but on
> || a setup a-la X Terminal it would cause trouble...
> 
> Looks like such setups are less theoretical than I thought.
> 
>> So, I will revise my patch and resend it.
> 
> Probably worth gathering the comments in one place.  Something like
> 	/*
> 	 * Check if the source is acceptable; anything mounted in
> 	 * our namespace is fine, otherwise it must be the root of
> 	 * some anon namespace and we need to make sure no namespace
> 	 * loops get created.
> 	 */
> 	if (!check_mnt(old_mnt)) {
> 		if (!is_mounted(&old_mnt->mnt) ||
> 		    !is_anon_ns(old_mnt->mnt_ns) ||
> 		    mnt_has_parent(old_mnt))
> 			return ERR_PTR(-EINVAL);
> 		if (!check_for_nsfs_mounts(old_mnt))
> 			return ERR_PTR(-EINVAL);
> 	}
> might be easier to follow.

I think it's good idea.
I will send a patch with that comment.

Thanks