hujun260 opened a new pull request, #19880: URL: https://github.com/apache/nuttx/pull/19880
## Summary Two independent defects in the `CONFIG_TICKET_SPINLOCK` paths of `include/nuttx/spinlock.h`. Refs: https://github.com/apache/nuttx/issues/19808 ### 1. `spin_trylock_notrace()` corrupts the lock it fails to acquire ```c if (!atomic_cmpxchg(&lock->next, &lock->owner, atomic_read(&lock->next) + 1)) ``` `&lock->owner` is passed as the *expected* pointer of the compare-exchange. 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`. This is explicit in NuttX's own fallback implementation, `CMP_EXCHANGE` in `libs/libc/machine/arch_atomic.c`, whose else branch does `*tmpexp = *tmpmem;`. After that write `owner == next`, which is exactly the *unlocked* state of a ticket lock. Consequences: - a lock still held by another CPU reports itself as free, `spin_is_locked()` returns false, and the lock can be taken again, so two CPUs end up inside the same critical section; - every subsequent unlock keeps incrementing `owner` past `next`, so the ticket of a real waiter never matches and the lock stays locked forever. Fix: keep the expected value in a local variable. The exchange then succeeds only when `next == owner`, i.e. the lock is free. Note that `rspin_lock()` / `rspin_trylock()` already use a local `old_val` as the expected value and are not affected. ### 2. `spin_unlock()` was never the function The function body was guarded by `#ifdef __SP_UNLOCK_FUNCTION`, a macro that is **not defined anywhere in the tree**. The `#ifdef` was therefore dead code and `spin_unlock()` always expanded to ```c #define spin_unlock(l) do { *(l) = SP_UNLOCKED; } while (0) ``` For a ticket lock `SP_UNLOCKED` is `{0, 0}`, so this zeroes *both* counters instead of releasing one ticket with `atomic_fetch_add(&lock->owner, 1)`. Already queued waiters are dropped, and a newcomer draws ticket 0 and walks straight into the critical section. The macro also skips the `UP_DMB` / `UP_DSB` / `UP_SEV` sequence, so there is no release barrier and waiters parked in `UP_WFE()` inside `spin_lock_notrace()` are not woken, and it skips `sched_note_spinlock_unlock()`. Fix: drop the dead `#ifdef` so `spin_unlock()` is always the function that calls `spin_unlock_notrace()` and emits the note. ## Impact - Architectures / boards: none specifically; affects any SMP configuration. Correctness fix only, no API or configuration change. - `CONFIG_TICKET_SPINLOCK=y` builds: `spin_trylock()` / `spin_trylock_notrace()` no longer destroy the lock state on failure, and `spin_unlock()` now releases a single ticket instead of resetting the lock. - **All** `CONFIG_SPINLOCK=y` builds, including the default non-ticket ones: `spin_unlock()` is now a real function, so it gains the `UP_DMB` / `UP_DSB` / `UP_SEV` barriers it was missing and emits `sched_note_spinlock_unlock()`. The stored value is unchanged (`*lock = SP_UNLOCKED` inside `spin_unlock_notrace()`). Users of `CONFIG_SCHED_INSTRUMENTATION_SPINLOCK` will see unlock notes that were previously absent. - In-tree callers of the bare `spin_unlock()` that change behaviour: `boards/boardctl.c`, `drivers/power/pm/pm_idle.c`, `arch/arm/src/rp2040/rp2040_flash_mtd.c`, `arch/arm/src/rp23xx/rp23xx_flash_mtd.c`, `arch/risc-v/src/mpfs/mpfs_coremmc.c`, `arch/risc-v/src/mpfs/mpfs_emmcsd.c`, `arch/risc-v/src/common/espressif/esp_timer_adapter.c`, `arch/xtensa/src/common/espressif/esp_timer_adapter.c`. - Documentation: none. Security: none. Build: none. ## Testing **Host:** Ubuntu 24.04.1 LTS, x86_64, gcc 13.3.0, arm-none-eabi-gcc 13.2.1 (Arm GNU Toolchain 13.2.rel1), QEMU 9.2.0. **Target:** `qemu-armv7a:smp` with `CONFIG_TICKET_SPINLOCK=y` enabled on top of the shipped defconfig (that config change is not part of this PR). `CONFIG_BUILD_FLAT=y` lets a test application include `<nuttx/spinlock.h>` and call the kernel inline lock primitives directly, so `CONFIG_BOARDCTL_SPINLOCK` is not needed. ``` make distclean -j20 ./tools/configure.sh -l qemu-armv7a:smp kconfig-tweak --enable CONFIG_TICKET_SPINLOCK && make olddefconfig make -j20 qemu-system-arm -cpu cortex-a7 -smp 4 -nographic \ -machine virt,virtualization=off,gic-version=2 -net none \ -chardev stdio,id=con,mux=on -serial chardev:con \ -mon chardev=con,mode=readline -kernel ./nuttx ``` The test application runs three checks and prints `owner` / `next` / `spin_is_locked()` around every step: - **Test 1** - single threaded and fully deterministic: take the lock, call `spin_trylock()` on it, and inspect the counters. Run twice, once against the in-tree `spin_trylock()` and once against a local copy carrying the fix, so both variants are exercised in the same boot. - **Test 2** - two pthreads pinned to CPU1 / CPU2 with `pthread_attr_setaffinity_np()`; B calls `spin_trylock()` while A holds the lock. - **Test 3** - call the public `spin_unlock()` on a ticket lock that has one queued waiter. Every busy-wait has an iteration budget, otherwise a broken lock hangs the board before anything can be printed. ### Before the fix (`CONFIG_TICKET_SPINLOCK=y`) - Test 1: `spin_trylock()` correctly returned false, but the failed call moved `owner` from 0 to 1. `spin_is_locked()` then reported false on a lock we were still holding, and `spin_lock()` on that same lock succeeded again. After two unlocks `owner=3 > next=2`, and the lock was permanently stuck locked. - Test 2: thread B entered the critical section while A held the lock. - Test 3: `spin_unlock()` turned `owner=0 next=2` into `{0, 0}`, discarding the queued ticket 1. ### After the fix ``` NuttShell (NSH) NuttX-13.0.0 nsh> hello ticket spinlock trylock repro (apache/nuttx#19808), ncpus=4 [Test 1: current spin_trylock] initial owner=0 next=0 is_locked=false after spin_lock (we hold it) owner=0 next=1 is_locked=true trylock on a HELD lock returned false (must be false) after the failed trylock owner=0 next=1 is_locked=true OK: lock state intact after unlock owner=1 next=1 is_locked=false trylock on a FREE lock returned true (must be true) after the successful trylock owner=1 next=2 is_locked=true [Test 1: suggested fix] initial owner=0 next=0 is_locked=false after spin_lock (we hold it) owner=0 next=1 is_locked=true trylock on a HELD lock returned false (must be false) after the failed trylock owner=0 next=1 is_locked=true OK: lock state intact after unlock owner=1 next=1 is_locked=false trylock on a FREE lock returned true (must be true) after the successful trylock owner=1 next=2 is_locked=true [Test 3: the spin_unlock() macro on a ticket lock] we hold ticket 0 owner=0 next=1 is_locked=true a waiter queued ticket 1 owner=0 next=2 is_locked=true after spin_unlock() owner=1 next=2 is_locked=true OK: owner advanced to 1, the queued waiter gets in [Test 2: two CPUs inside one critical section] A (cpu1): taking the lock B (cpu2): A holds the lock, calling spin_trylock B sees before trylock owner=0 next=1 is_locked=true B: trylock returned false B sees after trylock owner=0 next=1 is_locked=true result: exclusion held nsh> ``` All three checks pass and the two Test 1 variants now behave identically, which is the expected result once the in-tree version matches the fix. On this target the compare-exchange lowers to native ARMv7-A `ldrex` / `strex`, not the software fallback in `libs/libc/machine/arch_atomic.c`, confirming the root cause is the C-level aliasing of the expected pointer rather than the atomic implementation. `./tools/nxstyle include/nuttx/spinlock.h` is clean, and the build produces no new warnings. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
