[PATCH 0/3] dcache: fixes for mountpoints racing d_invalidate() and umount

[PATCH 0/3] dcache: fixes for mountpoints racing d_invalidate() and umount

[Christian Brauner]

d_set_mounted() hasn't bumped the seqcount of rename_lock since v6.17
and the lockless walk in d_invalidate() neither waits for nor retries
against a concurrent mount anymore.

So a mount below a directory that is being invalidated can end up below
the unhashed directory. Nobody can reach it anymore and it pins the
subtree until the mount namespace goes away. Giving d_set_mounted() a
seqlock of its own that d_invalidate() waits on after its unhash closes
that without going back to bumping the seqcount.

The second one is from v7.3 and was backported to 7.2.6. do_one_tree()
now takes every secondary root off ->s_roots before its dput(). If
another thread is evicting the root at the same time umount loses the
eviction to it and doesn't wait for it. That ends in busy inodes after
unmount and an iput() on a dead superblock.

The third one is old. With CAP_DAC_READ_SEARCH a disconnected dentry
can be mounted on via open_by_handle_at() and move_mount(). When that
mount goes away while the filesystem of the dentry is unmounted, umount
never sees the dentry. Same result as the second one. The fix makes such
a dentry a secondary root until it's evicted. So it needs the second
patch and it only goes back to v7.2.

I reproduced all three in a VM. With a FUSE directory 3000 levels deep
151 of 200 bind mounts racing FUSE_NOTIFY_INVAL_ENTRY ended up below the
invalidated directory and none did with the fix. The other two need
delays in the right places to trigger. With them both reliably end in
busy inodes and a GPF in iput() and with the fixes they don't.

This goes into vfs.fixes.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Christian Brauner (1):
      dcache: let d_invalidate() wait for a d_set_mounted() in flight

Josef Bacik (2):
      dcache: wait for a racing eviction of a secondary root
      dcache: let umount find a disconnected ex-mountpoint

 fs/dcache.c                    | 44 ++++++++++++++++++++++++++++++++++--------
 include/linux/fs/super_types.h |  2 +-
 2 files changed, 37 insertions(+), 9 deletions(-)
---
base-commit: 172658cf600904ff6121e240f69a0ccb36e65d6b
change-id: 20261029-work-dcache-ai-6e7ad5e247ce

[PATCH 1/3] dcache: let d_invalidate() wait for a d_set_mounted() in flight

[Christian Brauner]

d_invalidate() and d_set_mounted() are supposed to exclude each other.
Either the mount fails with -ENOENT because an ancestor of the
mountpoint was unhashed or d_invalidate() finds the mountpoint and
detaches the mount. d_invalidate() unhashes the dentry first and then
searches the subtree with d_walk() which starts out lockless and only
looks at the seqcount of rename_lock.

Since d_set_mounted() switched to read_seqlock_excl() nothing makes that
walk wait or retry:

  CPU0: mount on A/M                 CPU1: d_invalidate(A)
  d_set_mounted(M)
    read_seqlock_excl(&rename_lock)
    A is hashed, go on
                                     __d_drop(A)
                                     shrink_dcache_parent(A)
                                     d_walk(A, find_submount)
                                       read_seqbegin(&rename_lock)
                                       M is not a mountpoint
                                       need_seqretry() == false
                                     return
    M->d_flags |= DCACHE_MOUNTED
    return 0
  mount gets attached to M

The mount ends up below an unhashed dentry. It can't be reached anymore
and pins the subtree until its parent mount or the mount namespace goes
away. The window grows with the depth of the mountpoint. With a FUSE
directory 3000 levels deep a bind mount racing with
FUSE_NOTIFY_INVAL_ENTRY for the parent of the mountpoint survived in 151
of 200 attempts.

Before 5e317d23f6c6 the seqcount bump made the walk wait while
d_set_mounted() ran and retry under the lock when one had run in the
meantime. That was the right thing to wait for on the wrong counter:
every rename_lock reader paid for it, d_lookup(), d_path(), the ".."
step of the RCU walk.

So give d_set_mounted() a seqlock of its own. It is the only writer and
d_invalidate() is the only reader. After unhashing the dentry
d_invalidate() runs an empty read_seqbegin()/read_seqretry() section on
it. That spins while the count is odd, i.e. while a d_set_mounted() is
between its ancestor check and its flag, and goes round once more if
one started in the meantime. Nobody else retries anything. The ordering
comes from the locks both already take:

- If that d_set_mounted() saw the dentry hashed its check ran under the
  dentry's d_lock before __d_drop() took it. So the count is odd by the
  time d_invalidate() reads it and d_invalidate() waits.

- Once the count is even again the flag has been set under the
  mountpoint's d_lock and the walk takes that lock to look at it.

A d_set_mounted() that starts after the unhash fails its ancestor check
and never touches the flag. So the only flag the walk can pick up
belongs to the one mount that saw the dentry hashed and is still in
flight, and there is at most one of those because namespace_sem
serializes mounts. Either it gets attached and detach_mounts() takes it
off after waiting for namespace_sem or it fails later on and drops the
flag again. In that case the walk goes round once more and finds
nothing. The same goes for the mountpoint that attach_recursive_mnt()
preallocates on the root of the tree it attaches. IOW, d_invalidate()
does at most one extra round, which the removed-name trap (cf. [1])
relies on.

Note that on PREEMPT_RT read_seqbegin() takes the lock instead of
spinning on a preempted d_set_mounted().

The cost is a seqlock around the ancestor walk for every new mountpoint
and two loads of a cacheline that only d_set_mounted() writes for every
d_invalidate() of a positive dentry. Walking the ancestors a second time
after setting the flag would close the race as well but doubles the
cost of d_set_mounted() with the depth of the mountpoint: about 1us at
64 levels and 75us at 4096 levels.

[1]: https://patch.msgid.link/20261028-work-mount-trap-b4-v1-0-f20d9d0730d6@kernel.org

Fixes: 5e317d23f6c6 ("d_set_mounted(): we don't need to bump seqcount component of rename_lock")
Reported-by: Josef Bacik <josef@toxicpanda.com>
Cc: stable@vger.kernel.org # v6.17+
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/dcache.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index efd9332c8407..72b8e0adab0d 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1590,21 +1590,25 @@ int path_has_submounts(const struct path *parent)
 }
 EXPORT_SYMBOL(path_has_submounts);
 
+/* odd while d_set_mounted() runs, d_invalidate() waits on it */
+static __cacheline_aligned_in_smp DEFINE_SEQLOCK(d_set_mounted_lock);
+
 /*
  * Called by mount code to set a mountpoint and check if the mountpoint is
  * reachable (e.g. NFS can unhash a directory dentry and then the complete
  * subtree can become unreachable).
  *
  * Only one of d_invalidate() and d_set_mounted() must succeed.  For
- * this reason take rename_lock and d_lock on dentry and ancestors.
- * Likewise for dont_mount() which marks a dentry that is being removed
- * under d_lock.
+ * this reason take rename_lock, d_set_mounted_lock and d_lock on dentry
+ * and ancestors.  Likewise for dont_mount() which marks a dentry that
+ * is being removed under d_lock.
  */
 int d_set_mounted(struct dentry *dentry)
 {
 	struct dentry *p;
 	int ret = -ENOENT;
 	read_seqlock_excl(&rename_lock);
+	write_seqlock(&d_set_mounted_lock);
 	for (p = dentry->d_parent; !IS_ROOT(p); p = p->d_parent) {
 		/* Need exclusion wrt. d_invalidate() */
 		spin_lock(&p->d_lock);
@@ -1622,8 +1626,9 @@ int d_set_mounted(struct dentry *dentry)
 			ret = 0;
 		}
 	}
- 	spin_unlock(&dentry->d_lock);
+	spin_unlock(&dentry->d_lock);
 out:
+	write_sequnlock(&d_set_mounted_lock);
 	read_sequnlock_excl(&rename_lock);
 	return ret;
 }
@@ -1876,6 +1881,7 @@ static enum d_walk_ret find_submount(void *_data, struct dentry *dentry)
 void d_invalidate(struct dentry *dentry)
 {
 	bool had_submounts = false;
+	unsigned seq;
 	spin_lock(&dentry->d_lock);
 	if (d_unhashed(dentry)) {
 		spin_unlock(&dentry->d_lock);
@@ -1888,6 +1894,11 @@ void d_invalidate(struct dentry *dentry)
 	if (!dentry->d_inode)
 		return;
 
+	/* wait for a d_set_mounted() that saw us hashed to set its flag */
+	do {
+		seq = read_seqbegin(&d_set_mounted_lock);
+	} while (read_seqretry(&d_set_mounted_lock, seq));
+
 	shrink_dcache_parent(dentry);
 	for (;;) {
 		struct dentry *victim = NULL;

-- 
2.53.0

[PATCH 2/3] dcache: wait for a racing eviction of a secondary root

[Christian Brauner]

From: Josef Bacik <josef@toxicpanda.com>

Commit e9895609cb7f ("wind ->s_roots via ->d_sib instead of ->d_hash")
keeps a secondary root on ->s_roots until dentry_unlist() so that
shrink_dcache_for_umount() can find one that another thread is evicting
and wait for it. But do_one_tree() takes every root off the list before
its dput() which breaks that:

  thread A                              umount
  dentry_kill(root), refcount is 0
    lock_for_kill()
      spin_trylock(->i_lock) fails
      spin_unlock(->d_lock)
                                        shrink_dcache_for_umount()
                                          dget_dlock(root)
                                          do_one_tree(root)
                                            unlink_secondary_root(root)
      spin_lock(->i_lock)
                                            dput(root), refcount is 0
                                              lock_for_kill()
                                                trylock fails, waits
      spin_lock(->d_lock)
      refcount is 0 again, go on
    lockref_mark_dead()
    dentry_unlink_inode()
      drops both locks
                                                root is dead, give up
                                          ->s_roots is empty, done
                                        evict_inodes()
                                        "Busy inodes after unmount"
      iput()

Thread A evicts without pinning the superblock. namespace_unlock() does
that for old mountpoints after dropping namespace_sem, and a secondary
root goes with its last child.

So only take busy roots off the list. Leave the others to
dentry_unlist() so that shrink_dcache_for_umount() finds them and waits
for an eviction in progress. A root that becomes busy after the check is
picked up again and taken off the list then.

Fixes: 78db93943210 ("dcache: keep shrink_dcache_for_umount() making progress on busy roots")
Cc: stable@vger.kernel.org # v7.2+
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/dcache.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 72b8e0adab0d..c1d8b398b2d1 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1817,8 +1817,13 @@ static void do_one_tree(struct dentry *dentry)
 	d_walk(dentry, dentry, umount_check);
 	spin_lock(&dentry->d_lock);
 	__d_drop(dentry);
-	/* A busy root survives the dput() below so don't leave it on ->s_roots. */
-	if (unlikely(!hlist_unhashed(&dentry->d_sib)))
+	/*
+	 * A busy root survives the dput() below so don't leave it on ->s_roots.
+	 * An idle one stays until dentry_unlist() so that umount can wait for
+	 * a racing eviction, see lock_for_kill().
+	 */
+	if (unlikely(dentry->d_lockref.count > 1 &&
+		     !hlist_unhashed(&dentry->d_sib)))
 		unlink_secondary_root(dentry);
 	spin_unlock(&dentry->d_lock);
 	dput(dentry);

-- 
2.53.0

[PATCH 3/3] dcache: let umount find a disconnected ex-mountpoint

[Christian Brauner]

From: Josef Bacik <josef@toxicpanda.com>

When a mount goes away the reference to its mountpoint is dropped with
dput_to_list() and namespace_unlock() evicts the dentry after dropping
namespace_sem. Nothing pins the filesystem at that point so it can be
unmounted at the same time. That's fine as long as
shrink_dcache_for_umount() finds the dentry. It then either evicts it
itself or waits for the eviction to finish.

But it can't find a disconnected root as those are neither in the tree
nor on ->s_roots. And they can be mounted on:

  fd = open_by_handle_at(mnt_fd, handle of a file with no dentry, O_PATH);
  tree = open_tree(AT_FDCWD, "/tmp/x", OPEN_TREE_CLONE);
  move_mount(tree, "", fd, "", MOVE_MOUNT_F_EMPTY_PATH |
			       MOVE_MOUNT_T_EMPTY_PATH);
  close(fd);

  thread A                              thread B
  umount2("/proc/self/fd/<tree>", MNT_DETACH)
    namespace_unlock()
      up_write(&namespace_sem)
                                        umount of the filesystem
                                          shrink_dcache_for_umount()
                                          evict_inodes()
                                          "Busy inodes after unmount"
      shrink_dentry_list()
        dentry_kill()
          iput() of an inode of a superblock that is gone

So put such a dentry on ->s_roots when it's put on the shrink list. From
then on it's a secondary root like any other.

This needs the previous patch. Otherwise do_one_tree() takes the dentry
off ->s_roots and umount doesn't wait for an eviction in progress.

The stable tag is narrower than the Fixes tag on purpose. Before commit
e9895609cb7f ("wind ->s_roots via ->d_sib instead of ->d_hash") umount
doesn't wait for a secondary root that is being evicted either.

Fixes: 4edbe133f851 ("make struct mountpoint bear the dentry reference to mountpoint, not struct mount")
Cc: stable@vger.kernel.org # v7.2+
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/dcache.c                    | 16 ++++++++++++++--
 include/linux/fs/super_types.h |  2 +-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index c1d8b398b2d1..f902936e38d5 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -45,7 +45,8 @@
  * dcache_hash_bucket lock protects:
  *   - the dcache hash table
  * s_roots_lock protects:
- *   - the s_roots list (see __d_move()/dentry_unlist()/d_obtain_root())
+ *   - the s_roots list (see __d_move()/dentry_unlist()/d_obtain_root()/
+ *     dput_to_list())
  * dentry->d_sb->s_dentry_lru_lock protects:
  *   - the dcache lru lists and counters
  * d_lock protects:
@@ -1102,7 +1103,18 @@ void dput_to_list(struct dentry *dentry, struct list_head *list)
 {
 	if (likely(fast_dput(dentry)))
 		return;
-	__move_to_shrink_list(dentry, list);
+	/*
+	 * The list owner need not pin the superblock, so make a disconnected
+	 * root a secondary one for shrink_dcache_for_umount() to find.
+	 */
+	if (__move_to_shrink_list(dentry, list) &&
+	    unlikely(IS_ROOT(dentry) && hlist_unhashed(&dentry->d_sib))) {
+		struct super_block *sb = dentry->d_sb;
+
+		spin_lock(&sb->s_roots_lock);
+		hlist_add_head(&dentry->d_sib, &sb->s_roots);
+		spin_unlock(&sb->s_roots_lock);
+	}
 	spin_unlock(&dentry->d_lock);
 }
 
diff --git a/include/linux/fs/super_types.h b/include/linux/fs/super_types.h
index 72f4288e16c5..6ce105fe8359 100644
--- a/include/linux/fs/super_types.h
+++ b/include/linux/fs/super_types.h
@@ -173,7 +173,7 @@ struct super_block {
 	struct unicode_map			*s_encoding;
 	__u16					s_encoding_flags;
 #endif
-	struct hlist_head			s_roots;	/* alternate root dentries for NFS */
+	struct hlist_head			s_roots;	/* alternate root dentries for NFS and ex-mountpoints */
 	spinlock_t				s_roots_lock;
 	struct mount				*s_mounts;	/* list of mounts; _not_ for fs use */
 	struct block_device			*s_bdev;	/* can go away once we use an accessor for @s_bdev_file */

-- 
2.53.0