linux-kernel - RE: [PATCHv3 04/10] linux/kernel: introduce lower_48

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]

Date:   Tue, 22 Feb 2022 20:09:21 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Joe Perches' <joe@...ches.com>, Keith Busch <kbusch@...nel.org>,
        Christoph Hellwig <hch@....de>
CC:     "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "colyli@...e.de" <colyli@...e.de>,
        Bart Van Assche <bvanassche@....org>
Subject: RE: [PATCHv3 04/10] linux/kernel: introduce lower_48_bits macro

From: Joe Perches
> Sent: 22 February 2022 18:43
> 
> On Tue, 2022-02-22 at 08:56 -0800, Keith Busch wrote:
> > On Tue, Feb 22, 2022 at 05:50:45PM +0100, Christoph Hellwig wrote:
> > > On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote:
> > > > On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote:
> > > > > +/ *
> > > > > + * lower_48_bits - return bits 0-47 of a number
> > > > > + * @n: the number we're accessing
> > > > > + */
> > > > > +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull))
> > > >
> > > > why not make this a static inline function?
> > >
> > > Agreed.
> >
> > Sure, that sounds good to me. I only did it this way to match the
> > existing local convention, but I personally prefer the inline function
> > too.
> 
> The existing convention is used there to allow the compiler to
> avoid warnings and unnecessary conversions of a u32 to a u64 when
> shifting by 32 or more bits.
> 
> If it's possible to be used with an architecture dependent typedef
> like dma_addr_t, then perhaps it's reasonable to do something like:
> 
> #define lower_48_bits(val)					\
> ({								\
> 	typeof(val) high = lower_16_bits(upper_32_bits(val));	\
> 	typeof(val) low = lower_32_bits(val);			\
> 								\
> 	(high << 16 << 16) | low;				\
> })
> 
> and have the compiler have the return value be an appropriate type.

The compiler could make a real pigs breakfast of that.

For lower_46_bits() an integer promotion to u64 does no harm.
But for some other cases you get in a right mess when values
that should be unsigned get sign extended.

Although I think:
	(val) & (((typeof(val))1 << 48) - 1)
avoids any promotion if anyone tries lower_48_bits(int_var).
(It is even likely to be a compile error.)

Oh, did you look for GENMASK([^,]*,[ 0]*) ?
I'd only use something GENMASK() for bit ranges.
Even then it is often easier to just write the value in hex.

I think the only time I've written anything like that recently
(last 30 years) was for some hardware registers when the documentation
user 'bit 1' for the most significant bit.

It's rather like I just know that (x & (x - 1)) checks for 1 bit being set.
I have to lookup is_power_of_2() to see what it does.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)