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

Paul E. McKenney
2026-04-10T19:21:53Z

On Fri, Apr 10, 2026 at 07:52:43PM +0100, Al Viro wrote:
> 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...

If there is a case where a spinlock fails to act as an RCU read-side
critical section, then we have a bug that needs to be fixed.

Thanx, Paul

> Interesting...