Re: [PATCH] locking: Revert switching guards to _irq_{disable,enable}()

[Boqun Feng <boqun@kernel.org>]

On Wed, Aug 26, 2026 at 12:59:25AM +0200, Thomas Gleixner wrote:
> On Mon, Aug 24 2026 at 18:33, Boqun Feng wrote:
> > On Mon, Aug 24, 2026 at 12:55:23PM +0200, Peter Zijlstra wrote:
> >> 
> >> While the guards are properly nested, not all wrapped code is nice, as already
> >> highlighted by that fair.c hunk.
> >> 
> >> Syzbot found another instance of this pattern in posix_timer_delete(), which
> >> does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
> >> Combined with this patch, that goes sideways most spectacular.
> >> 
> >> Undo this change, until we've developed stronger tools / debug for such issues.
> >> 
> >
> > Mainly hand-waving, but if we make _irq(), irqsave(), _disable()
> > __acquires() different contexts, we may be able to catch these issues at
> > compile time. I will explore a bit on this.
> 
> No.
> 
> Just do a wholesale conversion of all functions which affect the CPU
> interrupt disabled state directly (local_irq_*) and indirectly (locking
> functions etc.)
> 
> Anything else is just a whack a mole game.
> 

Alright. But I'm afraid that's just another type of whack-a-mole games.

As I mentioned here [1], we are a few unpaired local_irq_disable() +
local_irq_enable(), we can spend time to clean them up, but no guarantee
people will not introduce more, plus we have code that does
spin_lock_irqsave(); spin_unlock_irq(); spin_lock_irq();
spin_unlock_irqrestore(); and expect it works. A more reasonable
approach to me is introducing the new API and fixing the problematic
usage one-by-one and then when we are certain about only a few cases
left, we do a flag day change.

Trying to do it (new API and whole conversion) in one go is easier
said than done. Of course I might miss something subtle here, looking
forwards to your suggestion.

> TBH, I do not understand why you thought that you can get away with this
> lazy approach especially after you discovered the same nasty problem in
> do_sched_cfs_period_timer(). The resolution of that got buried in
> 
>   1b0866874833 ("locking: Switch to _irq_{disable,enable}() variants in cleanup guards")
> 
> without even being mentioned.
> 

I have this in the commit log:

[boqun: Adjust the user-side changes in do_sched_cfs_*_timer() provided
    by Peter and Lyude]

but sure, I should have done a better job mentioning it.

A bit more context of switching the guard implementation: I wanted to
have some test/usage coverage other than Rust for the new API, and since
the guard() API is relatively new, so I thought people will not use it
"creatively" (but obviously I was wrong). Hence I add the conversation
for the guard APIs only. It is not a lazy approach IMO, but rather a way
to test how the new API works. Of course, a bug is a bug, I don't have
any excuse on that.

> When I was discussing the non-sensical syzbot messages earlier today
> with Peter it immediately occurred to me that this undocumented change in
> do_sched_cfs_period_timer() is not the only pattern which causes this to
> go belly up. It took me five seconds to find the posix timer one.
> 
> TBH, my hope really was that the RUST people take the only valid
> engineering principle "Correctness first" serious, but sadly they seem
> to be the same lazy sods than everyone else who want to push their
> agenda through no matter what.
> 

There seems some misunderstandings here. The only "lazy" part is we
defer the whole conversion because of the problems I mentioned above,
and that is because Correctness is valued.

[1]: https://lore.kernel.org/rust-for-linux/aPHlySQJQpDmgHAm@tardis.local/

Regards,
Boqun

> Thanks,
> 
>         tglx