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

Paul E. McKenney
2026-04-10T18:36:04Z

On Fri, Apr 10, 2026 at 02:26:47PM -0400, Jeff Layton wrote:
> On Fri, 2026-04-10 at 10:32 -0700, Paul E. McKenney 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?
> >
> > Adding to Boqun's email:
> >
> > The https://lore.kernel.org/all/20260410084839.GA1310153@ZenIV URL
> > has preemption disabled (right, Jeff?)
> >
>
> Not as familiar with this part of the kernel, but I think that's
> correct:
>
> # zgrep PREEMPT /proc/config.gz
> CONFIG_PREEMPT_NONE_BUILD=y
> CONFIG_ARCH_HAS_PREEMPT_LAZY=y
> CONFIG_PREEMPT_NONE=y
> # CONFIG_PREEMPT_VOLUNTARY is not set
> # CONFIG_PREEMPT is not set
> # CONFIG_PREEMPT_LAZY is not set
> # CONFIG_PREEMPT_RT is not set
> # CONFIG_PREEMPT_DYNAMIC is not set
> CONFIG_HAVE_PREEMPT_DYNAMIC=y
> CONFIG_HAVE_PREEMPT_DYNAMIC_CALL=y
> CONFIG_PREEMPT_NOTIFIERS=y
> # CONFIG_PREEMPTIRQ_DELAY_TEST is not set

Thank you, Jeff!

Yes that has preemption entirely disabled, so RCU readers do nothing but
disable and enable preemption. Except that it is everywhere disabled,
so the order of the rcu_read_unlock() and the spin_unlock() should
not matter. (Famous last words!)

Thanx, Paul

> > , in which case rcu_read_lock()
> > is just preempt_disable() and rcu_read_unlock() is just preempt_enable().
> > All the spinlocks also disable and enable preemption, so if you have
> > something like this:
> >
> > rcu_read_lock();
> > do_something();
> > spin_lock(...);
> > do_something_else();
> > rcu_read_unlock();
> > do_yet_another_thing();
> > spin_unlock(...);
> >
> > Then there should not be any calls to schedule() between the initial
> > rcu_read_lock() and the final spin_unlock(). But this is just
> > preempt_disable() doing its job, not much to do with RCU.
> >
> > And repeating Boqun's email:
> >
> > Now if this is a preemptible kernel, things are more involved.
> > However, in this case we know that the kernel has been built with
> > CONFIG_PREEMPT_COUNT=y. So rcu_read_unlock_special() can sample
> > preempt_count() and set preempt_bh_were_disabled is then true if either
> > preemption or softirq are disabled. Then irqs_were_disabled is set if
> > interrupts were disabled on entry. If either are set, the inner pair of
> > "if" statements set up to defer the end of the read-side critical section.
> > If this is important, which it might be, I will go through this in more
> > detail later.
> >
> > Otherwise, the call to rcu_preempt_deferred_qs_irqrestore() at the end
> > of rcu_read_unlock_special() immediately ends the RCU read-side critical
> > section.
> >
> > And more adding to Boqun's email:
> >
> > Now, CONFIG_PREEMPT_RT=y is a special case, because "spinlocks"
> > are preemptible. So __rt_spin_lock() and __rt_spin_trylock() both do
> > rcu_read_lock() and rt_spin_unlock() does rcu_read_unlock(). So that
> > should be covered via RCU nesting along with the rcu_read_unlock_special()
> > checks for disabled interrupts, softirq, and preemption.
> >
> > And rcutorture tests random overlapping of RCU readers with all those
> > disabled regions. But it might still be missing something. And I
> > will add spinlocks to the mix in addition to the preemption disabling.
> >
> > In the meantime, is there something more specific that I should be
> > looking at?
> >
> > Thanx, Paul
>
> --
> Jeff Layton <jlayton@kernel.org>