linux-kernel - Re: [PATCH 00/21] lib: add alternatives for GENMASK()

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] [day] [month] [year] [list]

Message-ID: <20251126103832.6581bbc6@pumpkin>
Date: Wed, 26 Nov 2025 10:38:32 +0000
From: david laight <david.laight@...box.com>
To: Rasmus Villemoes <ravi@...vas.dk>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, "Yury Norov (NVIDIA)"
 <yury.norov@...il.com>, Linus Walleij <linus.walleij@...aro.org>, Nicolas
 Frattaroli <nicolas.frattaroli@...labora.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/21] lib: add alternatives for GENMASK()

On Wed, 26 Nov 2025 10:07:34 +0100
Rasmus Villemoes <ravi@...vas.dk> wrote:

> On Sat, Oct 25 2025, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> > On Sat, 25 Oct 2025 at 09:40, Yury Norov (NVIDIA) <yury.norov@...il.com> wrote:  
> >>
> >> GENMASK(high, low) notation reflects a common pattern to describe
> >> hardware registers. However, out of drivers context it's confusing and
> >> error-prone.  
> >
> > So I obviously approve of the BITS() syntax, and am not a huge fan of
> > the odd "GENMASK()" argument ordering.
> >
> > That said, I'm not convinced about adding the other helpers. I don't
> > think "FIRST_BITS(8)" is any more readable than "BITS(0,7)", and I
> > think there's a real danger to having lots of specialized macros that
> > people then have to know what they mean.
> >
> > I have an absolutely *disgusting* trick to use the syntax
> >
> >     BITS(3 ... 25)
> >
> > to do this all, but it's so disgusting and limited that I don't think
> > it's actually reasonable.
> >
> > In case somebody can come up with a cleaner model, here's the trick:
> >
> >    #define LOWRANGE_0 0,
> >    #define LOWRANGE_1 1,
> >    #define LOWRANGE_2 2,
> >    #define LOWRANGE_3 3,
> >    #define LOWRANGE_4 4,
> >    #define LOWRANGE_5 5,
> >    // ..and so on
> >
> >    #define _HIGH_VAL(x) (sizeof((char[]){[x]=1})-1)
> >    #define __FIRST(x, ...) x
> >    #define ___LOW_VAL(x) __FIRST(x)
> >    #define __LOW_VAL(x) ___LOW_VAL(LOWRANGE_ ##x)
> >    #define _LOW_VAL(x) __LOW_VAL(x)
> >
> >    #define BITS(x) GENMASK(_HIGH_VAL(x), _LOW_VAL(x))
> >
> >    #define TESTVAL 5
> >    unsigned long val1 = BITS(3 ... 25);
> >    unsigned long val2 = BITS(4);
> >    unsigned long val3 = BITS(TESTVAL ... 31);
> >
> > and that syntax with either "3 ... 25" or just a plain "4" does
> > actually work. But only with fairly simple numbers.
> >
> > It doesn't work with more complex expressions (due to the nasty
> > preprocessor pasting hack), and I couldn't figure out a way to make it
> > do so.  
> 
> It's cute, but no, I also don't know how to make it work without the
> preprocessor concatenation stuff.
> 
> There is, however, an alternative that resembles the syntax in data
> sheets even more closely:
> 
> #define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))

That is both beautiful and horrid...
 
> 
> That'll work for
> 
> unsigned long val1 = BITS(3:25);
> unsigned long val3 = BITS(TESTVAL:31);
> 
> for most of the things TESTVAL might expand to - I'm not sure what would
> happen if one of the lo/hi values contains ternaries and isn't properly
> parenthesized, but nobody writes stuff like
> 
> #define RESET_BIT(rev) rev == 0xaa ? 7 : 9
> 
> It doesn't work for the single-bit case, but I don't think it's so bad
> to have to say
> 
> unsigned long val2 = BIT(4);
> 
> and obviously BITS(4:4) would work as well.
> 
> It also doesn't do anything to prevent the hi-lo 11:7 order.

For constants that gets trapped.
If the correct expression in used in GENMASK() the compiler will reject
the negative shift - so doesn't need an explicit test.

Not directly related...

GENMASK() and the FIELD_GET() family need to go on massive diets.
Nest them and the pre-processor generates multi-kb lines.

Somewhere there is a (probably pointless) _Generic() for all the integer types.
(Especially since FIELD_GET() is always u64.)

FIELD_GET(mask, val) is just (val & mask) / (mask & -mask), but even that
really needs mask assigned to a local (just to reduce the line length).
For variable 'mask' you might worry about the cost of the 'divide by power of 2'
but for constants it doesn't matter.

FIELD_PREP(mask, val) is (val * (mask & -mask)) & mask

'Bloat city' comes from:
#define MASK GENMASK(9, 0);
	x = FIELD_GET(MASK, MASK);

I've just looked at bitfield.h - no wonder it bloats the world.
95% of it needs deleting :-)

	David

> 
> Rasmus
>