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

Paul E. McKenney
2026-04-10T17:32:42Z

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?), 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