[PATCH 00/10] mount: a few additional bugfixes

[PATCH 00/10] mount: a few additional bugfixes

[Christian Brauner]

Here's a set of additional mount fixes that fell out of my recent look
at the mount code:

- Fix a UAF in mnt_change_mountpoint().
- Fix a UAF in statmount().
- FIX a UAF and various associated bugs in clone_mnt()
- Prevent copying an unmounted mount tree.
- Avoid pointless work in mount_setattr().
- Refuse fspick() on kernel internal superblocks.
- Simplify dissolve_on_fput() and unshare().

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Christian Brauner (10):
      namespace: reset the old parent's ->overmount in mnt_change_mountpoint()
      statmount: read the parent of an unmounted mount under mount_lock
      namespace: don't inherit MNT_UMOUNT in clone_mnt()
      namespace: don't copy an unmounted mount tree
      namespace: check the target before walking it in do_mount_setattr()
      fs: refuse fspick() on internal superblocks
      fs: put the old fs_struct before the old namespaces in unshare()
      namespace: drop the file's reference first in dissolve_on_fput()
      selftests/filesystems: check that an unmounted mount tree isn't walked
      selftests/filesystems: check that a dead mount's overmount isn't followed

 fs/file_table.c                                    |   3 +-
 fs/fsopen.c                                        |   3 +
 fs/namespace.c                                     |  37 +-
 include/linux/mount.h                              |   2 +-
 kernel/fork.c                                      |   9 +-
 tools/testing/selftests/Makefile                   |   1 +
 .../selftests/filesystems/mount_cycle/.gitignore   |   3 +
 .../selftests/filesystems/mount_cycle/Makefile     |   6 +
 .../mount_cycle/overmount_reparent_test.c          | 181 ++++++++
 .../filesystems/mount_cycle/unmounted_tree_test.c  | 484 +++++++++++++++++++++
 10 files changed, 719 insertions(+), 10 deletions(-)
---
base-commit: 762a6411a5a7aa4119570bae590662312ccb4be3
change-id: 20260926-work-mount-fixes-2-90d619ace163

[PATCH 01/10] namespace: reset the old parent's ->overmount in mnt_change_mountpoint()

[Christian Brauner]

__umount_mnt() resets ->overmount of the parent when it unmounts the
mount that overmounts it. mnt_change_mountpoint() doesn't. It moves the
mount to its new parent and leaves the old one pointing at it.

For a mount that is moved beneath in attach_recursive_mnt() that doesn't
matter. The new mount has already reset ->overmount of the old parent by
the time the old top mount is moved onto it.

It matters for reparent() though. propagate_umount() moves a surviving
overmount onto the first ancestor that isn't going to be unmounted. The
mount it was taken from is unmounted immediately afterwards. If a file
descriptor keeps that unmounted mount alive ->overmount points at the
moved mount. That's a dangling pointer and UAF as the moved mount is
unmounted and freed.

do_lock_mount() follows ->overmount from the target path under
mount_lock. With a file descriptor on such a mount as the target of a
mount beneath topmost_overmount() walks into the freed mount and
do_lock_mount() takes references on whatever it finds there.

So reset ->overmount of the old parent in mnt_change_mountpoint() the
same way __umount_mnt() does. A mount's ->overmount is then always a
mount that is attached to it.

Fixes: ffdc52fbbd58 ("prevent mount hash conflicts")
Cc: stable@vger.kernel.org # v6.17+
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index 73215f84e04d..230f95675a93 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1069,7 +1069,10 @@ static void attach_mnt(struct mount *mnt, struct mount *parent,
 void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp, struct mount *mnt)
 {
 	struct mountpoint *old_mp = mnt->mnt_mp;
+	struct mount *old_parent = mnt->mnt_parent;
 
+	if (old_parent->overmount == mnt)
+		old_parent->overmount = NULL;
 	list_del_init(&mnt->mnt_child);
 	hlist_del_init(&mnt->mnt_mp_list);
 	hlist_del_init_rcu(&mnt->mnt_hash);

-- 
2.53.0

[PATCH 02/10] statmount: read the parent of an unmounted mount under mount_lock

[Christian Brauner]

statmount_mnt_basic() reads the ids of the parent through m->mnt_parent
with namespace_sem held for reading. That's fine for a mount that is
in a namespace. Everything above it is pinned by that namespace.

STATMOUNT_BY_FD also accepts a file descriptor on a mount that has
been unmounted on purpose and that only the file descriptor keeps alive.
If that mount stayed attached to its parent when it was unmounted the
parent is unmounted as well and nothing pins it for the child.

The final mntput() unmounts the children under mount_lock alone and
frees the parent afterwards. Concurrently, statmount_mnt_basic() reads
m->mnt_parent twice while that can happen and can easily dereference a
mount that's already gone.

Read the parent of an unmounted mount under mount_lock. The parent
can't be put while it is held and both ids come from the same mount. A
mounted mount's parent is pinned by the namespace and a mount can't
leave its namespace while namespace_sem is held, so the common case
takes no global lock.

Fixes: 0e5032237ee5 ("statmount: accept fd as a parameter")
Cc: stable@vger.kernel.org # v7.0+
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/namespace.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 230f95675a93..d1e8dd2e1e8e 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5335,15 +5335,25 @@ static void statmount_sb_basic(struct kstatmount *s)
 	s->sm.sb_flags = sb->s_flags & (SB_RDONLY|SB_SYNCHRONOUS|SB_DIRSYNC|SB_LAZYTIME);
 }
 
+static void statmount_mnt_parent(struct kstatmount *s, const struct mount *m)
+{
+	s->sm.mnt_parent_id = m->mnt_parent->mnt_id_unique;
+	s->sm.mnt_parent_id_old = m->mnt_parent->mnt_id;
+}
+
 static void statmount_mnt_basic(struct kstatmount *s)
 {
 	struct mount *m = real_mount(s->mnt);
 
 	s->sm.mask |= STATMOUNT_MNT_BASIC;
 	s->sm.mnt_id = m->mnt_id_unique;
-	s->sm.mnt_parent_id = m->mnt_parent->mnt_id_unique;
 	s->sm.mnt_id_old = m->mnt_id;
-	s->sm.mnt_parent_id_old = m->mnt_parent->mnt_id;
+	/* An unmounted mount is cut loose from its parent under mount_lock alone. */
+	if (likely(is_mounted(s->mnt)))
+		statmount_mnt_parent(s, m);
+	else
+		scoped_guard(mount_locked_reader)
+			statmount_mnt_parent(s, m);
 	s->sm.mnt_attr = mnt_to_attr_flags(&m->mnt);
 	s->sm.mnt_propagation = mnt_to_propagation_flags(m);
 	s->sm.mnt_peer_group = m->mnt_group_id;

-- 
2.53.0

[PATCH 03/10] namespace: don't inherit MNT_UMOUNT in clone_mnt()

[Christian Brauner]

MNT_UMOUNT gets raised on a mount that umount_tree() or umount_one() has
taken out of its namespace. It's never cleared. __detach_mounts()
unhooks a mount that carries it instead of unmounting it.

disconnect_mount() lets an unmounted child stay attached only if its
parent carries it. propagate_umount() takes it as the sign that a mount
is going to be unmounted when it decides what else can be unmounted,
where a chain of locked mounts ends and where a for reparenting an
overmount.

clone_mnt() copies the flags of the source and only masks
MNT_INTERNAL_FLAGS which doesn't include MNT_UMOUNT. A mount that was
lazily unmounted but is kept alive by a file descriptor can still be
copied when its root is an nsfs or pidfs file because may_copy_tree()
accepts those before checking whether the source is mounted at all:

  mount --bind /proc/self/ns/net /tmp/x
  exec 3< /tmp/x
  umount -l /tmp/x
  mount --bind /proc/self/fd/3 /tmp/y

The mount on /tmp/y is attached to the caller's namespace with
MNT_UMOUNT already set and every copy of it inherits the flag again.

That causes a number of bugs:

- Unlink its mountpoint from a mount namespace where it isn't one.
  __detach_mounts() takes it for a mount that was unmounted earlier,
  unhooks it from its parent and drops the reference the namespace
  holds on it. It stays in the namespace with one reference too few
  and as its own parent, so it can't be unmounted through the file
  descriptor anymore and the namespace never finds it again. When the
  namespace goes away it's leaked and points to a freed struct
  mnt_namespace that may_change_propagation() and anon_ns_root() read
  on the next propagation change or mount_setattr() through the file
  descriptor.

- Make it shared and unmount a peer. trace_transfers() follows the
  flag from the peer into it and strips it from its peer group.

- Let a propagated umount reach it. gather_candidates() marks it with
  T_UMOUNT_CANDIDATE but never puts it on the candidate list, so the
  mark is never cleared and every later propagated umount skips it. If
  it's the parent of a locked mount handle_locked() takes it as the
  cutoff and commits the locked child that should have stayed and
  disconnect_mount() leaves that child attached to it, unmounted under
  a mounted parent, which is what disconnect_mount() exists to
  prevent. When the namespace goes away umount_tree() visits the child
  a second time. And once __detach_mounts() has made it its own parent
  reparent() never finds the end of the chain of unmounted parents.

A clone is a new mount that has never been unmounted. Add MNT_UMOUNT
to MNT_INTERNAL_FLAGS so clone_mnt() never copies it. And warn when
umount_tree() reaches a mount that already has it set.

Fixes: 590ce4bcbfb4 ("mnt: Add MNT_UMOUNT flag")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/namespace.c        | 2 ++
 include/linux/mount.h | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index d1e8dd2e1e8e..6d4ecde682df 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1799,6 +1799,8 @@ static void umount_tree(struct mount *mnt, enum umount_tree_flags how)
 
 	/* Gather the mounts to umount */
 	for (p = mnt; p; p = next_mnt(p, mnt)) {
+		/* A mount is unmounted once. */
+		VFS_WARN_ON_ONCE(p->mnt.mnt_flags & MNT_UMOUNT);
 		p->mnt.mnt_flags |= MNT_UMOUNT;
 		if (mnt_ns_attached(p))
 			move_from_ns(p);
diff --git a/include/linux/mount.h b/include/linux/mount.h
index acfe7ef86a1b..b16ba5d45605 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -52,7 +52,7 @@ enum mount_flags {
 	MNT_ATIME_MASK = MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME,
 
 	MNT_INTERNAL_FLAGS = MNT_INTERNAL | MNT_DOOMED |
-			     MNT_SYNC_UMOUNT | MNT_LOCKED
+			     MNT_SYNC_UMOUNT | MNT_LOCKED | MNT_UMOUNT
 };
 
 struct vfsmount {

-- 
2.53.0

Re: [PATCH 03/10] namespace: don't inherit MNT_UMOUNT in clone_mnt()

[Al Viro]

On Sat, Sep 26, 2026 at 03:44:26PM +0200, Christian Brauner wrote:

> clone_mnt() copies the flags of the source and only masks
> MNT_INTERNAL_FLAGS which doesn't include MNT_UMOUNT. A mount that was
> lazily unmounted but is kept alive by a file descriptor can still be
> copied when its root is an nsfs or pidfs file because may_copy_tree()
> accepts those before checking whether the source is mounted at all:
> 
>   mount --bind /proc/self/ns/net /tmp/x
>   exec 3< /tmp/x
>   umount -l /tmp/x
>   mount --bind /proc/self/fd/3 /tmp/y
> 
> The mount on /tmp/y is attached to the caller's namespace with
> MNT_UMOUNT already set and every copy of it inherits the flag again.
> 
> That causes a number of bugs:

[obviously]

I agree about adding it to MNT_INTERNAL_FLAGS, but I wonder if we should
ever allow to clone MNT_UMOUNT ones in the first place...

Re: [PATCH 03/10] namespace: don't inherit MNT_UMOUNT in clone_mnt()

[Christian Brauner]

On Sun, Oct 18, 2026 at 05:32:45AM +0100, Al Viro wrote:
> I agree about adding it to MNT_INTERNAL_FLAGS, but I wonder if we should
> ever allow to clone MNT_UMOUNT ones in the first place...

The next patch refuses to copy an unmounted tree recursively. Copying
just the root is kept on purpose...

Becaues a bind mount of an nsfs or pidfs file that got lazily unmounted
but is still pinned by an fd can be recreated from that fd and I'd
rather not break that...

With the flag not inherited anymore the copy is just a regular mount
with a reference on the old root. So I don't think refusing it buys us
anything.

[PATCH 04/10] namespace: don't copy an unmounted mount tree

[Christian Brauner]

may_copy_tree() accepts a mount whose root is an nsfs or pidfs file
before it checks whether that mount is mounted. This exception exists
for nsfs and pidfs mounts. They are part of no mount namespace so
neither check_mnt() nor is_mounted() would let them through and
bind-mounting a nsfsfd or a pidfd wouldn't work.

Problem is that exception also admits a bind mount of such a file when
it was lazily unmounted and is only kept alive by a file descriptor:

  mount --bind /proc/self/ns/net /tmp/x
  exec 3< /tmp/x
  umount -l /tmp/x
  mount --rbind /proc/self/fd/3 /tmp/y

With MS_REC or AT_RECURSIVE copy_tree() doesn't stop at the unmounted
mount. It copies every mount that stayed attached to it as well. So if
you have a bunch of overmounts over the nsfsfd or the pidfd the whole
unmounted stack is copied. That's a bug.

So refuse to walk an unmounted tree. Copying its root alone stays
possible. So a bind mount of a namespace that is gone can still be
recreated from a file descriptor on the old one. The file descriptor
pins that mount and its root and __has_locked_children() only reads the
direct children which don't leave the parent's list without
namespace_sem. The internal mounts carry MNT_NS_INTERNAL instead of NULL
and are still walked. Copying an unmounted tree recursively fails with
EINVAL.

Fixes: e149ed2b805f ("take the targets of /proc/*/ns/* symlinks to separate fs")
Fixes: ef4144ac2dec ("pidfs: allow bind-mounts")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index 6d4ecde682df..c7e252eb249e 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3007,6 +3007,9 @@ static struct mount *__do_loopback(const struct path *old_path,
 	if (!may_copy_tree(old_path))
 		return ERR_PTR(-EINVAL);
 
+	if (recurse && !old->mnt_ns)
+		return ERR_PTR(-EINVAL);
+
 	if (!recurse && __has_locked_children(old, old_path->dentry))
 		return ERR_PTR(-EINVAL);
 

-- 
2.53.0

[PATCH 05/10] namespace: check the target before walking it in do_mount_setattr()

[Christian Brauner]

When mount_setattr() changes the propagation type to MS_SHARED
do_mount_setattr() takes namespace_sem and calls invent_group_ids()
which walks the whole target tree with next_mnt() and allocates a peer
group id for every mount that doesn't have one.

Whether the caller may touch the target at all is only checked
afterwards under mount_lock. If it may not, cleanup_group_ids() walks
the tree a second time on the way out to free the ids again.

A detached tree that a file descriptor keeps alive, a mount in another
mount namespace or a mount below the root of an anonymous mount
namespace does a bunch of allocation work that is completely useless
because it will fail and revert right after.

Do the right thing and check the target right after taking namespace_sem
instead. A mount leaves its namespace only under namespace_sem so the
mount stays stable. The check under mount_lock stays for the case where
no propagation change is requested and namespace_sem isn't taken.
Callers get the same EINVAL as before.

Fixes: 2a1867219c7b ("fs: add mount_setattr()")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/namespace.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index c7e252eb249e..de28fe9058ab 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4976,6 +4976,11 @@ static int do_mount_setattr(const struct path *path, struct mount_kattr *kattr)
 		 * propagation.
 		 */
 		namespace_lock();
+		/* invent_group_ids() walks the tree, only walk a mounted one */
+		if (!anon_ns_root(mnt) && !check_mnt(mnt)) {
+			namespace_unlock();
+			return -EINVAL;
+		}
 		if (kattr->propagation == MS_SHARED) {
 			err = invent_group_ids(mnt, kattr->kflags & MOUNT_KATTR_RECURSE);
 			if (err) {
@@ -4988,6 +4993,7 @@ static int do_mount_setattr(const struct path *path, struct mount_kattr *kattr)
 	err = -EINVAL;
 	lock_mount_hash();
 
+	/* Checked under namespace_sem already if the propagation changes. */
 	if (!anon_ns_root(mnt) && !check_mnt(mnt))
 		goto out;
 

-- 
2.53.0

[PATCH 06/10] fs: refuse fspick() on internal superblocks

[Christian Brauner]

There's a few instances where a internal superblock is reachable by
fspick(). Block that. graft_tree() and fsmount() already refuse them.
fanotify refuses mount and superblock marks on them for the same reason.

Fixes: cf3cba4a429b ("vfs: syscall: Add fspick() to select a superblock for reconfiguration")
Cc: stable@vger.kernel.org # v5.2+
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/fsopen.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/fsopen.c b/fs/fsopen.c
index ae19e5136598..9d5a7a22b529 100644
--- a/fs/fsopen.c
+++ b/fs/fsopen.c
@@ -190,6 +190,9 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
 	ret = -EINVAL;
 	if (target.mnt->mnt_root != target.dentry)
 		goto err_path;
+	/* kernel-internal superblocks are nobody's to reconfigure */
+	if (target.dentry->d_sb->s_flags & SB_NOUSER)
+		goto err_path;
 
 	fc = fs_context_for_reconfigure(target.dentry, 0, 0);
 	if (IS_ERR(fc)) {

-- 
2.53.0

[PATCH 07/10] fs: put the old fs_struct before the old namespaces in unshare()

[Christian Brauner]

unshare(CLONE_NEWNS) always copies the fs_struct and copy_mnt_ns()
points root and pwd of that copy into the new mount namespace. The old
fs_struct pins the old root and pwd until ksys_unshare() frees it at the
end and switch_task_namespaces() will already put the old mount
namespace before.

This causes pointless work in user namespaces because the copied mounts
are locked and stay connected. Switch and free the fs_struct before
switching the namespaces. The old root and pwd are wasted while their
mount namespace is still alive which also keeps both puts on the
mntput() fast path.

setns() already does this right as it updates root and pwd before the
namespaces are switched.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 kernel/fork.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 416758c8a3d4..da48168c504f 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -3312,14 +3312,17 @@ int ksys_unshare(unsigned long unshare_flags)
 			shm_init_task(current);
 		}
 
+		if (new_fs) {
+			new_fs = switch_fs_struct(new_fs);
+			if (new_fs)
+				free_fs_struct(no_free_ptr(new_fs));
+		}
+
 		if (new_nsproxy) {
 			switch_task_namespaces(current, new_nsproxy);
 			new_nsproxy = NULL;
 		}
 
-		if (new_fs)
-			new_fs = switch_fs_struct(new_fs);
-
 		if (new_fd) {
 			guard(task_lock)(current);
 			swap(current->files, new_fd);

-- 
2.53.0

Re: [PATCH 07/10] fs: put the old fs_struct before the old namespaces in unshare()

[Bradley Morgan]

On 26 September 2026 14:44:30 BST, Christian Brauner <brauner@kernel.org>
wrote:
>unshare(CLONE_NEWNS) always copies the fs_struct and copy_mnt_ns()
>points root and pwd of that copy into the new mount namespace. The old
>fs_struct pins the old root and pwd until ksys_unshare() frees it at the
>end and switch_task_namespaces() will already put the old mount
>namespace before.
>
>This causes pointless work in user namespaces because the copied mounts
>are locked and stay connected. Switch and free the fs_struct before
>switching the namespaces. The old root and pwd are wasted while their
>mount namespace is still alive which also keeps both puts on the
>mntput() fast path.
>
>setns() already does this right as it updates root and pwd before the
>namespaces are switched.
>

I can't see a issue with this, unless I'm dumb and crazy

Reviewed-by: Bradley Morgan <brads@mainlining.org>




>Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
>---
> kernel/fork.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
>diff --git a/kernel/fork.c b/kernel/fork.c
>index 416758c8a3d4..da48168c504f 100644
>--- a/kernel/fork.c
>+++ b/kernel/fork.c
>@@ -3312,14 +3312,17 @@ int ksys_unshare(unsigned long unshare_flags)
> 			shm_init_task(current);
> 		}
> 
>+		if (new_fs) {
>+			new_fs = switch_fs_struct(new_fs);
>+			if (new_fs)
>+			

Ack.
	free_fs_struct(no_free_ptr(new_fs));
>+		}
>+
> 		if (new_nsproxy) {
> 			switch_task_namespaces(current, new_nsproxy);
> 			new_nsproxy = NULL;
> 		}
> 
>-		if (new_fs)
>-			new_fs = switch_fs_struct(new_fs);
>-

Deserved.

> 		if (new_fd) {
> 			guard(task_lock)(current);
> 			swap(current->files, new_fd);
>
>

--- Thanks!
"I'm not a very positive person" - Linus torvalds

[PATCH 08/10] namespace: drop the file's reference first in dissolve_on_fput()

[Christian Brauner]

A detached mount tree is held alive by a file. dissolve_on_fput()
unmounts the detached tree attached to that file.

Currently dissolve_on_fput() leaves the file's reference to the root of
the tree to the caller. So either __fput(), open_detached_copy(), or
fsmount() drop the mount.

That's backwards. The reference keeps the tree around and is dropped
when the tree is already gone. Instead, let dissolve_on_fput() the mount
and drop it after umount_tree() with namespace_sem still held. After
umount_tree() the mntput() is never the last reference and so the root
gets unmounted before its children.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/file_table.c | 3 ++-
 fs/namespace.c  | 9 ++++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index c68b8c0a4097..e2665eaa6f8c 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -520,7 +520,8 @@ static void __fput(struct file *file)
 	dput(dentry);
 	if (unlikely(mode & FMODE_NEED_UNMOUNT))
 		dissolve_on_fput(mnt);
-	mntput(mnt);
+	else
+		mntput(mnt);
 out:
 	file_free(file);
 }
diff --git a/fs/namespace.c b/fs/namespace.c
index de28fe9058ab..5b44eee28f39 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2312,9 +2312,11 @@ void drop_collected_paths(const struct path *paths, const struct path *prealloc)
 
 static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *, bool);
 
+/* Consumes the caller's reference to @mnt. */
 void dissolve_on_fput(struct vfsmount *mnt)
 {
-	struct mount *m = real_mount(mnt);
+	struct vfsmount *p __free(mntput) = mnt;
+	struct mount *m = real_mount(p);
 
 	/*
 	 * m used to be the root of anon namespace; if it still is one,
@@ -2339,6 +2341,7 @@ void dissolve_on_fput(struct vfsmount *mnt)
 		lock_mount_hash();
 		umount_tree(m, UMOUNT_CONNECTED);
 		unlock_mount_hash();
+		mntput(no_free_ptr(p));
 	}
 }
 
@@ -3109,7 +3112,7 @@ static struct file *open_detached_copy(struct path *path, unsigned int flags)
 	path->mnt = mntget(&ns->root->mnt);
 	file = dentry_open(path, O_PATH, current_cred());
 	if (IS_ERR(file))
-		dissolve_on_fput(path->mnt);
+		dissolve_on_fput(no_free_ptr(path->mnt));
 	else
 		file->f_mode |= FMODE_NEED_UNMOUNT;
 	return file;
@@ -4566,7 +4569,7 @@ SYSCALL_DEFINE3(fsmount, int, fs_fd, unsigned int, flags,
 	FD_PREPARE(fdf, (flags & FSMOUNT_CLOEXEC) ? O_CLOEXEC : 0,
 		   dentry_open(&new_path, O_PATH, fc->cred));
 	if (fdf.err) {
-		dissolve_on_fput(new_path.mnt);
+		dissolve_on_fput(no_free_ptr(new_path.mnt));
 		return fdf.err;
 	}
 

-- 
2.53.0

[PATCH 09/10] selftests/filesystems: check that an unmounted mount tree isn't walked

[Christian Brauner]

Add unmounted_tree_test:

- a detached nsfs or pidfs bind mount that a file descriptor keeps
  alive can't be copied recursively, a copy of the mount itself and
  a copy of a mounted one still work
- a detached tree with a child that was vacated behind it can't be
  copied recursively either
- a copy of a detached bind mount is an ordinary mount: unlinking its
  mountpoint from another mount namespace unmounts it
- mount_setattr() refuses to change the propagation of a detached
  tree and still allows it for the root of an anonymous mount
  namespace

Every case runs in a user and mount namespace of its own.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 tools/testing/selftests/Makefile                   |   1 +
 .../selftests/filesystems/mount_cycle/.gitignore   |   2 +
 .../selftests/filesystems/mount_cycle/Makefile     |   6 +
 .../filesystems/mount_cycle/unmounted_tree_test.c  | 484 +++++++++++++++++++++
 4 files changed, 493 insertions(+)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 43d4a33afe71..bc4fdb6f5337 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -43,6 +43,7 @@ TARGETS += filesystems/open_tree_ns
 TARGETS += filesystems/overlayfs
 TARGETS += filesystems/statmount
 TARGETS += filesystems/mount-notify
+TARGETS += filesystems/mount_cycle
 TARGETS += filesystems/nsfs
 TARGETS += filesystems/fuse
 TARGETS += filesystems/move_mount
diff --git a/tools/testing/selftests/filesystems/mount_cycle/.gitignore b/tools/testing/selftests/filesystems/mount_cycle/.gitignore
new file mode 100644
index 000000000000..7bdfe4268fb4
--- /dev/null
+++ b/tools/testing/selftests/filesystems/mount_cycle/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+unmounted_tree_test
diff --git a/tools/testing/selftests/filesystems/mount_cycle/Makefile b/tools/testing/selftests/filesystems/mount_cycle/Makefile
new file mode 100644
index 000000000000..46a68ca234ba
--- /dev/null
+++ b/tools/testing/selftests/filesystems/mount_cycle/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+TEST_GEN_PROGS := unmounted_tree_test
+
+CFLAGS += -Wall -O2 -g $(KHDR_INCLUDES)
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/filesystems/mount_cycle/unmounted_tree_test.c b/tools/testing/selftests/filesystems/mount_cycle/unmounted_tree_test.c
new file mode 100644
index 000000000000..233f8ea92efc
--- /dev/null
+++ b/tools/testing/selftests/filesystems/mount_cycle/unmounted_tree_test.c
@@ -0,0 +1,484 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * An unmounted mount tree that a file descriptor keeps alive is put and
+ * vacated behind the root's back, so nothing may walk it under
+ * namespace_sem: it can't be copied recursively and its propagation can't
+ * be changed. The mount at its root alone can still be copied and that copy
+ * is an ordinary mount.
+ */
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <linux/mount.h>
+
+#include "../wrappers.h"
+#include "../../kselftest_harness.h"
+
+#ifndef __NR_mount_setattr
+#define __NR_mount_setattr 442
+#endif
+
+#ifndef __NR_pidfd_open
+#define __NR_pidfd_open 434
+#endif
+
+#define PATH_LEN	64
+
+static inline int sys_mount_setattr(int dfd, const char *path, unsigned int flags,
+				    struct mount_attr *attr, size_t size)
+{
+	return syscall(__NR_mount_setattr, dfd, path, flags, attr, size);
+}
+
+static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
+{
+	return syscall(__NR_pidfd_open, pid, flags);
+}
+
+static int write_file(const char *path, const char *s)
+{
+	ssize_t n = -1;
+	int fd;
+
+	fd = open(path, O_WRONLY | O_CLOEXEC);
+	if (fd >= 0) {
+		n = write(fd, s, strlen(s));
+		close(fd);
+	}
+	return n == (ssize_t)strlen(s) ? 0 : -1;
+}
+
+static int touch(const char *path)
+{
+	int fd;
+
+	fd = open(path, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0644);
+	if (fd < 0)
+		return -1;
+	close(fd);
+	return 0;
+}
+
+/* Become root in a new user namespace with a private mount namespace. */
+static int enter_userns(void)
+{
+	uid_t uid = getuid();
+	gid_t gid = getgid();
+	char map[32];
+
+	if (unshare(CLONE_NEWUSER | CLONE_NEWNS))
+		return -1;
+	if (write_file("/proc/self/setgroups", "deny") && errno != ENOENT)
+		return -1;
+	snprintf(map, sizeof(map), "0 %d 1", uid);
+	if (write_file("/proc/self/uid_map", map))
+		return -1;
+	snprintf(map, sizeof(map), "0 %d 1", gid);
+	if (write_file("/proc/self/gid_map", map))
+		return -1;
+	if (setgid(0) || setuid(0))
+		return -1;
+	return mount("", "/", NULL, MS_REC | MS_PRIVATE, NULL);
+}
+
+/* A private mount namespace of our own, all mounts private. */
+static int own_mntns(void)
+{
+	if (unshare(CLONE_NEWNS))
+		return -1;
+	return mount("", "/", NULL, MS_REC | MS_PRIVATE, NULL);
+}
+
+FIXTURE(unmounted_tree) {
+	char dir[PATH_LEN];
+};
+
+FIXTURE_SETUP(unmounted_tree)
+{
+	snprintf(self->dir, sizeof(self->dir), "/tmp/unmounted_tree.XXXXXX");
+	ASSERT_NE(mkdtemp(self->dir), NULL);
+	if (enter_userns()) {
+		rmdir(self->dir);
+		SKIP(return, "test requires user namespaces");
+	}
+	ASSERT_EQ(mount("tmpfs", self->dir, "tmpfs", 0, NULL), 0);
+}
+
+FIXTURE_TEARDOWN(unmounted_tree)
+{
+	umount2(self->dir, MNT_DETACH);
+	rmdir(self->dir);
+}
+
+/* A recursive copy of the mount @fd refers to must fail with EINVAL. */
+static void assert_not_walked(struct __test_metadata *_metadata, int fd,
+			      const char *target)
+{
+	char link[PATH_LEN];
+	int tfd;
+
+	snprintf(link, sizeof(link), "/proc/self/fd/%d", fd);
+	EXPECT_EQ(mount(link, target, NULL, MS_BIND | MS_REC, NULL), -1);
+	EXPECT_EQ(errno, EINVAL);
+
+	tfd = sys_open_tree(fd, "", AT_EMPTY_PATH | AT_RECURSIVE | OPEN_TREE_CLONE |
+			    OPEN_TREE_CLOEXEC);
+	EXPECT_LT(tfd, 0);
+	EXPECT_EQ(errno, EINVAL);
+	if (tfd >= 0)
+		close(tfd);
+}
+
+/* The mount @fd refers to can be copied on its own and mounted on @target. */
+static void assert_root_copied(struct __test_metadata *_metadata, int fd,
+			       const char *target)
+{
+	int tfd;
+
+	tfd = sys_open_tree(fd, "", AT_EMPTY_PATH | OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC);
+	ASSERT_GE(tfd, 0);
+	ASSERT_EQ(sys_move_mount(tfd, "", AT_FDCWD, target, MOVE_MOUNT_F_EMPTY_PATH), 0);
+	close(tfd);
+	ASSERT_EQ(umount2(target, MNT_DETACH), 0);
+}
+
+/*
+ * A lazily unmounted bind mount of an nsfs file, kept alive by an fd, can't
+ * be copied recursively anymore. Copying the mount itself still works and
+ * so does copying a mounted bind mount of the same file.
+ */
+TEST_F(unmounted_tree, detached_nsfs_bind_not_walked)
+{
+	char x[PATH_LEN], y[PATH_LEN], z[PATH_LEN], link[PATH_LEN];
+	int fd, tfd;
+
+	snprintf(x, sizeof(x), "%s/x", self->dir);
+	snprintf(y, sizeof(y), "%s/y", self->dir);
+	snprintf(z, sizeof(z), "%s/z", self->dir);
+	ASSERT_EQ(touch(x), 0);
+	ASSERT_EQ(touch(y), 0);
+	ASSERT_EQ(touch(z), 0);
+
+	ASSERT_EQ(mount("/proc/self/ns/net", x, NULL, MS_BIND, NULL), 0);
+	fd = open(x, O_PATH | O_CLOEXEC);
+	ASSERT_GE(fd, 0);
+	ASSERT_EQ(umount2(x, MNT_DETACH), 0);
+
+	assert_not_walked(_metadata, fd, y);
+
+	snprintf(link, sizeof(link), "/proc/self/fd/%d", fd);
+	ASSERT_EQ(mount(link, y, NULL, MS_BIND, NULL), 0);
+	ASSERT_EQ(umount2(y, MNT_DETACH), 0);
+	assert_root_copied(_metadata, fd, y);
+	/* opening it is fine too */
+	tfd = sys_open_tree(fd, "", AT_EMPTY_PATH | OPEN_TREE_CLOEXEC);
+	ASSERT_GE(tfd, 0);
+	close(tfd);
+	close(fd);
+
+	ASSERT_EQ(mount("/proc/self/ns/net", y, NULL, MS_BIND, NULL), 0);
+	ASSERT_EQ(mount(y, z, NULL, MS_BIND | MS_REC, NULL), 0);
+	tfd = sys_open_tree(AT_FDCWD, y, AT_RECURSIVE | OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC);
+	ASSERT_GE(tfd, 0);
+	close(tfd);
+}
+
+/* Bind-mount the pidfd @pidfd on @target the way pidfd_bind_mount does. */
+static int bind_pidfd(int pidfd, const char *target)
+{
+	int tfd, ret;
+
+	tfd = sys_open_tree(pidfd, "", AT_EMPTY_PATH | OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC);
+	if (tfd < 0)
+		return -1;
+	ret = sys_move_mount(tfd, "", AT_FDCWD, target, MOVE_MOUNT_F_EMPTY_PATH);
+	close(tfd);
+	return ret;
+}
+
+/* The same for a bind mount of a pidfd. */
+TEST_F(unmounted_tree, detached_pidfs_bind_not_walked)
+{
+	char x[PATH_LEN], y[PATH_LEN];
+	int pidfd, fd, tfd;
+
+	snprintf(x, sizeof(x), "%s/x", self->dir);
+	snprintf(y, sizeof(y), "%s/y", self->dir);
+	ASSERT_EQ(touch(x), 0);
+	ASSERT_EQ(touch(y), 0);
+
+	pidfd = sys_pidfd_open(getpid(), 0);
+	ASSERT_GE(pidfd, 0);
+	ASSERT_EQ(bind_pidfd(pidfd, x), 0);
+	fd = open(x, O_PATH | O_CLOEXEC);
+	ASSERT_GE(fd, 0);
+	ASSERT_EQ(umount2(x, MNT_DETACH), 0);
+
+	assert_not_walked(_metadata, fd, y);
+	assert_root_copied(_metadata, fd, y);
+	close(fd);
+
+	ASSERT_EQ(bind_pidfd(pidfd, y), 0);
+	tfd = sys_open_tree(AT_FDCWD, y, AT_RECURSIVE | OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC);
+	ASSERT_GE(tfd, 0);
+	close(tfd);
+	close(pidfd);
+}
+
+/* What a child in its own mount namespace reports back. */
+enum {
+	CHILD_OK,
+	CHILD_NS,		/* could not set up the mount namespace */
+	CHILD_MOUNT,		/* could not set up the mounts */
+	CHILD_PIPE,		/* the parent went away */
+	CHILD_WALKED,		/* the detached tree was copied recursively */
+	CHILD_ROOT_REFUSED,	/* the mount at its root wasn't copied */
+	CHILD_STILL_MOUNTED,	/* the copy survived the unlink of its mountpoint */
+	CHILD_ERRNO,		/* refused, but not with EINVAL */
+};
+
+static const char *child_reason(int code)
+{
+	static const char *const reasons[] = {
+		[CHILD_OK]		= "ok",
+		[CHILD_NS]		= "could not set up the mount namespace",
+		[CHILD_MOUNT]		= "could not set up the mounts",
+		[CHILD_PIPE]		= "the parent went away",
+		[CHILD_WALKED]		= "the detached tree was copied recursively",
+		[CHILD_ROOT_REFUSED]	= "the mount at its root wasn't copied",
+		[CHILD_STILL_MOUNTED]	= "the copy survived the unlink of its mountpoint",
+		[CHILD_ERRNO]		= "refused, but not with EINVAL",
+	};
+
+	if (code < 0 || code >= (int)(sizeof(reasons) / sizeof(reasons[0])))
+		return "child died";
+	return reasons[code];
+}
+
+struct child_args {
+	const char *x;		/* the nsfs bind mount goes here */
+	const char *s;		/* a file to bind on top of it */
+	const char *y;		/* a target to copy to */
+};
+
+/*
+ * Run @fn in a child in a mount namespace of its own. Once the child
+ * reports that its mounts are in place, unlink @victim here, where it is a
+ * plain file, and let the child carry on. Returns what the child reported.
+ */
+static int run_child(struct __test_metadata *_metadata,
+		     int (*fn)(const struct child_args *, int, int),
+		     const struct child_args *a, const char *victim)
+{
+	int to_parent[2], to_child[2], status;
+	pid_t pid;
+	char c;
+
+	if (pipe(to_parent) || pipe(to_child))
+		return -1;
+	pid = fork();
+	if (pid < 0)
+		return -1;
+	if (pid == 0) {
+		close(to_parent[0]);
+		close(to_child[1]);
+		_exit(fn(a, to_parent[1], to_child[0]));
+	}
+	close(to_parent[1]);
+	close(to_child[0]);
+	if (read(to_parent[0], &c, 1) == 1) {
+		/* not a mountpoint in this namespace, so the file can go */
+		EXPECT_EQ(unlink(victim), 0);
+		EXPECT_EQ(write(to_child[1], "", 1), 1);
+	}
+	close(to_parent[0]);
+	close(to_child[1]);
+	if (waitpid(pid, &status, 0) != pid || !WIFEXITED(status))
+		return -1;
+	return WEXITSTATUS(status);
+}
+
+/*
+ * An nsfs bind mount on @x, an fd on it and a bind mount of @s on top. Once
+ * the parent has unlinked @x underneath, the first mount is detached and the
+ * second one, which stayed attached to it, has been vacated. The tree may
+ * not be walked, the mount at its root may still be copied.
+ */
+static int vacant_child(const struct child_args *a, int to_parent, int from_parent)
+{
+	char link[PATH_LEN];
+	int fd, tfd;
+	char c;
+
+	if (own_mntns())
+		return CHILD_NS;
+	if (mount("/proc/self/ns/net", a->x, NULL, MS_BIND, NULL))
+		return CHILD_MOUNT;
+	fd = open(a->x, O_PATH | O_CLOEXEC);
+	if (fd < 0 || mount(a->s, a->x, NULL, MS_BIND, NULL))
+		return CHILD_MOUNT;
+
+	if (write(to_parent, "", 1) != 1 || read(from_parent, &c, 1) != 1)
+		return CHILD_PIPE;
+
+	tfd = sys_open_tree(fd, "", AT_EMPTY_PATH | AT_RECURSIVE | OPEN_TREE_CLONE |
+			    OPEN_TREE_CLOEXEC);
+	if (tfd >= 0) {
+		close(tfd);
+		return CHILD_WALKED;
+	}
+	if (errno != EINVAL)
+		return CHILD_ERRNO;
+	snprintf(link, sizeof(link), "/proc/self/fd/%d", fd);
+	if (!mount(link, a->y, NULL, MS_BIND | MS_REC, NULL))
+		return CHILD_WALKED;
+	if (errno != EINVAL)
+		return CHILD_ERRNO;
+
+	tfd = sys_open_tree(fd, "", AT_EMPTY_PATH | OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC);
+	if (tfd < 0)
+		return CHILD_ROOT_REFUSED;
+	close(tfd);
+	/* the last reference on the detached mount collects the vacant one */
+	close(fd);
+	return CHILD_OK;
+}
+
+/*
+ * unlink() of the child's mountpoint from here, where it is a plain file,
+ * detaches the child's nsfs bind mount with the mount on top left attached
+ * to it. The mount on top loses its last reference right there and is
+ * vacated. The child must not be able to walk that tree.
+ */
+TEST_F(unmounted_tree, vacant_child_not_copied)
+{
+	char x[PATH_LEN], s[PATH_LEN], y[PATH_LEN];
+	struct child_args a = { .x = x, .s = s, .y = y };
+	int ret;
+
+	snprintf(x, sizeof(x), "%s/x", self->dir);
+	snprintf(s, sizeof(s), "%s/s", self->dir);
+	snprintf(y, sizeof(y), "%s/y", self->dir);
+	ASSERT_EQ(touch(x), 0);
+	ASSERT_EQ(touch(s), 0);
+	ASSERT_EQ(touch(y), 0);
+
+	ret = run_child(_metadata, vacant_child, &a, x);
+	ASSERT_EQ(ret, CHILD_OK)
+		TH_LOG("child: %s", child_reason(ret));
+}
+
+/*
+ * An nsfs bind mount on @x, lazily unmounted but kept alive by an fd, and a
+ * copy of it on @y. That copy is a mount like any other: once the parent has
+ * unlinked @y underneath, it is unmounted and its attributes can't be changed
+ * anymore. A copy that took the flag along is only unhooked, stays in the
+ * namespace and still takes the change.
+ */
+static int ordinary_copy(const struct child_args *a, int to_parent, int from_parent)
+{
+	struct mount_attr attr = {
+		.attr_set = MOUNT_ATTR_NOSUID,
+	};
+	char link[PATH_LEN];
+	int fd, fdy;
+	char c;
+
+	if (own_mntns())
+		return CHILD_NS;
+	if (mount("/proc/self/ns/net", a->x, NULL, MS_BIND, NULL))
+		return CHILD_MOUNT;
+	fd = open(a->x, O_PATH | O_CLOEXEC);
+	if (fd < 0 || umount2(a->x, MNT_DETACH))
+		return CHILD_MOUNT;
+	snprintf(link, sizeof(link), "/proc/self/fd/%d", fd);
+	if (mount(link, a->y, NULL, MS_BIND, NULL))
+		return CHILD_MOUNT;
+	fdy = open(a->y, O_PATH | O_CLOEXEC);
+	if (fdy < 0)
+		return CHILD_MOUNT;
+
+	if (write(to_parent, "", 1) != 1 || read(from_parent, &c, 1) != 1)
+		return CHILD_PIPE;
+
+	if (!sys_mount_setattr(fdy, "", AT_EMPTY_PATH, &attr, sizeof(attr)))
+		return CHILD_STILL_MOUNTED;
+	if (errno != EINVAL)
+		return CHILD_ERRNO;
+	close(fdy);
+	close(fd);
+	return CHILD_OK;
+}
+
+/*
+ * A copy of a detached nsfs bind mount must not take after its source and
+ * read as unmounted. unlink() of its mountpoint from here unmounts it like
+ * any other mount, so the child can't change its attributes anymore.
+ */
+TEST_F(unmounted_tree, copy_of_detached_bind_is_ordinary)
+{
+	char x[PATH_LEN], y[PATH_LEN];
+	struct child_args a = { .x = x, .y = y };
+	int ret;
+
+	snprintf(x, sizeof(x), "%s/x", self->dir);
+	snprintf(y, sizeof(y), "%s/y", self->dir);
+	ASSERT_EQ(touch(x), 0);
+	ASSERT_EQ(touch(y), 0);
+
+	ret = run_child(_metadata, ordinary_copy, &a, y);
+	ASSERT_EQ(ret, CHILD_OK)
+		TH_LOG("child: %s", child_reason(ret));
+}
+
+/*
+ * mount_setattr() may change the propagation of a detached tree while that
+ * tree is still the root of an anonymous mount namespace, but not once the
+ * tree has been dissolved and only a second fd keeps its root alive.
+ */
+TEST_F(unmounted_tree, detached_tree_setattr_refused)
+{
+	struct mount_attr attr = {
+		.propagation = MS_SHARED,
+	};
+	char a[PATH_LEN], b[PATH_LEN];
+	int tfd, fd;
+
+	snprintf(a, sizeof(a), "%s/a", self->dir);
+	snprintf(b, sizeof(b), "%s/a/b", self->dir);
+	ASSERT_EQ(mkdir(a, 0755), 0);
+	ASSERT_EQ(mount("tmpfs", a, "tmpfs", 0, NULL), 0);
+	ASSERT_EQ(mkdir(b, 0755), 0);
+	ASSERT_EQ(mount("tmpfs", b, "tmpfs", 0, NULL), 0);
+
+	tfd = sys_open_tree(AT_FDCWD, self->dir, AT_RECURSIVE | OPEN_TREE_CLONE |
+			    OPEN_TREE_CLOEXEC);
+	ASSERT_GE(tfd, 0);
+	ASSERT_EQ(sys_mount_setattr(tfd, "", AT_EMPTY_PATH | AT_RECURSIVE, &attr, sizeof(attr)), 0);
+	attr.propagation = MS_PRIVATE;
+	ASSERT_EQ(sys_mount_setattr(tfd, "", AT_EMPTY_PATH | AT_RECURSIVE, &attr, sizeof(attr)), 0);
+
+	/* a second reference on the root, then dissolve the tree */
+	fd = sys_open_tree(tfd, "", AT_EMPTY_PATH | OPEN_TREE_CLOEXEC);
+	ASSERT_GE(fd, 0);
+	close(tfd);
+
+	attr.propagation = MS_SHARED;
+	ASSERT_EQ(sys_mount_setattr(fd, "", AT_EMPTY_PATH | AT_RECURSIVE, &attr, sizeof(attr)), -1);
+	ASSERT_EQ(errno, EINVAL);
+	attr.propagation = MS_PRIVATE;
+	ASSERT_EQ(sys_mount_setattr(fd, "", AT_EMPTY_PATH, &attr, sizeof(attr)), -1);
+	ASSERT_EQ(errno, EINVAL);
+	close(fd);
+}
+
+TEST_HARNESS_MAIN

-- 
2.53.0

[PATCH 10/10] selftests/filesystems: check that a dead mount's overmount isn't followed

[Christian Brauner]

Add overmount_reparent_test:

- a mount that a file descriptor keeps alive after propagate_umount()
  reparented its overmount and unmounted it must not be walked into the
  freed overmount by move_mount(MOVE_MOUNT_BENEATH)

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 .../selftests/filesystems/mount_cycle/.gitignore   |   1 +
 .../selftests/filesystems/mount_cycle/Makefile     |   2 +-
 .../mount_cycle/overmount_reparent_test.c          | 181 +++++++++++++++++++++
 3 files changed, 183 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/filesystems/mount_cycle/.gitignore b/tools/testing/selftests/filesystems/mount_cycle/.gitignore
index 7bdfe4268fb4..03ca95de7765 100644
--- a/tools/testing/selftests/filesystems/mount_cycle/.gitignore
+++ b/tools/testing/selftests/filesystems/mount_cycle/.gitignore
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 unmounted_tree_test
+overmount_reparent_test
diff --git a/tools/testing/selftests/filesystems/mount_cycle/Makefile b/tools/testing/selftests/filesystems/mount_cycle/Makefile
index 46a68ca234ba..88271c82d1c1 100644
--- a/tools/testing/selftests/filesystems/mount_cycle/Makefile
+++ b/tools/testing/selftests/filesystems/mount_cycle/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-TEST_GEN_PROGS := unmounted_tree_test
+TEST_GEN_PROGS := unmounted_tree_test overmount_reparent_test
 
 CFLAGS += -Wall -O2 -g $(KHDR_INCLUDES)
 
diff --git a/tools/testing/selftests/filesystems/mount_cycle/overmount_reparent_test.c b/tools/testing/selftests/filesystems/mount_cycle/overmount_reparent_test.c
new file mode 100644
index 000000000000..46a56b722f3e
--- /dev/null
+++ b/tools/testing/selftests/filesystems/mount_cycle/overmount_reparent_test.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * propagate_umount() slides a surviving overmount off a mount that is being
+ * unmounted and unmounts that mount right after. If a file descriptor keeps
+ * the unmounted mount alive its ->overmount is left pointing at the moved
+ * mount, which is freed on its own schedule. move_mount(MOVE_MOUNT_BENEATH)
+ * with such a file descriptor as the target walks ->overmount in
+ * topmost_overmount() before it checks that the target is still mounted and
+ * must not step into the freed mount.
+ */
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+
+#include "../wrappers.h"
+#include "../../kselftest_harness.h"
+
+#ifndef MOVE_MOUNT_BENEATH
+#define MOVE_MOUNT_BENEATH 0x00000200
+#endif
+
+#ifndef FSCONFIG_CMD_CREATE
+#define FSCONFIG_CMD_CREATE 6
+#endif
+
+#define DIR_LEN 64
+#define PATH_LEN 128
+
+static int write_file(const char *path, const char *s)
+{
+	ssize_t n = -1;
+	int fd;
+
+	fd = open(path, O_WRONLY | O_CLOEXEC);
+	if (fd >= 0) {
+		n = write(fd, s, strlen(s));
+		close(fd);
+	}
+	return n == (ssize_t)strlen(s) ? 0 : -1;
+}
+
+/* Become root in a new user namespace with a private mount namespace. */
+static int enter_userns(void)
+{
+	uid_t uid = getuid();
+	gid_t gid = getgid();
+	char map[32];
+
+	if (unshare(CLONE_NEWUSER | CLONE_NEWNS))
+		return -1;
+	if (write_file("/proc/self/setgroups", "deny") && errno != ENOENT)
+		return -1;
+	snprintf(map, sizeof(map), "0 %d 1", uid);
+	if (write_file("/proc/self/uid_map", map))
+		return -1;
+	snprintf(map, sizeof(map), "0 %d 1", gid);
+	if (write_file("/proc/self/gid_map", map))
+		return -1;
+	if (setgid(0) || setuid(0))
+		return -1;
+	return mount("", "/", NULL, MS_REC | MS_PRIVATE, NULL);
+}
+
+/* A detached tmpfs mount to serve as the source of a move. */
+static int detached_tmpfs(void)
+{
+	int sfd, mfd;
+
+	sfd = sys_fsopen("tmpfs", 0);
+	if (sfd < 0)
+		return -1;
+	if (sys_fsconfig(sfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0)) {
+		close(sfd);
+		return -1;
+	}
+	mfd = sys_fsmount(sfd, 0, 0);
+	close(sfd);
+	return mfd;
+}
+
+FIXTURE(overmount_reparent) {
+	char dir[DIR_LEN];
+};
+
+FIXTURE_SETUP(overmount_reparent)
+{
+	snprintf(self->dir, sizeof(self->dir), "/tmp/overmount_reparent.XXXXXX");
+	ASSERT_NE(mkdtemp(self->dir), NULL);
+	if (enter_userns()) {
+		rmdir(self->dir);
+		SKIP(return, "test requires user namespaces");
+	}
+}
+
+FIXTURE_TEARDOWN(overmount_reparent)
+{
+	char p[PATH_LEN];
+
+	snprintf(p, sizeof(p), "%s/b", self->dir);
+	umount2(p, MNT_DETACH);
+	snprintf(p, sizeof(p), "%s/a", self->dir);
+	umount2(p, MNT_DETACH);
+}
+
+/*
+ * A shared mount base_a is bind-mounted to base_b as its peer. A mount X on
+ * base_a/mp propagates a copy X_b onto base_b/mp. A file descriptor pins X_b,
+ * then X_b is made private and an overmount is stacked on its root.
+ *
+ * A lazy unmount of X propagates to X_b: X_b is committed to the unmount while
+ * its overmount survives, so propagate_umount() reparents the overmount onto
+ * base_b and unmounts X_b, which the file descriptor keeps alive. Unmounting
+ * the reparented overmount frees it. X_b->overmount now dangles unless
+ * mnt_change_mountpoint() reset it.
+ *
+ * move_mount(MOVE_MOUNT_BENEATH) through the file descriptor makes
+ * do_lock_mount() follow X_b->overmount in topmost_overmount() before it
+ * notices that X_b is no longer mounted. With the pointer reset the move fails
+ * cleanly with ENOENT; otherwise it reads the freed overmount.
+ */
+TEST_F(overmount_reparent, dead_overmount_holder_not_followed)
+{
+	char a[PATH_LEN], b[PATH_LEN], amp[PATH_LEN], bmp[PATH_LEN];
+	int fd, mfd, ret;
+
+	snprintf(a, sizeof(a), "%s/a", self->dir);
+	snprintf(b, sizeof(b), "%s/b", self->dir);
+	ASSERT_EQ(mkdir(a, 0755), 0);
+	ASSERT_EQ(mkdir(b, 0755), 0);
+
+	ASSERT_EQ(mount("tmpfs", a, "tmpfs", 0, NULL), 0);
+	ASSERT_EQ(mount(NULL, a, NULL, MS_SHARED, NULL), 0);
+	ASSERT_EQ(mount(a, b, NULL, MS_BIND, NULL), 0);
+
+	snprintf(amp, sizeof(amp), "%s/a/mp", self->dir);
+	snprintf(bmp, sizeof(bmp), "%s/b/mp", self->dir);
+	/* base_a and base_b share one superblock, so this dir is in both. */
+	ASSERT_EQ(mkdir(amp, 0755), 0);
+
+	/* X on base_a/mp propagates a copy X_b onto base_b/mp. */
+	ASSERT_EQ(mount("tmpfs", amp, "tmpfs", 0, NULL), 0);
+
+	/* Pin X_b before anything is stacked on top of it. */
+	fd = open(bmp, O_PATH | O_CLOEXEC);
+	ASSERT_GE(fd, 0);
+
+	/* Keep the overmount local to X_b. */
+	ASSERT_EQ(mount(NULL, bmp, NULL, MS_PRIVATE, NULL), 0);
+
+	/* The overmount on X_b's root: X_b->overmount points at it. */
+	ASSERT_EQ(mount("tmpfs", bmp, "tmpfs", 0, NULL), 0);
+
+	/* Reparents the overmount onto base_b and unmounts X_b. */
+	ASSERT_EQ(umount2(amp, MNT_DETACH), 0);
+
+	/* Free the reparented overmount. */
+	ASSERT_EQ(umount2(bmp, MNT_DETACH), 0);
+	usleep(100000);
+
+	mfd = detached_tmpfs();
+	ASSERT_GE(mfd, 0);
+
+	ret = sys_move_mount(mfd, "", fd, "",
+			     MOVE_MOUNT_F_EMPTY_PATH | MOVE_MOUNT_T_EMPTY_PATH |
+			     MOVE_MOUNT_BENEATH);
+	EXPECT_EQ(ret, -1);
+	EXPECT_EQ(errno, ENOENT);
+
+	close(mfd);
+	close(fd);
+}
+
+TEST_HARNESS_MAIN

-- 
2.53.0