netdev - Re: [RFC PATCH 2/2] net: convert to nla_get_*

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: <4c9b5027a70f7d9039ffb5579a1e7a08c7fa5ddb.camel@sipsolutions.net>
Date: Thu, 24 Oct 2024 17:29:44 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] net: convert to nla_get_*_default()

On Thu, 2024-10-24 at 17:17 +0200, Sabrina Dubroca wrote:
> 2024-10-24, 16:52:00 +0200, Johannes Berg wrote:
> > On Thu, 2024-10-24 at 16:48 +0200, Sabrina Dubroca wrote:
> > > 
> > > If nla_get_*_default was a macro (generating an "attr ? getter :
> > > default" expression) you wouldn't have that problem I think, 
> > 
> > Hmm. Perhaps. In the conditional operator (?:) they're subject to
> > integer promotion though
> 
> Is it?
> 
>     #define nla_get_u16_default(attr, d) (attr ? nla_get_u16(attr) : d)
>     int v = nla_get_u16_default(NULL, -1);
> 
> seems to put the correct value into v.
> (but -ENOFOOD and -ELOWCOFFEE here, so I don't trust this quick test
> much :))

I'm probably -ELOWBLOODSUGAR right now ;-)

But yeah, I couldn't yet construct a scenario where it mattered, but I'
also couldn't convince myself that there isn't one. Maybe too much
inspired by the enum compare warnings:

https://lore.kernel.org/linux-wireless/20241018151841.3821671-1-arnd@kernel.org/


> > , I wonder if that could cause some subtle issue
> > too especially if nla_get_u*() is used with signed variables?
> 
> The issue in that example is pretty subtle and I'm fairly sure people
> are going to mess up :/

I didn't think it was that bad, but it's well possible that my
calibration for "subtle" is way off ;-)

> But I'm not attached to that macro I just suggested, it's just a
> thought.

Sure.

> > > but you
> > > couldn't nicely generate all the helpers with MAKE_NLA_GET_DEFAULT
> > > anymore.
> > 
> > Right, that too.
> > 
> > I think it's probably better to just review them, and only commit the
> > obvious ones originally?
> 
> Well, this one looked reasonable too. I'm not convinced reviewers are
> going to catch those problems. Or authors of new code using those
> _default helpers from the start.

Fair point. I didn't think it was that bad, in fact there were some I
was far less sure about, say

> +	request->page = nla_get_u8_default(info->attrs[NL802154_ATTR_PAGE],
> +					   wpan_phy->current_page);

which really depends on the types of 'page' and 'current_page'...


I think for most cases it's probably still worth doing, and I wouldn't
be _too_ concerned about the type issues here, most places are just
using zero or a small constant as the default anyway.

Even nla_get_XX_or_zero() would be a win, but that seemed too special
...

johannes