linux-kernel - Re: [PATCH v2] x86/i8253: fix possible deadlock when turning off the PIT

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: <878qoqxjew.ffs@tglx>
Date: Thu, 27 Mar 2025 22:17:43 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Fernando Fernandez Mancera <ffmancera@...eup.net>, x86@...nel.org,
 linux-kernel@...r.kernel.org
Cc: dwmw@...zon.co.uk, mhkelley@...look.com, mingo@...nel.org
Subject: Re: [PATCH v2] x86/i8253: fix possible deadlock when turning off
 the PIT

On Thu, Mar 27 2025 at 20:54, Fernando Fernandez Mancera wrote:
> On 3/27/25 6:15 PM, Thomas Gleixner wrote:
> I followed Ingo's suggestions on V1 [1]. It made sense to me, if the 
> problem was the one described on the commit message. So, is there 
> consensus about this being a false positive? If so, I will send a new 
> patch just suppressing the warning as suggested below.

I personally don't care whether there is consensus simply because it's a
matter of fact, that at the point where pit_timer_init() is invoked there
can't be concurrency on the lock by any means. Therefore it _is_ a false
positive.

Ingo is right that pit_timer_init() should disable interrupts before
invoking clockevent_i8253_disable() and not inflicting the irqsave() on
the callback function.

But it should do so for the sake of consistency and correctness and not
to "fix" a impossible deadlock or an magically assumed invalid assumption.

The assumption,

    - assumed that the author of the offending commit made
      any assumptions at all (pun intended) -

that invoking clockevent_i8253_disable() with interrupts enabled at this
point in the boot process is harmless, is completely correct.

Therefore I really prefer to have this described as:

  x86/i8253: Invoke clockevent_i8253_disable() with interrupts disabled

with a proper explanation that the current code makes lockdep
(rightfully) complain, but that it has no actual deadlock potential in
the current state of the code.

That means the code change serves two purposes:

   1) Prevent lockdep from detecting a false positive

   2) Future proving the code

#1 is a matter of fact with the current code

#2 is valuable despite the fact that PIT is a legacy, which won't
   suddenly roar its ugly head in unexpected ways.

I know that's word smithing, but I'm observing a increasing tendency of
"fixing" problems based on tooling output without any further analysis.

I'm absolutely not blaming you for that and your patch is fine, except
for the technical details I pointed out and the change log related
issues.

Though I really want people to sit down and think about the factual
impact of a tool based problem observation. Tools are good in detecting
problems, but they are patently bad in properly analysing them. And no,
AI is not going to fix that anytime soon, quite the contrary.

Taking the tools output at face value leads exactly to what triggered my
response:

  "fix possible deadlock when turning off the PIT"

which is misleading at best as I explained before.

Wording matters, but maybe that's just me...

Thanks,

        tglx