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

["Kumar Kartikeya Dwivedi" <memxor@gmail.com>]

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.