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 for Android: free password hash cracker in your pocket

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

Message-ID: <1a89af34-8f7a-486b-a7f8-0a56d0447ce7@riseup.net>
Date: Thu, 27 Mar 2025 20:54:13 +0100
From: Fernando Fernandez Mancera <ffmancera@...eup.net>
To: Thomas Gleixner <tglx@...utronix.de>, 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 3/27/25 6:15 PM, Thomas Gleixner wrote:
> On Thu, Mar 27 2025 at 16:22, Fernando Fernandez Mancera wrote:
>> As the PIT could be disabled during the init, it can possibly cause a
>> deadlock. hpet_time_init()->pit_timer_init() is called without IRQ off.
>> It assumes that clockevent_i8253_disable() is IRQ-safe, which it isn't.
> 
> It assumes nothing and all the missing interrupt disable is causing is a
> lockdep false positive.
> 
> Lockdep complains correctly due to the observed contexts, but in reality
> there is no possible deadlock at all. Definitely not the one your
> subject line is claiming to be possible.
> 
> At the point where pit_timer_init() is invoked there is no other usage
> of 8253_lock possible because the system is still in the very early boot
> stage.
> 

Thanks for taking the time to review the patch.

I was not aware of this. I took a look to other functions that used 
i8253_lock like pcspkr_event() and thought it could be possible.

> So disabling interrupt here just prevents lockdep triggering a false
> positive and not more.
> 
> Please analyze problems properly instead of assuming that the lockdep
> splat is the ultimate truth.

I tried, but it seems I failed. I just found out this while working on 
something else not related in a VM.

> 
>>   bool __init pit_timer_init(void)
>>   {
>> +	unsigned long flags;
>> +
>>   	if (!use_pit()) {
>>   		/*
>>   		 * Don't just ignore the PIT. Ensure it's stopped, because
>>   		 * VMMs otherwise steal CPU time just to pointlessly waggle
>>   		 * the (masked) IRQ.
>>   		 */
>> +		local_irq_save(flags);
> 
> Why save()? You just established that interrupts are enabled here, so
> this really wants to be:
> 

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.

[1] https://lore.kernel.org/linux-kernel/Z-B6ob0zLZr81e8i@gmail.com/

>                  scoped_guard(irq)()
> 	              	clockevent_i8253_disable();
>    		return false;
> 
> Thanks,
> 
>          tglx