[PATCH] wifi: nl80211: drop impossible negative band check

[PATCH] wifi: nl80211: drop impossible negative band check

[Sun Jian]

band is derived from nla_type() of a nested netlink attribute, which is
a masked u16 value and therefore cannot be negative. Drop the dead
"band < 0" checks and keep the upper bound validation.

No functional change intended.

Signed-off-by: Sun Jian <sun.jian.kdev@gmail.com>
---
 net/wireless/nl80211.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 03efd45c007f..a92b4e24b28b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5759,7 +5759,7 @@ static int nl80211_parse_tx_bitrate_mask(struct genl_info *info,
 		enum nl80211_band band = nla_type(tx_rates);
 		int err;
 
-		if (band < 0 || band >= NUM_NL80211_BANDS)
+		if (band >= NUM_NL80211_BANDS)
 			return -EINVAL;
 		sband = rdev->wiphy.bands[band];
 		if (sband == NULL)
@@ -10536,7 +10536,7 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 				    tmp) {
 			enum nl80211_band band = nla_type(attr);
 
-			if (band < 0 || band >= NUM_NL80211_BANDS) {
+			if (band >= NUM_NL80211_BANDS) {
 				err = -EINVAL;
 				goto out_free;
 			}
-- 
2.43.0

Re: [PATCH] wifi: nl80211: drop impossible negative band check

[Johannes Berg]

On Wed, 2026-02-04 at 16:18 +0800, Sun Jian wrote:
> band is derived from nla_type() of a nested netlink attribute, which is
> a masked u16 value and therefore cannot be negative. Drop the dead
> "band < 0" checks and keep the upper bound validation.

I've seen this before, but I'm not really convinced it is entirely
correct. C says:

   All enumerations have an underlying type. The underlying type can be
   explicitly specified using an enum type specifier and is its fixed
   underlying type. If it is not explicitly specified, the underlying
   type is the enumeration’s compatible type, which is either char or a
   standard or extended signed or unsigned integer type.

It would thus _seem_ to be possible for an enum to generally be a signed
type, and therefore a 'signed short', and therefore an nla_type() that's
a u16 could end up with a negative value...

Am I wrong?

johannes

Re: [PATCH] wifi: nl80211: drop impossible negative band check

[sun jian]

On Wed, Feb 4, 2026 at 4:36 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Wed, 2026-02-04 at 16:18 +0800, Sun Jian wrote:
> > band is derived from nla_type() of a nested netlink attribute, which isAgreed — in general the enum underlying type can be signed.
> > a masked u16 value and therefore cannot be negative. Drop the dead
> > "band < 0" checks and keep the upper bound validation.
>
> I've seen this before, but I'm not really convinced it is entirely
> correct. C says:
>
>    All enumerations have an underlying type. The underlying type can be
>    explicitly specified using an enum type specifier and is its fixed
>    underlying type. If it is not explicitly specified, the underlying
>    type is the enumeration’s compatible type, which is either char or a
>    standard or extended signed or unsigned integer type.
>

Agreed — in general the enum underlying type can be signed.

> It would thus _seem_ to be possible for an enum to generally be a signed
> type, and therefore a 'signed short', and therefore an nla_type() that's
> a u16 could end up with a negative value...

The key detail here is that band isn't assigned the raw __u16
nla->nla_type, but nla_type().

And nla_type() is effectively:
nla->nla_type & NLA_TYPE_MASK

and NLA_TYPE_MASK clears the two high flag bits:
NLA_F_NESTED (1 << 15)
NLA_F_NET_BYTEORDER (1 << 14)

So the result is restricted to the low 14 bits, i.e. 0..0x3fff.

With that restriction, even if enum nl80211_band ended up with a signed
16-bit underlying type, the sign bit (bit 15) can never be set by
nla_type(), so the value cannot become negative.

>
> Am I wrong?

I think the "enum may be signed" concern is valid in general, but for
this particular assignment the masking guarantees the value is always in
a non-negative range.

Thanks,
Sun Jian

Re: [PATCH] wifi: nl80211: drop impossible negative band check

[Johannes Berg]

On Wed, 2026-02-04 at 17:13 +0800, sun jian wrote:
> On Wed, Feb 4, 2026 at 4:36 PM Johannes Berg <johannes@sipsolutions.net> wrote:
> > 
> > On Wed, 2026-02-04 at 16:18 +0800, Sun Jian wrote:
> > > band is derived from nla_type() of a nested netlink attribute, which isAgreed — in general the enum underlying type can be signed.
> > > a masked u16 value and therefore cannot be negative. Drop the dead
> > > "band < 0" checks and keep the upper bound validation.
> > 
> > I've seen this before, but I'm not really convinced it is entirely
> > correct. C says:
> > 
> >    All enumerations have an underlying type. The underlying type can be
> >    explicitly specified using an enum type specifier and is its fixed
> >    underlying type. If it is not explicitly specified, the underlying
> >    type is the enumeration’s compatible type, which is either char or a
> >    standard or extended signed or unsigned integer type.
> > 
> 
> Agreed — in general the enum underlying type can be signed.

But nothing says it cannot be "signed char".

> > It would thus _seem_ to be possible for an enum to generally be a signed
> > type, and therefore a 'signed short', and therefore an nla_type() that's
> > a u16 could end up with a negative value...

I was just using 'signed short' as an example, but your argument:

> The key detail here is that band isn't assigned the raw __u16
> nla->nla_type, but nla_type().
> 
> And nla_type() is effectively:
> nla->nla_type & NLA_TYPE_MASK
> 
> and NLA_TYPE_MASK clears the two high flag bits:
> NLA_F_NESTED (1 << 15)
> NLA_F_NET_BYTEORDER (1 << 14)
> 
> So the result is restricted to the low 14 bits, i.e. 0..0x3fff.
> 
> With that restriction, even if enum nl80211_band ended up with a signed
> 16-bit underlying type, the sign bit (bit 15) can never be set by
> nla_type(), so the value cannot become negative.

applies _only_ to signed short, not to signed char?

Now we can argue a "sane compiler" won't do that, and we can also argue
that "gcc and clang are sane compilers", although sometimes I definitely
have doubts about the latter ;-)

johannes

Re: [PATCH] wifi: nl80211: drop impossible negative band check

[sun jian]

>
> applies _only_ to signed short, not to signed char?
Fair point, thanks. I'll drop this patch.

Regards,

Sun Jian

Re: [PATCH] wifi: nl80211: drop impossible negative band check

[Johannes Berg]

On Wed, 2026-02-04 at 19:21 +0800, sun jian wrote:
> > 
> > applies _only_ to signed short, not to signed char?
> Fair point, thanks. I'll drop this patch.

I've thought the better way to address that warning would be to simply
use 'int' instead of the enum there, but I forgot where the warning even
appears. I don't think it's generally with gcc/clang, is it?

johannes

Re: [PATCH] wifi: nl80211: drop impossible negative band check

[sun jian]

On Wed, Feb 4, 2026 at 7:29 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Wed, 2026-02-04 at 19:21 +0800, sun jian wrote:
> > >
> > > applies _only_ to signed short, not to signed char?
> > Fair point, thanks. I'll drop this patch.
>
> I've thought the better way to address that warning would be to simply
> use 'int' instead of the enum there, but I forgot where the warning even
> appears. I don't think it's generally with gcc/clang, is it?
>

Right, I only saw it from sparse, not from gcc/clang.

It was reported for net/wireless/nl80211.c at the checks around lines
~5762 and ~10539 in my tree (both are "band < 0 || band >=
NUM_NL80211_BANDS" with band coming from nla_type()).

Thanks,
Sun Jian

Re: [PATCH] wifi: nl80211: drop impossible negative band check

[Jeff Johnson]

On 2/4/2026 3:33 AM, sun jian wrote:
> On Wed, Feb 4, 2026 at 7:29 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>>
>> On Wed, 2026-02-04 at 19:21 +0800, sun jian wrote:
>>>>
>>>> applies _only_ to signed short, not to signed char?
>>> Fair point, thanks. I'll drop this patch.
>>
>> I've thought the better way to address that warning would be to simply
>> use 'int' instead of the enum there, but I forgot where the warning even
>> appears. I don't think it's generally with gcc/clang, is it?
>>
> 
> Right, I only saw it from sparse, not from gcc/clang.
> 
> It was reported for net/wireless/nl80211.c at the checks around lines
> ~5762 and ~10539 in my tree (both are "band < 0 || band >=
> NUM_NL80211_BANDS" with band coming from nla_type()).

For future reference, if you are fixing a warning, then your commit text
should include that information. That way someone else who has that warning
can search the archives for that warning and then find your fix.

/jeff

Re: [PATCH] wifi: nl80211: drop impossible negative band check

[sun jian]

On Thu, Feb 5, 2026 at 12:20 AM Jeff Johnson
<jeff.johnson@oss.qualcomm.com> wrote:
>
> For future reference, if you are fixing a warning, then your commit text
> should include that information. That way someone else who has that warning
> can search the archives for that warning and then find your fix.
>
Ack, thanks, I will include the exact warning text in future
warning-fix commits.

sun jian