netdev - Re: [PATCH net-next 12/13] ice: implement low latency PHY timer updates

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: <961f010f-4c53-4bb6-a625-289b6a52525a@intel.com>
Date: Mon, 13 Jan 2025 10:50:38 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>
CC: <davem@...emloft.net>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<andrew+netdev@...n.ch>, <netdev@...r.kernel.org>,
	<anton.nadezhdin@...el.com>, <przemyslaw.kitszel@...el.com>,
	<milena.olech@...el.com>, <arkadiusz.kubalewski@...el.com>,
	<richardcochran@...il.com>, Karol Kolacinski <karol.kolacinski@...el.com>,
	Rinitha S <sx.rinitha@...el.com>
Subject: Re: [PATCH net-next 12/13] ice: implement low latency PHY timer
 updates

On 1/9/2025 6:18 PM, Jakub Kicinski wrote:
> On Wed,  8 Jan 2025 14:17:49 -0800 Tony Nguyen wrote:
>> +	spin_lock_irqsave(&params->atqbal_wq.lock, flags);
>> +
>> +	/* Wait for any pending in-progress low latency interrupt */
>> +	err = wait_event_interruptible_locked_irq(params->atqbal_wq,
> 
> Don't you need an irqsave() flavor of
> wait_event_interruptible_locked_irq() for this to work correctly? 🤔️

My understanding was that spin_lock_irqsave saves the IRQ state, where
as spin_lock_irq doesn't save the state and assumes the interrupts
should always be enabled.

In this case, we lock with irqsave, keeping track of the interrupt state
before, then wait_event_interruptible_locked_irq would enable interrupts
when it unlocks to sleep.. Hm

So this code will correctly restore the interrupt state at the end after
we call spin_unlock_irqrestore, but there is a window within the
wait_event_interruptible_locked_irq where interrupts will be enabled
when they potentially shouldn't be..

Oops