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

[sashiko-bot@kernel.org]

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