Re: [RFC][PATCH] make sure that lock_for_kill() callers drop the locks in safe order

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2026-04-11 at 22:33 +0100, Al Viro wrote:
> On Fri, Apr 10, 2026 at 09:24:04PM +0100, Al Viro wrote:
> > On Fri, Apr 10, 2026 at 12:30:13PM -0700, Linus Torvalds wrote:
> > 
> > > The reason it exists is because lock_for_kill() can drop d_lock(), but
> > > that's in the unlikely case that we cn't just immediately get the
> > > inode lock.
> > > 
> > > So honestly, I think that rcu_read_lock() should be inside
> > > lock_for_kill(), rather than in the caller as a "just in case things
> > > go down".
> > 
> > Yup, in the cascade of followups I've mentioned...
> 
> FWIW, see #work.dcache-cleanups (on top of #work.dcache-busy-wait).  That's
> obviously next cycle fodder, and it needs review and testing (at the moment
> I've only build-tested that).
> 
> Branch is in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.dcache-cleanups
> individual patches in followups.
> 
> I think the eviction machinery got easier to read, if nothing else, and
> rcu_read_lock() scopes are saner now.  OTOH, if we *do* have RCU bugs wrt
> overlapping scopes, we'll probably see a lot of UAF on that - rcu_read_lock()
> scopes got aggressively minimized...
> 
> I've split the rcu_read_lock() massage into a series of small steps after
> getting confused while trying to do them at once; not sure if it needs
> to be carved up so much, but...
> 
> Shortlog:
> Al Viro (11):
>       shrink_dentry_list(): start with removing from shrink list
>       fold lock_for_kill() into shrink_kill()
>       fold lock_for_kill() and __dentry_kill() into common helper
>       reducing rcu_read_lock() scopes in dput and friends, step 1
>       reducing rcu_read_lock() scopes in dput and friends, step 2
>       reducing rcu_read_lock() scopes in dput and friends, step 3
>       reducing rcu_read_lock() scopes in dput and friends, step 4
>       reducing rcu_read_lock() scopes in dput and friends, step 5
>       reducing rcu_read_lock() scopes in dput and friends, step 6
>       adjust calling conventions of lock_for_kill(), fold __dentry_kill() into dentry_kill()
>       document dentry_kill()
> 
> Diffstat:
>  fs/dcache.c | 214 ++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 129 insertions(+), 85 deletions(-)
> 63 lines of comments added - outside of comments it's -20LoC...

FWIW, I ran fstests + the earlier reproducer on a v6.19-ish kernel
without any of your fixes, and with "slub_debug=P,dentry" on the kernel
command line. It also had the patch below. I got no warnings back:

--------------------------8<--------------------------

diff --git a/fs/Kconfig b/fs/Kconfig
index 0bfdaecaa877..f3a228cd14e8 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -9,6 +9,15 @@ menu "File systems"
 config DCACHE_WORD_ACCESS
        bool
 
+config DCACHE_SHRINK_RACE_DEBUG
+	bool "Debug: inject delay in __dentry_kill to widen race window"
+	depends on DEBUG_KERNEL
+	default n
+	help
+	  Inject a probabilistic delay in __dentry_kill() between releasing
+	  d_lock and re-acquiring it, to make dcache shrink races reproducible
+	  in test environments. Only enable for testing.
+
 config VALIDATE_FS_PARSER
 	bool "Validate filesystem parameter description"
 	help
diff --git a/fs/dcache.c b/fs/dcache.c
index 66dd1bb830d1..b1625f7e30e1 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -32,6 +32,8 @@
 #include <linux/bit_spinlock.h>
 #include <linux/rculist_bl.h>
 #include <linux/list_lru.h>
+#include <linux/delay.h>
+#include <linux/random.h>
 #include "internal.h"
 #include "mount.h"
 
@@ -675,6 +677,16 @@ static struct dentry *__dentry_kill(struct dentry *dentry)
 		dentry->d_op->d_release(dentry);
 
 	cond_resched();
+#ifdef CONFIG_DCACHE_SHRINK_RACE_DEBUG
+	/*
+	 * Probabilistic delay to widen the race window in __dentry_kill()
+	 * between dropping d_lock and re-acquiring it.  30% chance of 5ms
+	 * delay for proc dentries.
+	 */
+	if (dentry->d_sb->s_magic == 0x9fa0 /* PROC_SUPER_MAGIC */ &&
+	    get_random_u32_below(100) < 30)
+		mdelay(5);
+#endif
 	/* now that it's negative, ->d_parent is stable */
 	if (!IS_ROOT(dentry)) {
 		parent = dentry->d_parent;
@@ -2722,6 +2734,9 @@ static wait_queue_head_t *__d_lookup_unhash(struct dentry *dentry)
 
 	lockdep_assert_held(&dentry->d_lock);
 
+	if (WARN_ON(!(dentry->d_flags & DCACHE_PAR_LOOKUP)))
+		return NULL;
+
 	b = in_lookup_hash(dentry->d_parent, dentry->d_name.hash);
 	hlist_bl_lock(b);
 	dentry->d_flags &= ~DCACHE_PAR_LOOKUP;