| 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
| ||
|
Message-ID: <4ezps56sdj7fmr27ivkaqjakv4ex46f5cvmy6oqr3z6gkhiorl@us4qd53jzq34> Date: Thu, 18 Jan 2024 14:42:12 -0600 From: Lucas De Marchi <lucas.demarchi@...el.com> To: Yury Norov <yury.norov@...il.com> CC: Andrew Morton <akpm@...ux-foundation.org>, <intel-gfx@...ts.freedesktop.org>, Kevin Brodsky <kevin.brodsky@....com>, <linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>, Christian König <christian.koenig@....com>, Masahiro Yamada <masahiroy@...nel.org>, Alex Deucher <alexander.deucher@....com>, "Thomas Gleixner" <tglx@...utronix.de>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, <intel-xe@...ts.freedesktop.org> Subject: Re: Re: [Intel-xe] [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and BIT macros Hi, Reviving this thread as now with xe driver merged we have 2 users for a fixed-width BIT/GENMASK. On Wed, Jun 21, 2023 at 07:20:59PM -0700, Yury Norov wrote: >Hi Lucas, all! > >(Thanks, Andy, for pointing to this thread.) > >On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote: >> Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8() macros to create >> masks for fixed-width types and also the corresponding BIT_U32(), >> BIT_U16() and BIT_U8(). > >Can you split BIT() and GENMASK() material to separate patches? > >> All of those depend on a new "U" suffix added to the integer constant. >> Due to naming clashes it's better to call the macro U32. Since C doesn't >> have a proper suffix for short and char types, the U16 and U18 variants >> just use U32 with one additional check in the BIT_* macros to make >> sure the compiler gives an error when the those types overflow. > >I feel like I don't understand the sentence... > >> The BIT_U16() and BIT_U8() need the help of GENMASK_INPUT_CHECK(), >> as otherwise they would allow an invalid bit to be passed. Hence >> implement them in include/linux/bits.h rather than together with >> the other BIT* variants. > >I don't think it's a good way to go because BIT() belongs to a more basic >level than GENMASK(). Not mentioning possible header dependency issues. >If you need to test against tighter numeric region, I'd suggest to >do the same trick as GENMASK_INPUT_CHECK() does, but in uapi/linux/const.h >directly. Something like: > #define _U8(x) (CONST_GT(U8_MAX, x) + _AC(x, U)) but then make uapi/linux/const.h include linux/build_bug.h? I was thinking about leaving BIT() define where it is, and add the fixed-width versions in this header. I was thinking uapi/linux/const.h was more about allowing the U/ULL suffixes for things shared with asm. Lucas De Marchi > >> The following test file is is used to test this: >> >> $ cat mask.c >> #include <linux/types.h> >> #include <linux/bits.h> >> >> static const u32 a = GENMASK_U32(31, 0); >> static const u16 b = GENMASK_U16(15, 0); >> static const u8 c = GENMASK_U8(7, 0); >> static const u32 x = BIT_U32(31); >> static const u16 y = BIT_U16(15); >> static const u8 z = BIT_U8(7); >> >> #if FAIL >> static const u32 a2 = GENMASK_U32(32, 0); >> static const u16 b2 = GENMASK_U16(16, 0); >> static const u8 c2 = GENMASK_U8(8, 0); >> static const u32 x2 = BIT_U32(32); >> static const u16 y2 = BIT_U16(16); >> static const u8 z2 = BIT_U8(8); >> #endif >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@...el.com> >> --- >> include/linux/bits.h | 22 ++++++++++++++++++++++ >> include/uapi/linux/const.h | 2 ++ >> include/vdso/const.h | 1 + >> 3 files changed, 25 insertions(+) >> >> diff --git a/include/linux/bits.h b/include/linux/bits.h >> index 7c0cf5031abe..ff4786c99b8c 100644 >> --- a/include/linux/bits.h >> +++ b/include/linux/bits.h >> @@ -42,4 +42,26 @@ >> #define GENMASK_ULL(h, l) \ >> (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) >> >> +#define __GENMASK_U32(h, l) \ >> + (((~U32(0)) - (U32(1) << (l)) + 1) & \ >> + (~U32(0) >> (32 - 1 - (h)))) >> +#define GENMASK_U32(h, l) \ >> + (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U32(h, l)) >> + >> +#define __GENMASK_U16(h, l) \ >> + ((U32(0xffff) - (U32(1) << (l)) + 1) & \ >> + (U32(0xffff) >> (16 - 1 - (h)))) >> +#define GENMASK_U16(h, l) \ >> + (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U16(h, l)) >> + >> +#define __GENMASK_U8(h, l) \ >> + (((U32(0xff)) - (U32(1) << (l)) + 1) & \ >> + (U32(0xff) >> (8 - 1 - (h)))) >> +#define GENMASK_U8(h, l) \ >> + (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U8(h, l)) > >[...] > >I see nothing wrong with fixed-wight versions of GENMASK if it helps >people to write safer code. Can you please in commit message mention >the exact patch(es) that added a bug related to GENMASK() misuse? It >would be easier to advocate the purpose of new API with that in mind. > >Regarding implementation - we should avoid copy-pasting in cases >like this. Below is the patch that I boot-tested for x86_64 and >compile-tested for arm64. > >It looks less opencoded, and maybe Andy will be less skeptical about >this approach because of less maintenance burden. Please take it if >you like for v2. > >Thanks, >Yury > >>From 39c5b35075df67e7d88644470ca78a3486367c02 Mon Sep 17 00:00:00 2001 >From: Yury Norov <yury.norov@...il.com> >Date: Wed, 21 Jun 2023 15:27:29 -0700 >Subject: [PATCH] bits: introduce fixed-type genmasks > >Generalize __GENMASK() to support different types, and implement >fixed-types versions of GENMASK() based on it. > >Signed-off-by: Yury Norov <yury.norov@...il.com> >--- > include/linux/bitops.h | 1 - > include/linux/bits.h | 22 ++++++++++++---------- > 2 files changed, 12 insertions(+), 11 deletions(-) > >diff --git a/include/linux/bitops.h b/include/linux/bitops.h >index 2ba557e067fe..1db50c69cfdb 100644 >--- a/include/linux/bitops.h >+++ b/include/linux/bitops.h >@@ -15,7 +15,6 @@ > # define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n))) > #endif > >-#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 7c0cf5031abe..cb94128171b2 100644 >--- a/include/linux/bits.h >+++ b/include/linux/bits.h >@@ -6,6 +6,8 @@ > #include <vdso/bits.h> > #include <asm/bitsperlong.h> > >+#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) >+ > #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)) >@@ -30,16 +32,16 @@ > #define GENMASK_INPUT_CHECK(h, l) 0 > #endif > >-#define __GENMASK(h, l) \ >- (((~UL(0)) - (UL(1) << (l)) + 1) & \ >- (~UL(0) >> (BITS_PER_LONG - 1 - (h)))) >-#define GENMASK(h, l) \ >- (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) >+#define __GENMASK(t, h, l) \ >+ (GENMASK_INPUT_CHECK(h, l) + \ >+ (((t)~0ULL - ((t)(1) << (l)) + 1) & \ >+ ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h))))) > >-#define __GENMASK_ULL(h, l) \ >- (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ >- (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))) >-#define GENMASK_ULL(h, l) \ >- (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) >+#define GENMASK(h, l) __GENMASK(unsigned long, h, l) >+#define GENMASK_ULL(h, l) __GENMASK(unsigned long long, h, l) >+#define GENMASK_U8(h, l) __GENMASK(u8, h, l) >+#define GENMASK_U16(h, l) __GENMASK(u16, h, l) >+#define GENMASK_U32(h, l) __GENMASK(u32, h, l) >+#define GENMASK_U64(h, l) __GENMASK(u64, h, l) > > #endif /* __LINUX_BITS_H */ >-- >2.39.2 >
Powered by blists - more mailing lists