This is an automated email from the ASF dual-hosted git repository.
xiaoxiang781216 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
The following commit(s) were added to refs/heads/master by this push:
new b7c8430de69 spinlock: fix ticket lock corruption in trylock and unlock
b7c8430de69 is described below
commit b7c8430de698e43386683e12f10680c48e90ba26
Author: hujun5 <[email protected]>
AuthorDate: Mon Aug 17 13:46:09 2026 +0800
spinlock: fix ticket lock corruption in trylock and unlock
Two defects in the CONFIG_TICKET_SPINLOCK paths of spinlock.h:
1. spin_trylock_notrace() passed &lock->owner as the "expected" pointer
of atomic_cmpxchg(). A failed compare-exchange writes the current
value of the target object back through that pointer, so a losing
trylock stores lock->next into lock->owner. owner then equals next,
which is the unlocked state: a lock still held by another CPU reports
itself as free, spin_is_locked() returns false and the lock can be
taken again. Every later unlock keeps incrementing owner past next,
so the ticket of a real waiter never matches and the lock stays
locked forever. Keep the expected value in a local variable.
2. spin_unlock() was wrapped in #ifdef __SP_UNLOCK_FUNCTION, a macro
that is never defined anywhere in the tree. The function body was
therefore dead code and spin_unlock() always expanded to
"do { *(l) = SP_UNLOCKED; } while (0)", which zeroes both ticket
counters instead of releasing one ticket with
atomic_fetch_add(&lock->owner, 1). That drops queued waiters, lets a
newcomer draw ticket 0 and enter the critical section, and also skips
the UP_DMB/UP_DSB/UP_SEV release barriers and the
sched_note_spinlock_unlock() note. Drop the dead #ifdef so
spin_unlock() is always the function.
Both were reproduced on qemu-armv7a:smp (cortex-a7 x4) with
CONFIG_TICKET_SPINLOCK=y, where the compare-exchange lowers to native
ldrex/strex. This confirms the root cause is the C-level aliasing of
the expected pointer, not the atomic implementation.
Refs: https://github.com/apache/nuttx/issues/19808
Signed-off-by: hujun5 <[email protected]>
---
include/nuttx/spinlock.h | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/include/nuttx/spinlock.h b/include/nuttx/spinlock.h
index 208686b5b55..63884365547 100644
--- a/include/nuttx/spinlock.h
+++ b/include/nuttx/spinlock.h
@@ -276,8 +276,18 @@ static inline_function bool
spin_trylock_notrace(FAR volatile spinlock_t *lock)
{
#ifdef CONFIG_TICKET_SPINLOCK
- if (!atomic_cmpxchg(&lock->next, &lock->owner,
- atomic_read(&lock->next) + 1))
+ /* The expected value must live in a local. A failed compare-exchange
+ * writes the current value of the target object back through the
+ * expected pointer, so passing &lock->owner here would clobber the
+ * owner counter and make a lock held by another CPU appear unlocked.
+ *
+ * The exchange succeeds only when next == owner, which is the unlocked
+ * state of a ticket lock.
+ */
+
+ uint32_t expected = atomic_read(&lock->owner);
+
+ if (!atomic_cmpxchg(&lock->next, &expected, expected + 1))
#else /* CONFIG_TICKET_SPINLOCK */
if (up_testset(lock) == SP_LOCKED)
#endif /* CONFIG_TICKET_SPINLOCK */
@@ -394,7 +404,6 @@ spin_unlock_notrace(FAR volatile spinlock_t *lock)
****************************************************************************/
#ifdef CONFIG_SPINLOCK
-# ifdef __SP_UNLOCK_FUNCTION
static inline_function void spin_unlock(FAR volatile spinlock_t *lock)
{
/* Unlock without trace note */
@@ -405,9 +414,6 @@ static inline_function void spin_unlock(FAR volatile
spinlock_t *lock)
sched_note_spinlock_unlock(lock);
}
-# else
-# define spin_unlock(l) do { *(l) = SP_UNLOCKED; } while (0)
-# endif
#else
# define spin_unlock(lock)
#endif /* CONFIG_SPINLOCK */
