| Message ID | 20210329141443.23245-1-claudiu.manoil@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] enetc: Avoid implicit sign extension | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3 this patch: 3 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | fail | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 10 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3 this patch: 3 |
| netdev/header_inline | success | Link |
On Mon, Mar 29, 2021 at 05:14:43PM +0300, Claudiu Manoil wrote: > Static analysis tool reports: > "Suspicious implicit sign extension - 'flags' with type u8 (8 bit, > unsigned) is promoted in 'flags' << 24 to type int (32 bits, signed), > then sign-extended to type unsigned long long (64 bits, unsigned). > If flags << 24 is greater than 0x7FFFFFFF, the upper bits of the result This is a backwards way of saying 'if flags & BIT(7) is set', no? But BIT(7) is ENETC_TXBD_FLAGS_F (the 'final BD' bit), and I've been testing SO_TXTIME with single BD frames, and haven't seen this problem. > will all be 1." > > Use lower_32_bits() to avoid this scenario. > > Fixes: 82728b91f124 ("enetc: Remove Tx checksumming offload code") > > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> > --- > v2 - added 'fixes' tag > > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h > index 00938f7960a4..07e03df8af94 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h > +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h > @@ -535,8 +535,8 @@ static inline __le32 enetc_txbd_set_tx_start(u64 tx_start, u8 flags) > { > u32 temp; > > - temp = (tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) | > - (flags << ENETC_TXBD_FLAGS_OFFSET); > + temp = lower_32_bits(tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) | > + (u32)(flags << ENETC_TXBD_FLAGS_OFFSET); I don't actually understand why lower_32_bits called on the TX time helps, considering that the value is masked already. The static analysis tool says that the right hand side of the "|" operator is what is sign-extended: (flags << ENETC_TXBD_FLAGS_OFFSET); Isn't it sufficient that you replace "u8 flags" in the function prototype with "u32 flags"? > > return cpu_to_le32(temp); > } > -- > 2.25.1 >
>-----Original Message----- >From: Vladimir Oltean <vladimir.oltean@nxp.com> >Sent: Monday, March 29, 2021 7:24 PM >To: Claudiu Manoil <claudiu.manoil@nxp.com> >Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>; David S . >Miller <davem@davemloft.net> >Subject: Re: [PATCH net v2] enetc: Avoid implicit sign extension > >On Mon, Mar 29, 2021 at 05:14:43PM +0300, Claudiu Manoil wrote: >> Static analysis tool reports: >> "Suspicious implicit sign extension - 'flags' with type u8 (8 bit, >> unsigned) is promoted in 'flags' << 24 to type int (32 bits, signed), >> then sign-extended to type unsigned long long (64 bits, unsigned). >> If flags << 24 is greater than 0x7FFFFFFF, the upper bits of the result > >This is a backwards way of saying 'if flags & BIT(7) is set', no? But >BIT(7) is ENETC_TXBD_FLAGS_F (the 'final BD' bit), and I've been testing >SO_TXTIME with single BD frames, and haven't seen this problem. > Better be safe than sorry. >> will all be 1." >> >> Use lower_32_bits() to avoid this scenario. >> >> Fixes: 82728b91f124 ("enetc: Remove Tx checksumming offload code") >> >> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> >> --- >> v2 - added 'fixes' tag >> >> drivers/net/ethernet/freescale/enetc/enetc_hw.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h >b/drivers/net/ethernet/freescale/enetc/enetc_hw.h >> index 00938f7960a4..07e03df8af94 100644 >> --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h >> +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h >> @@ -535,8 +535,8 @@ static inline __le32 enetc_txbd_set_tx_start(u64 >tx_start, u8 flags) >> { >> u32 temp; >> >> - temp = (tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) | >> - (flags << ENETC_TXBD_FLAGS_OFFSET); >> + temp = lower_32_bits(tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) >| >> + (u32)(flags << ENETC_TXBD_FLAGS_OFFSET); > >I don't actually understand why lower_32_bits called on the TX time >helps, considering that the value is masked already. Just want to ensure it's handled as u32 and not u64. I also think lower_32_bits() is the cleanest way to convert from u64 to u32, in this case at least. >The static analysis >tool says that the right hand side of the "|" operator is what is >sign-extended: > > (flags << ENETC_TXBD_FLAGS_OFFSET); > >Isn't it sufficient that you replace "u8 flags" in the function >prototype with "u32 flags"? > I prefer to cast it to u32 after the shift. The 'flags' argument passed to this helper function is always u8 as it matches the 8-bit field of the Tx BD DMA structure.
From: Vladimir Oltean > Sent: 29 March 2021 17:24 > > On Mon, Mar 29, 2021 at 05:14:43PM +0300, Claudiu Manoil wrote: > > Static analysis tool reports: > > "Suspicious implicit sign extension - 'flags' with type u8 (8 bit, > > unsigned) is promoted in 'flags' << 24 to type int (32 bits, signed), > > then sign-extended to type unsigned long long (64 bits, unsigned). > > If flags << 24 is greater than 0x7FFFFFFF, the upper bits of the result > > This is a backwards way of saying 'if flags & BIT(7) is set', no? But > BIT(7) is ENETC_TXBD_FLAGS_F (the 'final BD' bit), and I've been testing > SO_TXTIME with single BD frames, and haven't seen this problem. > > > will all be 1." > > > > Use lower_32_bits() to avoid this scenario. > > > > Fixes: 82728b91f124 ("enetc: Remove Tx checksumming offload code") > > > > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > --- > > v2 - added 'fixes' tag > > > > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h > b/drivers/net/ethernet/freescale/enetc/enetc_hw.h > > index 00938f7960a4..07e03df8af94 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h > > +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h > > @@ -535,8 +535,8 @@ static inline __le32 enetc_txbd_set_tx_start(u64 tx_start, u8 flags) > > { > > u32 temp; > > > > - temp = (tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) | > > - (flags << ENETC_TXBD_FLAGS_OFFSET); > > + temp = lower_32_bits(tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) | > > + (u32)(flags << ENETC_TXBD_FLAGS_OFFSET); > > I don't actually understand why lower_32_bits called on the TX time > helps, considering that the value is masked already. Not only that the high bits get thrown away by the assignment. The change just gives the reader more to parse for zero benefit. > The static analysis > tool says that the right hand side of the "|" operator is what is > sign-extended: > > (flags << ENETC_TXBD_FLAGS_OFFSET); > > Isn't it sufficient that you replace "u8 flags" in the function > prototype with "u32 flags"? That would be much better. It may save the value having to be masked with 0xff as well. Regardless of the domain of function parameters/results (and local variables) using machine-register sized types will typically give better code. x86 is probably unique in having sub-32bit arithmetic. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h index 00938f7960a4..07e03df8af94 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h @@ -535,8 +535,8 @@ static inline __le32 enetc_txbd_set_tx_start(u64 tx_start, u8 flags) { u32 temp; - temp = (tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) | - (flags << ENETC_TXBD_FLAGS_OFFSET); + temp = lower_32_bits(tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) | + (u32)(flags << ENETC_TXBD_FLAGS_OFFSET); return cpu_to_le32(temp); }
Static analysis tool reports: "Suspicious implicit sign extension - 'flags' with type u8 (8 bit, unsigned) is promoted in 'flags' << 24 to type int (32 bits, signed), then sign-extended to type unsigned long long (64 bits, unsigned). If flags << 24 is greater than 0x7FFFFFFF, the upper bits of the result will all be 1." Use lower_32_bits() to avoid this scenario. Fixes: 82728b91f124 ("enetc: Remove Tx checksumming offload code") Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> --- v2 - added 'fixes' tag drivers/net/ethernet/freescale/enetc/enetc_hw.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)