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

[Jeff Layton <jlayton@kernel.org>]

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>