netdev - Re: [Intel-wired-lan] [PATCH v3 iwl-net 1/8] ice: respect netif readiness in AF

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: <ZmsZWfn1ev6KWo3r@boxer>
Date: Thu, 13 Jun 2024 18:07:53 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <larysa.zaremba@...el.com>,
	<netdev@...r.kernel.org>, <michal.kubiak@...el.com>,
	<anthony.l.nguyen@...el.com>, <jacob.e.keller@...el.com>, Chandan Kumar Rout
	<chandanx.rout@...el.com>, <magnus.karlsson@...el.com>, Shannon Nelson
	<shannon.nelson@....com>
Subject: Re: [Intel-wired-lan] [PATCH v3 iwl-net 1/8] ice: respect netif
 readiness in AF_XDP ZC related ndo's

On Thu, Jun 13, 2024 at 05:51:25PM +0200, Maciej Fijalkowski wrote:
> On Wed, Jun 12, 2024 at 11:15:31AM +0200, Alexander Lobakin wrote:
> > From: Alexander Lobakin <aleksander.lobakin@...el.com>
> > Date: Wed, 12 Jun 2024 11:09:10 +0200
> > 
> > > From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> > > Date: Tue, 11 Jun 2024 16:21:27 +0200
> > 
> > [...]
> > 
> > >>>> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > >>>> index 2015f66b0cf9..1bd4b054dd80 100644
> > >>>> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > >>>> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > >>>> @@ -1048,6 +1048,10 @@ bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> > >>>>  
> > >>>>  	ice_clean_xdp_irq_zc(xdp_ring);
> > >>>>  
> > >>>> +	if (!netif_carrier_ok(xdp_ring->vsi->netdev) ||
> > >>>> +	    !netif_running(xdp_ring->vsi->netdev))
> > 
> > Oh BTW, I noticed some time ago that netif_running() is less precise
> > than checking for %IFF_UP.
> > For example, in this piece (main netdev ifup function)[0],
> > netif_running() will start returning true *before* driver's .ndo_open()
> > is called, but %IFF_UP will be set only after .ndo_open() is done (with
> > no issues).
> 
> I see, thanks for bringing this up! I'd like to try this out. Tony sorry
> for the noise, but it seems I'll go with v4 and will decorate the
> mentioned statements with unlikely().
> 
> > That means, I'd check for %IFF_UP honestly (maybe even before checking
> > the carrier).
> 
> I wonder whether it is the ultimate check and two existing ones (that we
> are adding in this patch) could be dropped?

In netdev closing path [1], __LINK_STATE_START is cleared before IFF_UP.
What we were initially experiencing when netif_running() check wasn't in
place was that xsk was producing a bunch of Tx descs when device was being
brought down. So let me keep what we have here and add IFF_UP check on
top. Better be safe than sorry as the bug we were dealing with was pretty
nasty.

> 
> > 
> > [0] https://elixir.bootlin.com/linux/v6.10-rc3/source/net/core/dev.c#L1466

[1]: https://elixir.bootlin.com/linux/v6.10-rc3/source/net/core/dev.c#L1532

> > 
> > Thanks,
> > Olek