linux-kernel - RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is

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: <b1ff4a65594a4d39b2e9b8b44770214e@AcuMS.aculab.com>
Date: Sat, 7 Dec 2024 11:19:05 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Vincent Mailhol' <vincent.mailhol@...il.com>, Linus Torvalds
	<torvalds@...ux-foundation.org>, "w@....eu" <w@....eu>
CC: Luc Van Oostenryck <luc.vanoostenryck@...il.com>, Nathan Chancellor
	<nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, "Bill
 Wendling" <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, "Yury
 Norov" <yury.norov@...il.com>, Rasmus Villemoes <linux@...musvillemoes.dk>,
	Kees Cook <kees@...nel.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>,
	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>, Suzuki K Poulose <suzuki.poulose@....com>,
	Mike Leach <mike.leach@...aro.org>, James Clark <james.clark@...aro.org>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Rikard Falkeborn
	<rikard.falkeborn@...il.com>, "linux-sparse@...r.kernel.org"
	<linux-sparse@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "llvm@...ts.linux.dev"
	<llvm@...ts.linux.dev>, "linux-hardening@...r.kernel.org"
	<linux-hardening@...r.kernel.org>, "intel-gfx@...ts.freedesktop.org"
	<intel-gfx@...ts.freedesktop.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "coresight@...ts.linaro.org"
	<coresight@...ts.linaro.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "uecker@...raz.at" <uecker@...raz.at>
Subject: RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of
 __is_constexpr()

From: Vincent Mailhol
> Sent: 07 December 2024 07:43
...
> > So maybe the slightly long lines:
> > #define const_true(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 1L) : (char *)0, char *: 1, void *: 0)
> > #define const_expr(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 0L) : (char *)0, char *: 1, void *: 0)

Clearly they can be implemented in terms of a common define.
But I don't see a need for a const_zero() and nested expansions make extra
work for the compiler.

> 
> This still throws a -Wnull-pointer-arithmetic on clang on const_expr(NULL):
>   https://godbolt.org/z/vo5W7efdE

I was worried about that one.

> I just do not see a method to silence that one. So three options:
> 
>   1. is_const() does not accept pointers and throws a constraint violation:
>        #define is_const(x) __is_const_zero(0 * (x))
>      This is my current patch.

Is that going to affect things like const_true(x << y)?
Disallowing that seems counter-productive.
(Remember it might be passed into a #define that is then
checking its argument for being constant.)

>   2. is_const() accept pointers but is_const(NULL) returns false:
>        #define is_const(x) __is_const_zero((x) != (x))
>      This keeps the current __is_constexpr() behaviour.

No good - expands everything twice.

>   3. is_const() accepts pointers and is_const(NULL) return true:
> 
>        #define const_expr(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 0L)
> : (char *)0, char *: 1, void *: 0)
> 
>      David's latest proposal, it requires to remove the
>      -Wnull-pointer-arithmetic clang warning.

Only for const_expr(NULL) - and since clang gets that wrong
maybe the warning is a good thing.

You can just add:
#define const_NULL(ptr) const_true(!(ptr))
Probably the only place where you actually want to test for zero.

> 
> I vote for 1. or 2. (with a preference for 1.). IMHO, we are just
> adding an unreasonable level of complexity for making the macro treat
> NULL as an integer. Would someone find a solution for 3. that does not
> yield a warning, then why not. But if we have to remove a compiler
> check for a theoretical use case that does not even exist in the
> kernel, then it is not worth the trade off.
> 
> Concerning is_const(var << 2), the patch I submitted works fine as-is
> with all scalars including that (var << 2):
> 
>   https://godbolt.org/z/xer4aMees
> 
> And can we ignore the case (!(var << 2))? This is not a warning
> because of the macro, but because of the caller! If I do any of:
> 
>           if (!(var << 2)) {}
>           (void)__builtin_constant_p(!(var << 2));
> 
> I also got the warning. The point is that the macro should not
> generate *new* warnings. If the given argument already raises a
> warning, it is the caller's responsibility to fix.

Except it could easily happen way inside some other expansion.
Perhaps someone optimises frobnicate(x) for constant input.
Suddenly frobnicate(!(var << 2)) generates a compile error.

	David

> 
> 
> Yours sincerely,
> Vincent Mailhol

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