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: <3092c889-f112-4a20-bc7f-d703c162499c@intel.com>
Date: Fri, 14 Jun 2024 11:34:53 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...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

From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Date: Thu, 13 Jun 2024 18:07:53 +0200

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

Oh man, inconsistency in its best :D

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

Sure, I didn't know %IFF_UP's not that reliable :s

Thanks,
Olek