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

Al Viro
2026-04-10T08:44:52Z

[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;