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 for Android: free password hash cracker in your pocket

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

Message-ID: <d412bbf7-bfb7-42bc-4352-fd99d38121bd@intel.com>
Date:   Wed, 12 Apr 2023 18:49:01 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Jan Sokolowski <jan.sokolowski@...el.com>
CC:     <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: 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.

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

* 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