netdev - RE: [Intel-wired-lan] [PATCH net v1] ice: fix undersized tx

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] [day] [month] [year] [list]

Message-ID: <SJ0PR11MB5815C2C1645B74378E59298A999C9@SJ0PR11MB5815.namprd11.prod.outlook.com>
Date:   Mon, 17 Apr 2023 10:16:45 +0000
From:   "Sokolowski, Jan" <jan.sokolowski@...el.com>
To:     "Lobakin, Aleksander" <aleksander.lobakin@...el.com>
CC:     "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [Intel-wired-lan] [PATCH net v1] ice: fix undersized tx_flags
 field

From: Lobakin, Aleksander <aleksander.lobakin@...el.com> 
Sent: Wednesday, April 12, 2023 6:49 PM
To: Sokolowski, Jan <jan.sokolowski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org
>Subject: Re: [Intel-wired-lan] [PATCH net v1] ice: fix undersized tx_flags field
>>From: Jan Sokolowski <jan.sokolowski@...el.com>
>>Date: Tue, 11 Apr 2023 09:37:07 +0200
>
>Please always add original authors to Ccs when you modify some code. I
>found this mail only by scrolling IWL, while I should've got it from the
>start.
>
>+ Cc netdev (no idea why you didn't do that)
>> As not all ICE_TX_FLAGS_* fit in current 16-bit limited
>> tx_flags field, some flags would not properly apply.
>
>Could you give more details here? With the actual definitions and also
>how it was found and what's the regression is.
>I found that there's VLAN tag which uses upper 16 bits only by browsing
>the code, while I'd say you should've written it here.

The definitions are ICE_TX_FLAGS_VLAN_* ones, for example ICE_TX_FLAGS_VLAN_M that is a
0xffff0000 mask.

The regression found was with some vlan traffic no longer passing through after commit 
aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers")

>> 
>> Fix that by removing 16 bit limitation.
>> 
>> Signed-off-by: Jan Sokolowski <jan.sokolowski@...el.com>
>> Fixes: aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers")
>
>Your SoB must go last, i.e. "Fixes:" should be placed above it.
>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
>> index fff0efe28373..46c108cc5283 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
>> @@ -183,7 +183,7 @@ struct ice_tx_buf {
>>  		unsigned int nr_frags;	/* used for mbuf XDP */
>>  	};
>>  	u32 type:16;			/* &ice_tx_buf_type */
>> -	u32 tx_flags:16;
>> +	u32 tx_flags;
>
>Please always provide pahole output when you change fields
>size/structure. Here you create a 16-bit hole and increase structure
>size with no mentioning.

Ok, will do that. 

>I wouldn't say the fix is optimal. From what I see, we have such flags
>(correct me if I'm wrong):
>
>TSO			BIT(0)
>[bits 1-7 are used]
>OUTER_SINGLE_VLAN	BIT(8)
>[bits 9-15 are UNused]
>VLAN_S (shift)		16
>[bits 16-31 are used for VLAN tag]
>
>So you have 7 free bits to reuse for &ice_tx_buf_type, but you just
>restored the before-commit ::tx_flags size =\
>I would do the following:
>
>	u32 tx_flags:12;
>	u32 type:4;
>	u32 vid:16;

Ok, will try to refactor it in this way and see whether it'll work.

>* no structure size change (even no layout change);
>* ::type range is 0-15 -- more than enough, as the last &ice_tx_buf_type
>  value is 6;
>* ::tx_flags still has 3 free bits left (9, 10, and 11);
>* ::vid makes it easier to set a VLAN tag (no explicit masking-shifting,
>  just don't forget to adjust the places where %ICE_TX_VLAN_{M,S} are
>  used).
>
>Don't just use "first that works" approach =\
>
>>  	DEFINE_DMA_UNMAP_LEN(len);
>>  	DEFINE_DMA_UNMAP_ADDR(dma);
>>  };
>
>Thanks,
>Olek