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

Jeff Layton
2026-04-10T11:56:16Z

On Fri, 2026-04-10 at 07:18 -0400, Jeff Layton wrote:
> On Fri, 2026-04-10 at 09:48 +0100, Al Viro wrote:
> > [this may or may not be the source of UAFs caught by Jeff and by Helge]
> >
> > lock_for_kill() losing a race to eviction by another thread will end up
> > returning false (correctly - the caller should *not* evict dentry in
> > that case), with ->d_lock held and rcu read-critical area still unbroken,
> > so the caller can safely drop the locks, provided that it's done in the
> > right order - ->d_lock should be dropped before rcu_read_unlock().
> >
> > Unfortunately, 3 of 4 callers did it the other way round and for two of them
> > (finish_dput() and shrink_kill()) that ended up with a possibility of UAF.
> > The third (shrink_dentry_list()) had been safe for other reasons (dentry being
> > on the shrink list => the other thread wouldn't even schedule its freeing
> > until observing dentry off the shrink list while holding ->d_lock), but it's
> > simpler to make it a general rule - in case of lock_for_kill() failure
> > ->d_lock must be dropped before rcu_read_unlock().
> >
> > UAF scenario was basically this:
> > dput() drops the last reference to foo/bar and evicts it.
> > The reference it held to foo happens to be the last one.
> > When trylock on ->i_lock of parent's inode fails, we
> > (under rcu_read_lock()) drop ->d_lock and take the locks in
> > the right order; while we'd been spinning on ->i_lock somebody
> > else comes and evicts the parent.
> > Noticing non-zero (negative) refcount we decide there's nothing
> > left to do. And had we only dropped parent's ->d_lock before
> > rcu_read_unlock(), everything would be fine. Unfortunately,
> > doing that the other way round allows rcu-scheduled freeing of
> > parent to proceed before we drop its ->d_lock.
> > The same applies if instead of final dput() of foo/bar it gets evicted
> > by shrink_dcache_parent() or memory pressure, again taking out the last
> > reference to that used to pin its parent.
> >
> > Fixes: 339e9e13530b ("don't try to cut corners in shrink_lock_dentry()")
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 0c8faeee02e2..b1c163f20db6 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -751,6 +751,8 @@ static struct dentry *__dentry_kill(struct dentry *dentry)
> > *
> > * Return false if dentry is busy. Otherwise, return true and have
> > * that dentry's inode locked.
> > + *
> > + * On failure the caller must drop ->d_lock *before* rcu_read_unlock()
> > */
> >
> > static bool lock_for_kill(struct dentry *dentry)
> > @@ -933,8 +935,8 @@ static void finish_dput(struct dentry *dentry)
> > }
> > rcu_read_lock();
> > }
> > - rcu_read_unlock();
> > spin_unlock(&dentry->d_lock);
> > + rcu_read_unlock();
> > }
> >
> > /*
> > @@ -1193,11 +1195,12 @@ static inline void shrink_kill(struct dentry *victim)
> > do {
> > rcu_read_unlock();
> > victim = __dentry_kill(victim);
> > + if (!victim)
> > + return;
> > rcu_read_lock();
> > - } while (victim && lock_for_kill(victim));
> > + } while (lock_for_kill(victim));
> > + spin_unlock(&victim->d_lock);
> > rcu_read_unlock();
> > - if (victim)
> > - spin_unlock(&victim->d_lock);
> > }
> >
> > void shrink_dentry_list(struct list_head *list)
> > @@ -1210,10 +1213,10 @@ void shrink_dentry_list(struct list_head *list)
> > rcu_read_lock();
> > if (!lock_for_kill(dentry)) {
> > bool can_free;
> > - rcu_read_unlock();
> > d_shrink_del(dentry);
> > can_free = dentry->d_flags & DCACHE_DENTRY_KILLED;
> > spin_unlock(&dentry->d_lock);
> > + rcu_read_unlock();
> > if (can_free)
> > dentry_free(dentry);
> > continue;
>
> I'm a little skeptical that this explains the UAF we've seen. It looks
> like this would mostly affect the parent dentry rather than the
> children on the shrink list, but parents can be children too, so we
> can't rule it out.
>
> Either way, releasing the rcu_read_lock() protecting a dentry you're
> holding a lock on seems like a bad idea.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

FWIW, here's Claude's review:

Summary
-------

The patch fixes a use-after-free caused by incorrect lock ordering
when lock_for_kill() returns false (race lost to another thread's
eviction). Three of four callers were dropping rcu_read_unlock()
before spin_unlock(&dentry->d_lock), which allowed RCU-scheduled
freeing of the dentry to proceed while d_lock was still held on
the freed object.

Changes
-------

finish_dput() (line 935-938):

Swaps the unlock order so d_lock is dropped before rcu_read_unlock().
This is reachable from dput() and d_make_discardable(). The UAF
scenario described in the commit message applies directly here:
dput() evicts foo/bar, the parent's refcount drops to zero,
lock_for_kill() fails on the parent (trylock on i_lock failed and
during the retry another thread evicted the parent), then
rcu_read_unlock() lets the parent be freed while d_lock is still
held on it.

shrink_kill() (lines 1193-1203):

Restructured to pull the !victim early return out of the while
condition. When __dentry_kill() returns NULL (parent's refcount
didn't drop to zero), the function now returns immediately without
touching rcu at all -- correct, since __dentry_kill() entered
with rcu_read_unlock() already called. When lock_for_kill() fails,
the loop exits and d_lock is dropped before rcu_read_unlock().
This is the path most likely responsible for the production crashes
in T262643929: shrink_dentry_list() calls shrink_kill(), which
walks up to parent dentries via __dentry_kill().

shrink_dentry_list() (lines 1213-1218):

Swaps the unlock order in the lock_for_kill() failure path. As
the commit message notes, this was already safe for other reasons:
the dentry is on a shrink list (DCACHE_SHRINK_LIST set), so the
other thread's __dentry_kill() would see that flag and set
can_free = false, preventing dentry_free() from being called.
Changed for consistency with the general rule.

lock_for_kill() comment:

Documents the ordering requirement: on failure, callers must drop
d_lock before rcu_read_unlock().

Production crash relevance
--------------------------

The shrink_kill() fix directly addresses the UAF pattern observed
in three production vmcores (od0103.dkl2, od5175.prn6,
twshared90354.17.frc2). In all three, the crashed thread was in
shrink_dentry_list() -> spin_lock() on a dentry whose slab page had
been freed and reused by kmalloc-64 or kmalloc-96.

The scenario: shrink_dentry_list() kills a dentry via shrink_kill(),
__dentry_kill() returns the parent with d_lock held and refcount
decremented to zero, lock_for_kill() fails on the parent because
another thread raced in and evicted it. With the old code, the
rcu_read_unlock() in shrink_kill() allowed the parent's
call_rcu-scheduled free to complete, and the subsequent
spin_unlock(&victim->d_lock) touched freed memory.

Whether this is the full explanation for the list corruption
observed in the vmcores (LRU dentries spliced onto the dispose
list) remains an open question. The lock ordering fix prevents
the immediate UAF on the parent dentry, but the mechanism by
which entire LRU chains ended up on the dispose list may involve
additional factors.

--
Jeff Layton <jlayton@kernel.org>