Re: rt_spin_unlock order of operations [was: Re: [syzbot] [fs?] KASAN: slab-use-after-free Read in shrink_dcache_tree]

[Thomas Gleixner]

On Thu, Jun 18 2026 at 22:03, Al Viro wrote:

> On Thu, Jun 18, 2026 at 09:59:53PM +0100, Al Viro wrote:
>> > https://docs.kernel.org/next/RCU/whatisRCU.html guarantees that
>> > spinlock APIs imply RCU, and
>> > https://docs.kernel.org/locking/mutex-design.html says: "This is in
>> > contrast with spin_unlock() [...], which APIs can be used to guarantee
>> > that the memory is not touched by the lock implementation after
>> > spin_unlock()/completion_done() releases the lock.".
>> > Neither of these explicitly guarantees that the RCU read-side critical
>> > section (and the protection against migration?) should still hold
>> > while the lock is being dropped, but I think that would fit best with
>> > the explicit guarantees?
>> 
>> I'm trying to recall if PREEMPT_RT had been enabled in the last round of
>> UAF in that area back in early April...
>> 
>> As far as I'm concerned, we *do* need to keep RCU read-side critical area
>> all the way until the end of spin_unlock(); it very well might be the
>> only thing to prevent freeing the sucker under us.

Right. That's clearly a bug in rt_spin_unlock(). I think I wrote it that
way for symmetry vs. lock(), which is obviously wrong.

Fix below.

Thanks,

        tglx
---
Subject: locking/rt: Fix the incorrect RCU protection in rt_spin_unlock()
From: Thomas Gleixner <tglx@xxxxxxxxxx>
Date: Thu, 18 Jun 2026 23:32:43 +0200

rt_spin_unlock() releases the RCU protection before unlocking the
lock. That opens the door for the following UAF scenario:

 T1					T2
 spin_lock(&p->lock);		rcu_read_lock();
 invalidate(p);			p = rcu_dereference(ptr);
 rcu_assign_pointer(ptr, NULL);	if (!p) return; // Not taken
 spin_unlock(&p->lock);		spin_lock(&p->lock)
 				   lock(&lock->lock);
				   rcu_read_lock();
 kfree_rcu(p);			rcu_read_unlock();
				....
				spin_unlock(&p->lock)
				  rcu_read_unlock(); // Ends grace period
 rcu_do_batch()
   kfree(p);
			 UAF ->	  rt_mutex_cmpxchg_release(&lock->lock...)

Regular spinlocks keep preemption disabled accross the unlock operation,
which provides full RCU protection, but the RT substitution fails to
resemble that.

Move the rcu_read_unlock() invocation past the unlock operation to match
the non-RT semantics and add a comment explaining why rcu_read_unlock()
must come last.

This makes it asymmetric vs. rt_spin_lock(), but that's harmless as the
caller needs to hold RCU read lock across the lock operation. The
migrate_enable() call stays before the unlock operation because there is
no per CPU operation in the unlock path which would require migration to
be kept disabled.

Fixes: 0f383b6dc96e ("locking/spinlock: Provide RT variant")
Reported-by: syzbot+000c800a02097aaa10ed@xxxxxxxxxxxxxxxxxxxxxxxxx
Decoded-by: Jann Horn <jannh@xxxxxxxxxx>
Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
---
 kernel/locking/spinlock_rt.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

--- a/kernel/locking/spinlock_rt.c
+++ b/kernel/locking/spinlock_rt.c
@@ -79,10 +79,27 @@ void __sched rt_spin_unlock(spinlock_t *
 {
 	spin_release(&lock->dep_map, _RET_IP_);
 	migrate_enable();
-	rcu_read_unlock();
 
 	if (unlikely(!rt_mutex_cmpxchg_release(&lock->lock, current, NULL)))
 		rt_mutex_slowunlock(&lock->lock);
+
+	/*
+	 * This must be last to prevent the following UAF:
+	 *
+	 * T1					T2
+	 * spin_lock(&p->lock);			rcu_read_lock();
+	 * invalidate(p);			p = rcu_dereference(ptr);
+	 * rcu_assign_pointer(ptr, NULL);	if (!p) return;
+	 * spin_unlock(&p->lock);		spin_lock(&p->lock);
+	 * kfree_rcu(p);			rcu_read_unlock();
+	 *					....
+	 *					spin_unlock(&p->lock)
+	 *					  rcu_read_unlock(); // Ends grace period
+	 * rcu_do_batch()
+	 *   kfree(p);
+	 *			    UAF ->	  rt_mutex_cmpxchg_release(&p->lock.lock...)
+	 */
+	rcu_read_unlock();
 }
 EXPORT_SYMBOL(rt_spin_unlock);