linux-kernel - Re: [PATCH v1 1/1] x86/percpu: Cast -1 to argument type when comparing in percpu_add

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: <f02e0624-ad4f-473c-b172-6dadea37f600@intel.com>
Date: Wed, 16 Oct 2024 08:44:56 -0700
From: Dave Hansen <dave.hansen@...el.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Ingo Molnar <mingo@...nel.org>, Uros Bizjak <ubizjak@...il.com>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Cc: Dennis Zhou <dennis@...nel.org>, Tejun Heo <tj@...nel.org>,
 Christoph Lameter <cl@...ux.com>, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
 Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
 "H. Peter Anvin" <hpa@...or.com>, Nathan Chancellor <nathan@...nel.org>,
 Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling
 <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>
Subject: Re: [PATCH v1 1/1] x86/percpu: Cast -1 to argument type when
 comparing in percpu_add_op()

Andy,

The subject here is not very informative.  It explains the "what" of the
patch, but not the "why".

A better subject might have been:

	x86/percpu: Fix clang warning when dealing with unsigned types

> --- a/arch/x86/include/asm/percpu.h
> +++ b/arch/x86/include/asm/percpu.h
> @@ -234,9 +234,10 @@ do {									\
>   */
>  #define percpu_add_op(size, qual, var, val)				\
>  do {									\
> -	const int pao_ID__ = (__builtin_constant_p(val) &&		\
> -			      ((val) == 1 || (val) == -1)) ?		\
> -				(int)(val) : 0;				\
> +	const int pao_ID__ =						\
> +		(__builtin_constant_p(val) &&				\
> +			((val) == 1 ||					\
> +			 (val) == (typeof(val))-1)) ? (int)(val) : 0;	\

This doesn't _look_ right.

Let's assume 'val' is a u8.  (u8)-1 is 255, right?  So casting the -1
over to a u8 actually changed its value.  So the comparison that you
added would actually trigger for 255:

	(val) == (typeof(val))-1))

	255 == (u8)-1
	255 == 255

That's not the end of the world because the pao_ID__ still ends up at
255 and the lower if() falls into the "add" bucket, but it isn't great
for reading the macro.  It seems like it basically works on accident.

Wouldn't casting 'val' over to an int be shorter, more readable, not
have that logical false match *and* line up with the cast later on in
the expression?

        const int pao_ID__ = (__builtin_constant_p(val) &&
                              ((val) == 1 || (int)(val) == -1)) ?

                                (int)(val) : 0;

Other suggestions to make it more readable would be welcome.

Since I'm making comments, I would have really appreciated some extra
info here like why you are hitting this and nobody else is.  This is bog
standard code that everybody compiles.  Is clang use _that_ unusual?  Or
do most clang users just ignore all the warnings?  Or are you using a
bleeding edge version of clang that spits out new warnings that other
clang users aren't seeing?

Another nice thing would have been to say that this produces the exact
same code with and without the patch.  Or that you had tested it in
*some* way.  It took me a couple of minutes to convince myself that your
version works and doesn't do something silly like a "dec" if you hand in
val==255.