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

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

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h

b/drivers/net/ethernet/freescale/enetc/enetc_hw.h

quoted

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)