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(-)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-03-29 16:25:05
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.
quoted hunk
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(-)
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"?
-----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 [off-list ref]
Subject: Re: [PATCH net v2] enetc: Avoid implicit sign extension
On Mon, Mar 29, 2021 at 05:14:43PM +0300, Claudiu Manoil wrote:
quoted
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.
quoted
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(-)
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: David Laight <hidden> Date: 2021-03-30 08:41:47
From: Vladimir Oltean
Sent: 29 March 2021 17:24
On Mon, Mar 29, 2021 at 05:14:43PM +0300, Claudiu Manoil wrote:
quoted
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.
quoted
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(-)
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)