linux-kernel - Re: [PATCH] clocksource: disable irq when holding watchdog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231020090245.cPAViz09@linutronix.de>
Date:   Fri, 20 Oct 2023 11:02:45 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     John Stultz <jstultz@...gle.com>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Stephen Boyd <sboyd@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "Paul E. McKenney" <paulmck@...nel.org>, x86@...nel.org
Subject: Re: [PATCH] clocksource: disable irq when holding watchdog_lock.

On 2023-10-16 23:47:55 [+0200], Thomas Gleixner wrote:
>   But the explicit local_irq_disable()/enable() pair around the readout
>   means that the patch above is obviously broken:
> 
>     spin_lock_irqsave(&watchdog_lock, flags);
>     cs_watchdog_read()
>       local_irq_disable();
>       ...
>       local_irq_enable();  <--- FAIL
> 
>   Lockdep should have yelled at that too, but even if it failed to do
>   so then it's still very obviously wrong.

Lockdep will shout from local_irq_enable() if the lock is still held.
That is working and I just verified it. However I didn't get to that
part in cs_watchdog_read() in my testing so that might be why lockdep
didn't complain in Tetsuo's case. After adding a irq-enable before the
unlock for lockdep-testing I did see the splat.

> Something like the uncompiled/untested below should cure it for real. It
> really does not matter whether the TSC unstable event happens a bit
> later. The system is unhappy no matter what.
The snippet below is perfect :)

> That said, this whole clocksource watchdog mess wants a proper
> overhaul. It has become a pile of warts and duct tape by now and after
> staring at it long enough there is no real reason to run it in a timer
> callback anymore. It just can move into delayed work and the whole
> locking problem can be reduced to the clocksource_mutex and some well
> thought out atomic operations to handle the mark unstable case. But
> that's a different story and not relevant for curing the problem at
> hand.

A pile of warts. Okay. This explains why the lock is acquired everywhere
with irqsave while in-IRQ access is not desired.

> Thanks,
> 
>         tglx

Sebastian