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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CADEbmW3As4t9LbZqvjKe0CyWQkYMOVKMzQgtmJdcqkQbyayP1w@mail.gmail.com>
Date: Mon, 13 Jan 2025 21:46:50 +0100
From: Michal Schmidt <mschmidt@...hat.com>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>, 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 Mon, Jan 13, 2025 at 7:51 PM Jacob Keller <jacob.e.keller@...el.com> wrote:
> 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

Do you even need spin_lock_irqsave() here? It seems to me that all the
functions where you're adding the
wait_event_interruptible_locked_irq() calls are always entered with
interrupts enabled, so it should be safe to just use spin_lock_irq().

Michal

> 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..