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
| ||
|
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
Powered by blists - more mailing lists