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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <522a1e9d-0453-447b-b541-86b76fa245bd@jacekk.info>
Date: Tue, 8 Jul 2025 21:40:12 +0200
From: Jacek Kowalski <jacek@...ekk.info>
To: Simon Horman <horms@...nel.org>
Cc: 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

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

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

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

-- 
Best regards,
   Jacek Kowalski