[PATCH net v2] enetc: Avoid implicit sign extension

[PATCH net v2] enetc: Avoid implicit sign extension

[Claudiu Manoil]

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(-)

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);
 }
-- 
2.25.1

Re: [PATCH net v2] enetc: Avoid implicit sign extension

[Vladimir Oltean]

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
> 

RE: [PATCH net v2] enetc: Avoid implicit sign extension

[Claudiu Manoil]

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

RE: [PATCH net v2] enetc: Avoid implicit sign extension

[David Laight]

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)