[PATCH bpf-next] rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule

[PATCH bpf-next] rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule

[Gabriele Monaco]

raw_res_spin_unlock_irqrestore() calls raw_res_spin_unlock() and then
restores interrupts, this means preemption is enabled when interrupts
are still disabled (as part of raw_res_spin_unlock()) so this cannot
trigger an actual preemption.
This is inconsistent with other spinlock implementations
(raw_spin_unlock_irqrestore() and bpf_res_spin_unlock_irqrestore()
itself).

Adjust the macro to ensure interrupts are enabled before enabling
preemption, allowing to schedule at that point. Make the same
modification in the error path of raw_res_spin_lock_irqsave().

Fixes: 101acd2e78b1 ("rqspinlock: Add macros for rqspinlock usage")
Cc: stable@vger.kernel.org
Acked-by: Arnd Bergmann <arnd@arndb.de> # asm-generic
Acked-by: Waiman Long <longman@redhat.com>
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
---
New submission of [1]

[1] - https://lore.kernel.org/lkml/20260609094941.56122-1-gmonaco@redhat.com
---
 include/asm-generic/rqspinlock.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/rqspinlock.h b/include/asm-generic/rqspinlock.h
index 151d267a49..4d46643f46 100644
--- a/include/asm-generic/rqspinlock.h
+++ b/include/asm-generic/rqspinlock.h
@@ -243,12 +243,20 @@ static __always_inline void res_spin_unlock(rqspinlock_t *lock)
 	({                                        \
 		int __ret;                        \
 		local_irq_save(flags);            \
-		__ret = raw_res_spin_lock(lock);  \
-		if (__ret)                        \
+		preempt_disable();                \
+		__ret = res_spin_lock(lock);      \
+		if (__ret) {                      \
 			local_irq_restore(flags); \
+			preempt_enable();         \
+		}                                 \
 		__ret;                            \
 	})
 
-#define raw_res_spin_unlock_irqrestore(lock, flags) ({ raw_res_spin_unlock(lock); local_irq_restore(flags); })
+#define raw_res_spin_unlock_irqrestore(lock, flags) \
+	({                                          \
+		res_spin_unlock(lock);              \
+		local_irq_restore(flags);           \
+		preempt_enable();                   \
+	})
 
 #endif /* __ASM_GENERIC_RQSPINLOCK_H */

base-commit: e43ffb69e0438cddd72aaa30898b4dc446f664f8
-- 
2.54.0

Re: [PATCH bpf-next] rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] kernel/locking/locktorture.c ignores the return value of raw_res_spin_lock and raw_res_spin_lock_irqsave, proceeding to modify shared state without holding the lock on failure.
--

commit 92f88363aff605830b5600d6bc2a3ec351170c8e
Author: Gabriele Monaco <gmonaco@redhat.com>

rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule

This commit fixes the preemption enabling order in the resilient spinlock
unlock macros. It ensures interrupts are enabled before preemption to
allow scheduling, maintaining consistency with other spinlock implementations.

> diff --git a/include/asm-generic/rqspinlock.h b/include/asm-generic/rqspinlock.h
> index 151d267a496b3..4d46643f46cbc 100644
> --- a/include/asm-generic/rqspinlock.h
> +++ b/include/asm-generic/rqspinlock.h
> @@ -243,12 +243,20 @@ static __always_inline void res_spin_unlock(rqspinlock_t *lock)
>  	({                                        \
>  		int __ret;                        \
>  		local_irq_save(flags);            \
> -		__ret = raw_res_spin_lock(lock);  \
> -		if (__ret)                        \
> +		preempt_disable();                \
> +		__ret = res_spin_lock(lock);      \
> +		if (__ret) {                      \
>  			local_irq_restore(flags); \
> +			preempt_enable();         \
> +		}                                 \
>  		__ret;                            \
>  	})
>  
> -#define raw_res_spin_unlock_irqrestore(lock, flags) ({ raw_res_spin_unlock(lock); local_irq_restore(flags); })
> +#define raw_res_spin_unlock_irqrestore(lock, flags) \
> +	({                                          \
> +		res_spin_unlock(lock);              \
> +		local_irq_restore(flags);           \
> +		preempt_enable();                   \
> +	})

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the resilient
spinlock changes, I noticed a pre-existing issue in the locktorture module.

Does locktorture correctly handle the return values from the resilient
spinlock API?

In kernel/locking/locktorture.c:torture_raw_res_spin_write_lock(), the
return value is discarded:

    raw_res_spin_lock(&rqspinlock);
    return 0;

Similarly, in torture_raw_res_spin_write_lock_irq():

    raw_res_spin_lock_irqsave(&rqspinlock, flags);
    cxt.cur_ops->flags = flags;
    return 0;

Since raw_res_spin_lock and raw_res_spin_lock_irqsave can fail and return
an error code like -EDEADLK or -ETIMEDOUT, ignoring the return value
allows the caller to proceed unlocked into the critical section.

When the torture thread later calls raw_res_spin_unlock(), it will cause
an unbalanced preempt_enable() because the failed lock acquisition path
already restored preemption.

Furthermore, it forcibly clears the lock state, which could unlock a lock
legitimately held by another thread, potentially causing lock state
corruption and panics.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610090431.32427-1-gmonaco@redhat.com?part=1

Re: [PATCH bpf-next] rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2673 bytes --]

> diff --git a/include/asm-generic/rqspinlock.h b/include/asm-generic/rqspinlock.h
> index 151d267a496b..4d46643f46cb 100644
> --- a/include/asm-generic/rqspinlock.h
> +++ b/include/asm-generic/rqspinlock.h
> @@ -243,12 +243,20 @@ static __always_inline void res_spin_unlock(rqspinlock_t *lock)
>  	({                                        \
>  		int __ret;                        \
>  		local_irq_save(flags);            \
> -		__ret = raw_res_spin_lock(lock);  \
> -		if (__ret)                        \
> +		preempt_disable();                \
> +		__ret = res_spin_lock(lock);      \
> +		if (__ret) {                      \
>  			local_irq_restore(flags); \
> +			preempt_enable();         \
> +		}                                 \
>  		__ret;                            \
>  	})
>
> -#define raw_res_spin_unlock_irqrestore(lock, flags) ({ raw_res_spin_unlock(lock); local_irq_restore(flags); })
> +#define raw_res_spin_unlock_irqrestore(lock, flags) \
> +	({                                          \
> +		res_spin_unlock(lock);              \
> +		local_irq_restore(flags);           \
> +		preempt_enable();                   \
> +	})
>
>  #endif /* __ASM_GENERIC_RQSPINLOCK_H */

Related issue in kernel/locking/locktorture.c

During the v2 review discussion, the Sashiko AI bot identified a
pre-existing bug in the locktorture module where return values from
raw_res_spin_lock() and raw_res_spin_lock_irqsave() are ignored in
torture_raw_res_spin_write_lock():

    static int torture_raw_res_spin_write_lock(int tid __maybe_unused)
    {
        raw_res_spin_lock(&rqspinlock);
        return 0;
    }

Since these functions can fail with -EDEADLK or -ETIMEDOUT, ignoring
the return value allows the caller to proceed into the critical section
without holding the lock. When the torture thread later calls
raw_res_spin_unlock(), it will cause an unbalanced preempt_enable()
because the failed lock acquisition path already restored preemption.

Furthermore, forcibly clearing the lock state could unlock a lock
legitimately held by another thread, potentially causing lock state
corruption.

The same issue exists in torture_raw_res_spin_write_lock_irq() which
also ignores the return value from raw_res_spin_lock_irqsave().

Should these torture functions check the return values and handle lock
acquisition failures appropriately?

Reference: https://lore.kernel.org/all/20260610091602.318C01F00893@smtp.kernel.org/


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/27265934925

Re: [PATCH bpf-next] rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule

[Kumar Kartikeya Dwivedi]

On Wed Jun 10, 2026 at 11:16 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] kernel/locking/locktorture.c ignores the return value of raw_res_spin_lock and raw_res_spin_lock_irqsave, proceeding to modify shared state without holding the lock on failure.
> --
>
> commit 92f88363aff605830b5600d6bc2a3ec351170c8e
> Author: Gabriele Monaco <gmonaco@redhat.com>
>
> rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule
>
> This commit fixes the preemption enabling order in the resilient spinlock
> unlock macros. It ensures interrupts are enabled before preemption to
> allow scheduling, maintaining consistency with other spinlock implementations.
>
>> diff --git a/include/asm-generic/rqspinlock.h b/include/asm-generic/rqspinlock.h
>> index 151d267a496b3..4d46643f46cbc 100644
>> --- a/include/asm-generic/rqspinlock.h
>> +++ b/include/asm-generic/rqspinlock.h
>> @@ -243,12 +243,20 @@ static __always_inline void res_spin_unlock(rqspinlock_t *lock)
>>  	({                                        \
>>  		int __ret;                        \
>>  		local_irq_save(flags);            \
>> -		__ret = raw_res_spin_lock(lock);  \
>> -		if (__ret)                        \
>> +		preempt_disable();                \
>> +		__ret = res_spin_lock(lock);      \
>> +		if (__ret) {                      \
>>  			local_irq_restore(flags); \
>> +			preempt_enable();         \
>> +		}                                 \
>>  		__ret;                            \
>>  	})
>>
>> -#define raw_res_spin_unlock_irqrestore(lock, flags) ({ raw_res_spin_unlock(lock); local_irq_restore(flags); })
>> +#define raw_res_spin_unlock_irqrestore(lock, flags) \
>> +	({                                          \
>> +		res_spin_unlock(lock);              \
>> +		local_irq_restore(flags);           \
>> +		preempt_enable();                   \
>> +	})
>
> [Severity: High]
> This isn't a bug introduced by this patch, but while reviewing the resilient
> spinlock changes, I noticed a pre-existing issue in the locktorture module.
>
> Does locktorture correctly handle the return values from the resilient
> spinlock API?
>
> In kernel/locking/locktorture.c:torture_raw_res_spin_write_lock(), the
> return value is discarded:
>
>     raw_res_spin_lock(&rqspinlock);
>     return 0;
>
> Similarly, in torture_raw_res_spin_write_lock_irq():
>
>     raw_res_spin_lock_irqsave(&rqspinlock, flags);
>     cxt.cur_ops->flags = flags;
>     return 0;
>
> Since raw_res_spin_lock and raw_res_spin_lock_irqsave can fail and return
> an error code like -EDEADLK or -ETIMEDOUT, ignoring the return value
> allows the caller to proceed unlocked into the critical section.
>
> When the torture thread later calls raw_res_spin_unlock(), it will cause
> an unbalanced preempt_enable() because the failed lock acquisition path
> already restored preemption.
>
> Furthermore, it forcibly clears the lock state, which could unlock a lock
> legitimately held by another thread, potentially causing lock state
> corruption and panics.

In case of locktorture, error checking is skipped because it does not exercise
deadlocks, it's only for locking scalability and correctness checks.