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

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

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)

quoted

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

|

quoted

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