netdev - Re: [net PATCH 3/6] fbnic: Add additional handling of IRQs

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: <20250502135153.GL3339421@horms.kernel.org>
Date: Fri, 2 May 2025 14:51:53 +0100
From: Simon Horman <horms@...nel.org>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
	pabeni@...hat.com
Subject: Re: [net PATCH 3/6] fbnic: Add additional handling of IRQs

On Thu, May 01, 2025 at 04:30:10PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@...com>
> 
> We have two issues that need to be addressed in our IRQ handling.
> 
> One is the fact that we can end up double-freeing IRQs in the event of an
> exception handling error such as a PCIe reset/recovery that fails. To
> prevent that from becoming an issue we can use the msix_vector values to
> indicate that we have successfully requested/freed the IRQ by only setting
> or clearing them when we have completed the tiven action.

nit: given

> 
> The other issue is that we have several potential races in our IRQ path due
> to us manipulating the mask before the vector has been truly disabled. In
> order to handle that in the case of the FW mailbox we need to not
> auto-enable the IRQ and instead will be enabling/disabling it separately.
> In the case of the PCS vector we can mitigate this by unmapping it and
> synchronizing the IRQ before we clear the mask.
> 
> The general order of operations after this change is now to request the
> interrupt, poll the FW mailbox to ready, and then enable the interrupt. For
> the shutdown we do the reverse where we disable the interrupt, flush any
> pending Tx, and then free the IRQ. I am renaming the enable/disable to
> request/free to be equivilent with the IRQ calls being used. We may see
> additions in the future to enable/disable the IRQs versus request/free them
> for certain use cases.

Thanks for the nice explanation. And likewise throughout this series.
It's much appreciated.

> 
> Fixes: da3cde08209e ("eth: fbnic: Add FW communication mechanism")
> Fixes: 69684376eed5 ("eth: fbnic: Add link detection")
> Signed-off-by: Alexander Duyck <alexanderduyck@...com>

Reviewed-by: Simon Horman <horms@...nel.org>