linux-kernel - Re: [PATCH v6 2/7] bits: introduce fixed-type genmasks

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: <Z9mjJ3gJoqLwjIFX@thinkpad>
Date: Tue, 18 Mar 2025 12:45:27 -0400
From: Yury Norov <yury.norov@...il.com>
To: mailhol.vincent@...adoo.fr
Cc: Lucas De Marchi <lucas.demarchi@...el.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Jani Nikula <jani.nikula@...ux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
	Rodrigo Vivi <rodrigo.vivi@...el.com>,
	Tvrtko Ursulin <tursulin@...ulin.net>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
	dri-devel@...ts.freedesktop.org,
	Andi Shyti <andi.shyti@...ux.intel.com>,
	David Laight <David.Laight@...lab.com>,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Jani Nikula <jani.nikula@...el.com>
Subject: Re: [PATCH v6 2/7] bits: introduce fixed-type genmasks

On Sat, Mar 08, 2025 at 01:48:49AM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Yury Norov <yury.norov@...il.com>
> 
> Add GENMASK_TYPE() which generalizes __GENMASK() to support different
> types, and implement fixed-types versions of GENMASK() based on it.
> The fixed-type version allows more strict checks to the min/max values
> accepted, which is useful for defining registers like implemented by
> i915 and xe drivers with their REG_GENMASK*() macros.
> 
> The strict checks rely on shift-count-overflow compiler check to fail
> the build if a number outside of the range allowed is passed.
> Example:
> 
>   #define FOO_MASK GENMASK_U32(33, 4)
> 
> will generate a warning like:
> 
>   include/linux/bits.h:51:27: error: right shift count >= width of type [-Werror=shift-count-overflow]
>      51 |               type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))
>         |                           ^~
> 
> Signed-off-by: Yury Norov <yury.norov@...il.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@...el.com>
> Acked-by: Jani Nikula <jani.nikula@...el.com>
> Co-developed-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> ---
> Changelog:
> 
>   v5 -> v6:
> 
>     - No changes.
> 
>   v4 -> v5:
> 
>     - Rename GENMASK_t() to GENMASK_TYPE().
> 
>     - Fix typo in patch description.
> 
>     - Use tab indentations instead of single space to separate the
>       macro name from its body.
> 
>     - s/__GENMASK_U*()/GENMASK_U*()/g in the comment.
> 
>     - Add a tag to credit myself as Co-developer. Keep Yury as the
>       main author.
> 
>     - Modify GENMASK_TYPE() to match the changes made to __GENMASK()
>       in: https://github.com/norov/linux/commit/1e7933a575ed
> 
>     - Replace (t)~_ULL(0) with type_max(t). This is OK because
>       GENMASK_TYPE() is not available in asm.
> 
>     - linux/const.h and asm/bitsperlong.h are not used anymore. Remove
>       them.
> 
>     - Apply GENMASK_TYPE() to GENMASK_U128().
> 
>     - Remove the unsigned int cast for the U8 and U16 variants. Cast
>       to the target type instead. Do that cast directly in
>       GENMASK_TYPE().
> 
>   v3 -> v4:
> 
>     - The v3 is one year old. Meanwhile people started using
>       __GENMASK() directly. So instead of generalizing __GENMASK() to
>       support different types, add a new GENMASK_t().
> 
>     - replace ~0ULL by ~_ULL(0). Otherwise, GENMASK_t() would fail in
>       asm code.
> 
>     - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In
>       v3, due to the integer promotion rules, these were returning a
>       signed integer. By casting these to unsigned int, at least the
>       signedness is kept.
> ---
>  include/linux/bitops.h |  1 -
>  include/linux/bits.h   | 55 ++++++++++++++++++++++++++++++++------------------
>  2 files changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -8,7 +8,6 @@
>  
>  #include <uapi/linux/kernel.h>
>  
> -#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>  #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>  #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
>  #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 9c1c7ce0bba6bb09490d891904c143a5394fd512..b690611c769be61ab2b5ced43c8302ba5693308b 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -2,16 +2,15 @@
>  #ifndef __LINUX_BITS_H
>  #define __LINUX_BITS_H
>  
> -#include <linux/const.h>
>  #include <vdso/bits.h>
>  #include <uapi/linux/bits.h>
> -#include <asm/bitsperlong.h>
>  
>  #define BIT_MASK(nr)		(UL(1) << ((nr) % BITS_PER_LONG))
>  #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
>  #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
>  #define BIT_ULL_WORD(nr)	((nr) / BITS_PER_LONG_LONG)
>  #define BITS_PER_BYTE		8
> +#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>  
>  /*
>   * Create a contiguous bitmask starting at bit position @l and ending at
> @@ -20,28 +19,44 @@
>   */
>  #if !defined(__ASSEMBLY__)
>  
> -#include <linux/build_bug.h>
> -#include <linux/compiler.h>
> -
> -#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
> -
> -#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))
> -
>  /*
>   * Missing asm support
>   *
> - * __GENMASK_U128() depends on _BIT128() which would not work
> - * in the asm code, as it shifts an 'unsigned __int128' 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.
> + * GENMASK_U*() depends on BITS_PER_TYPE() which relies on sizeof(),
> + * something not available in asm. Nethertheless, fixed width integers
> + * is a C concept. Assembly code can rely on the long and long long
> + * versions instead.
>   */
> -#define GENMASK_U128(h, l) \
> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
> +
> +#include <linux/build_bug.h>
> +#include <linux/compiler.h>
> +#include <linux/overflow.h>
> +
> +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
> +
> +/*
> + * Generate a mask for the specified type @t. Additional checks are made to
> + * guarantee the value returned fits in that type, relying on
> + * shift-count-overflow compiler check to detect incompatible arguments.
> + * For example, all these create build errors or warnings:
> + *
> + * - GENMASK(15, 20): wrong argument order
> + * - GENMASK(72, 15): doesn't fit unsigned long
> + * - GENMASK_U32(33, 15): doesn't fit in a u32
> + */
> +#define GENMASK_TYPE(t, h, l)					\
> +	((t)(GENMASK_INPUT_CHECK(h, l) +			\
> +	     (type_max(t) << (l) &				\
> +	      type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))
> +
> +#define GENMASK(h, l)		GENMASK_TYPE(unsigned long,  h, l)
> +#define GENMASK_ULL(h, l)	GENMASK_TYPE(unsigned long long, h, l)

I like everything except this part. We switch GENMASK() from a well
tested implementation, including an asm code, and we split uapi and
non-uapi users, with no functionality changes.

Unification is a solid point, however.

Let's make it a 2-step procedure? Adding fixed-width GENMASKs is a
non-questionable improvement. Switching an existing API from one
implementation to another should be a separate patch, and probably
even a separate series. And we should be very clear that __GENMASK()
is uapi-only thing from now.

If we decide to switch GENMASK() in a separate series, we'll have some
extra time to think about unification...

> +#define GENMASK_U8(h, l)	GENMASK_TYPE(u8, h, l)
> +#define GENMASK_U16(h, l)	GENMASK_TYPE(u16, h, l)
> +#define GENMASK_U32(h, l)	GENMASK_TYPE(u32, h, l)
> +#define GENMASK_U64(h, l)	GENMASK_TYPE(u64, h, l)
> +#define GENMASK_U128(h, l)	GENMASK_TYPE(u128, h, l)
>  
>  #else /* defined(__ASSEMBLY__) */
>  
> 
> -- 
> 2.45.3
>