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

Al Viro
2026-04-10T18:48:57Z

On Fri, Apr 10, 2026 at 08:25:27AM -0700, Linus Torvalds wrote:
> [ Adding RCU maintainers to the participants: see
>
> https://lore.kernel.org/all/20260410084839.GA1310153@ZenIV/
>
> and the fairly long thread associated with it for context ]
>
> On Fri, 10 Apr 2026 at 01:44, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > [this may or may not be the source of UAFs caught by Jeff and by Helge]
>
> Hmm.
>
> I think this patch may indeed fix the problem, and I don't mind how it looks.
>
> But while I think the patch looks fine, I am still quite unhappy about
> it if it matters - because we have very much documented that spinlocks
> in themselves are also RCU read locks:
>
> Note that anything that disables bottom halves, preemption,
> or interrupts also enters an RCU read-side critical section.
> Acquiring a spinlock also enters an RCU read-side critical
> sections, even for spinlocks that do not disable preemption,
> as is the case in kernels built with CONFIG_PREEMPT_RT=y.
> Sleeplocks do *not* enter RCU read-side critical sections.
>
> so *if* this makes a difference, I think it's actually implying that
> there's something wrong with our "rcu_read_unlock()" implementation,
> and that it doesn't nest properly.
>
> Because our documentation also makes it very clear that this should
> all work as-is, and your patch should be a complete no-op. Just a few
> lines later in that core RCU doc, we have
>
> Note that RCU read-side critical sections may be nested and/or
> overlapping.
>
> so the order of the spin_unlock() and the rcu_read_unlock() *should*
> be entirely immaterial, and the order of unlocking simply shouldn't
> matter.
>
> So it may indeed be that relying on the ordering of RCU read unlock
> matters for the case of explicit unlocks and the implicit unlocks by a
> spinlock, but that's not great.
>
> Does the rcu_read_unlock() code perhaps only check the RCU count, not
> the atomicity count? That would explain it, and looking at
> __rcu_read_unlock() in kernel/rcu/tree_plugin.h that may indeed be the
> case (rcu_preempt_read_exit() seems to only check
> rcu_read_lock_nesting).
>
> This is most likely dependent on kernel config options, and it may all
> be unavoidable becasue RCU might not even have a way to tell that it's
> still in a critical region without preemption counts etc.
>
> But if it's unavoidable, we need to update the docs about this gotcha,
> and we need to have some tooling scan our existing code for this
> documentation change.
>
> RCU people - what do you think?

FWIW, I wonder if we would be better off with the following:

void shrink_dentry_list(struct list_head *list)
{
while (!list_empty(list)) {
struct dentry *dentry;

dentry = list_entry(list->prev, struct dentry, d_lru);
spin_lock(&dentry->d_lock);
d_shrink_del(dentry);
if (unlikely(dentry->d_flags & DCACHE_DENTRY_KILLED)) {
// killed while on shrink list, freeing left to us
spin_unlock(&dentry->d_lock);
dentry_free(dentry);
continue;
}
rcu_read_lock();
if (!lock_for_kill(dentry)) {
spin_unlock(&dentry->d_lock);
rcu_read_unlock();
continue;
}
shrink_kill(dentry);
}
}

because in that case both callers of shrink_kill() have the same form - the
other one is
if (!lock_for_kill(v)) {
spin_unlock(&v->d_lock);
rcu_read_unlock();
} else {
shrink_kill(v);
}
and we could fold lock_for_kill() into shrink_kill(), with a nice series of
cleanups becoming possible after that, especially if we can rely upon
spinlock acting as RCU read-side critical in all cases...

Interesting...