netdev - Re: [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250714132114.70feff08@pumpkin>
Date: Mon, 14 Jul 2025 13:21:14 +0100
From: David Laight <david.laight.linux@...il.com>
To: Jacek Kowalski <jacek@...ekk.info>
Cc: Simon Horman <horms@...nel.org>, Tony Nguyen
 <anthony.l.nguyen@...el.com>, Przemek Kitszel
 <przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org
Subject: Re: [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts
 to u16

On Tue, 8 Jul 2025 21:40:12 +0200
Jacek Kowalski <jacek@...ekk.info> wrote:

> >> -		if ((old_vid != (u16)E1000_MNG_VLAN_NONE) &&
> >> +		if ((old_vid != E1000_MNG_VLAN_NONE) &&  
> >
> > Ditto.
> >
> > But more importantly, both Clang 20.1.7 W=1 builds (or at any rate,
> > builds with -Wtautological-constant-out-of-range-compare), and Smatch
> > complain that the comparison above is now always true because
> > E1000_MNG_VLAN_NONE is -1, while old_vid is unsigned.

I'm guessing 'old_vid' is actually u16 (or the compiler knows the
value came from a u16).

In either case the compiler can 'know' that the condition is always
true - but a 'u16 old_vid' is promoted to 'signed int' prior to
the compare with -1, whereas if a 'u32 old_vid' is known to contain
a 16bit value it is the -1 that is converted to ~0u.

> 
> You are right - I have missed that E1000_MNG_VLAN_NONE is negative.
> Therefore (u16)E1000_MNG_VLAN_NONE has a side effect of causing a 
> wraparound.
> 
> It's even more interesting that (inadvertently) I have not made a 
> similar change in e1000e:
> 
> ./drivers/net/ethernet/intel/e1000e/netdev.c:
> if (adapter->mng_vlan_id != (u16)E1000_MNG_VLAN_NONE) {
> 
> 
> > Perhaps E1000_MNG_VLAN_NONE should be updated to be UINT16_MAX?  
> 
> There's no UINT16_MAX in kernel as far as I know. I'd rather leave it as 
> it was or, if you insist on further refactoring, use either one of:
> 
> #define E1000_MNG_VLAN_NONE (u16)(~((u16) 0))
> mimick ACPI: #define ACPI_UINT16_MAX                 (u16)(~((u16) 0))
> 
> #define E1000_MNG_VLAN_NONE ((u16)-1)
> move the cast into the constant
> 
> #define E1000_MNG_VLAN_NONE 0xFFFF
> use ready-made value

Possibly better is 0xFFFFu.
Then 'u16 old_val' is first promoted to 'signed int' and then implicitly
cast to 'unsigned int' prior to the comparison.

Remember C does all maths as [un]signed int (except for types bigger than int).
Local variables, function parameters and function results should really
be [un]signed int provided the domain of value doesn't exceed that of int.
Otherwise the compile is forced to generate explicit instructions to mask
the result of arithmetic operations to the smaller type.

	David 

> 
> (parentheses left only due to the constant being "(-1)" and not "-1").
>