Skip to content

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

Open
kernel-patches-daemon-bpf[bot] wants to merge 1 commit into
bpf-next_basefrom
series/1109134=>bpf-next
Open

rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule#12427
kernel-patches-daemon-bpf[bot] wants to merge 1 commit into
bpf-next_basefrom
series/1109134=>bpf-next

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1109134

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 140fa23
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1109134
version: 1

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 3877437442143482e4619130b16a3c7814dc886d
Author: Gabriele Monaco <gmonaco@redhat.com>

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

This commit fixes the ordering in raw_res_spin_unlock_irqrestore() to
enable interrupts before enabling preemption, allowing the scheduler to
run at that point. The same fix is applied to the error path of
raw_res_spin_lock_irqsave().

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

> 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

In-Reply-To-Subject: rqspinlock: Fix order in raw_res_spin_(un)lock_irq to allow schedule
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/27265934925

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 2e8ad1f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1109134
version: 1

1 similar comment
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 2e8ad1f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1109134
version: 1

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: 101acd2 ("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>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 30dee2c
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1109134
version: 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant