linux-kernel - Re: [PATCH v2 next] linux/bits.h: Simplify 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z2Dfaw7OTagl47ir@yury-ThinkPad>
Date: Mon, 16 Dec 2024 18:18:19 -0800
From: Yury Norov <yury.norov@...il.com>
To: David Laight <David.Laight@...lab.com>
Cc: "'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>,
	'Rasmus Villemoes' <linux@...musvillemoes.dk>,
	'Andrew Morton' <akpm@...ux-foundation.org>,
	'Masahiro Yamada' <masahiroy@...nel.org>,
	'Vincent Mailhol' <mailhol.vincent@...adoo.fr>,
	'Linus Torvalds' <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v2 next] linux/bits.h: Simplify GENMASK()

On Mon, Dec 16, 2024 at 09:39:23AM +0000, David Laight wrote:
> Change 95b980d62d52c replaced ~0ul and ~0ull with ~UL(0) and ~ULL(0)
> in the GENMASK() defines as part of a change to allow the bitops
> definitions be used from assembly.
> 
> The definitions have since been moved to a uapi header which
> probably shouldn't require some other header be included first.

Sorry, I don't understand that. Are you saying that uapi headers
should not include one another? What is exactly wrong with that
for you? Or did you mean something else? 

> The definition of __GENMASK() is also overcomplicated partially
> due to avoiding overflow warnings from shifting ~0u left.
>
> Implement GENMASK() using the simpler (1u << hi) * 2 - (1u << lo) formula.

I don't think that this formula is any simpler than the original one.

> This doesn't rely on right shifts

What is wrong with right shifts? 

> and doesn't need to know the number
> of bits in the integral type.

What is wrong in BITS_PER_LONG?


> It can be used for different types by just changing the type of the 1u.
> __GENMASK() __GENMASK_ULL() and __GENMASK_U128() can now implemeted
> using a single ___GENMASK(one, hi, lo).

I like idea of generic implementation or different flavors of GENMASK().
I even proposed something similar back then for fixed-type genmasks: 

https://lkml.org/lkml/2023/6/21/1734

Both you and I do the same - provide type as a macro parameter. I like
my way of doing it a bit more because it's more explicit. But now that
we have _Generic(), we don't need this hack at all.

> 
> Overflow warnings (from shifting out the MSB) are avoided by subtracting 1
> before the multiply and adding two back in later.
> The complers see straight through the subterfuge when generating code.

Ironically... You're trying to simplify a part that silences Woverflow, and
end up by adding another part that silences Woverflow...

> Since there are already conditionals for ASSEMBLY in bits.h, for ASSEMBLY
> directly expand GENMASK() and GENMASK_ULL() as ___GENMASK(1, hi, lo)
> rather than through the __GENMASK() and __GENMASK_ULL() uapi defines.
> Remove the UL(x) and ULL(x) from the uapi file.
> 
> GENMASK() and GENMASK_ULL() now generate the correct values when
> ASSEMBLY is defined.
> Fortunately they've never been used.

They were used before. They are not used now - that's true.

Can you explain in details how are those macros are broken for assemblers?
Can you add a test?

> 
> Rename 'h' to 'hi' and 'l' to 'lo' because 'l' looks like '1' in many fonts.

No please don't!

This type of changes don't help at all. They effectively wipe all the
history for absolutely nothing. If your fonts mess characters, please
use other fonts.
 
> Signed-off-by: David Laight <david.laight@...lab.com>
> ---
> 
> v2: '__uint128' => 'unsigned __int128'
> 
>  include/linux/bits.h      | 43 ++++++++++++++++-----------------------
>  include/uapi/linux/bits.h | 15 +++++++-------
>  2 files changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 60044b608817..d5cf0ec22e43 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -14,41 +14,32 @@
>  #define BITS_PER_BYTE		8
>  
>  /*
> - * Create a contiguous bitmask starting at bit position @l and ending at
> - * position @h. For example
> + * Create a contiguous bitmask starting at bit position @lo and ending at
> + * position @hi. For example
>   * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>   */
>  #if !defined(__ASSEMBLY__)
>  #include <linux/build_bug.h>
> -#define GENMASK_INPUT_CHECK(h, l) \
> +#define GENMASK_INPUT_CHECK(hi, lo) \
>  	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> -		__is_constexpr((l) > (h)), (l) > (h), 0)))
> -#else
> -/*
> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> - * disable the input check if that is the case.
> - */
> -#define GENMASK_INPUT_CHECK(h, l) 0
> -#endif
> +		__is_constexpr((lo) > (hi)), (lo) > (hi), 0)))
>  
> -#define GENMASK(h, l) \
> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> -#define GENMASK_ULL(h, l) \
> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> +#define GENMASK(hi, lo) \
> +	(GENMASK_INPUT_CHECK(hi, lo) + __GENMASK(hi, lo))
> +#define GENMASK_ULL(hi, lo) \
> +	(GENMASK_INPUT_CHECK(hi, lo) + __GENMASK_ULL(hi, lo))
>  
> -#if !defined(__ASSEMBLY__)
> +#define GENMASK_U128(hi, lo) \
> +	(GENMASK_INPUT_CHECK(hi, lo) + __GENMASK_U128(hi, lo))
> +#else
>  /*
> - * Missing asm support
> - *
> - * __GENMASK_U128() depends on _BIT128() which would not work
> - * in the asm code, as it shifts an 'unsigned __init128' data
> - * type instead of direct representation of 128 bit constants
> - * such as long and unsigned long. The fundamental problem is
> - * that a 128 bit constant will get silently truncated by the
> - * gcc compiler.
> + * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> + * 128bit exprssions don't work, neither can C 0UL (etc) constants be used.
> + * These definitions only have to work for constants and don't require
> + * that ~0 have any specific number of set bits.
>   */
> -#define GENMASK_U128(h, l) \
> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
> +#define GENMASK(hi, lo) ___GENMASK(1, hi, lo)
> +#define GENMASK_ULL(hi, lo) ___GENMASK(1, hi, lo)
>  #endif
>  
>  #endif	/* __LINUX_BITS_H */
> diff --git a/include/uapi/linux/bits.h b/include/uapi/linux/bits.h
> index 5ee30f882736..a25d9dfb7072 100644
> --- a/include/uapi/linux/bits.h
> +++ b/include/uapi/linux/bits.h
> @@ -4,15 +4,14 @@
>  #ifndef _UAPI_LINUX_BITS_H
>  #define _UAPI_LINUX_BITS_H
>  
> -#define __GENMASK(h, l) \
> -        (((~_UL(0)) - (_UL(1) << (l)) + 1) & \
> -         (~_UL(0) >> (__BITS_PER_LONG - 1 - (h))))
> +/* Result is '(1u << (hi + 1)) - (1u << lo)' coded to avoid overflow. */
> +#define ___GENMASK(one, hi, lo) \
> +	((((one) << (hi)) - 1) * 2 + 1 - (((one) << (lo)) - 1))
>  
> -#define __GENMASK_ULL(h, l) \
> -        (((~_ULL(0)) - (_ULL(1) << (l)) + 1) & \
> -         (~_ULL(0) >> (__BITS_PER_LONG_LONG - 1 - (h))))
> +#define __GENMASK(hi, lo) ___GENMASK(1UL, hi, lo)
>  
> -#define __GENMASK_U128(h, l) \
> -	((_BIT128((h)) << 1) - (_BIT128(l)))
> +#define __GENMASK_ULL(hi, lo) ___GENMASK(1ULL, hi, lo)
> +
> +#define __GENMASK_U128(hi, lo) ___GENMASK((unsigned __int128)1, hi, lo)
>  
>  #endif /* _UAPI_LINUX_BITS_H */
> -- 
> 2.17.1
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)