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: <ZxAAlEyvxzWpwQQ0@smile.fi.intel.com>
Date: Wed, 16 Oct 2024 21:06:12 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: Ingo Molnar <mingo@...nel.org>, Uros Bizjak <ubizjak@...il.com>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	llvm@...ts.linux.dev, 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()

On Wed, Oct 16, 2024 at 08:44:56AM -0700, Dave Hansen wrote:
> 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

Thanks, makes sense!

> > --- 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.

But if feels right if we really want to supply unsigned types here.
Maybe some more magic is needed (like in min() case).

> 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?

Maybe more readable, but wouldn't it be theoretically buggy for u64?
I'm talking about the case when u64 == UINT_MAX, which will be true
in your case and false in mine.

>         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?

Why are you asking me about this? I don't know...

> Or do most clang users just ignore all the warnings?

Same here. I don't know...

Both Qs sounds rhetorical to me.

> Or are you using a bleeding edge version of clang that spits out new warnings
> that other clang users aren't seeing?

AFAICT It's *not* even close to the bleeding edge. It's standard Debian supply.

> 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.

I have run percpu_test in both cases and also checked code with `bloat-o-meter`
and `cmp -b`. Everything is the same. I even added a test case for the above
mentioned situation.

> 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.

It took me much more to find the best solution that appears
not everyone likes :-)

P.S. And as Nick pointed out it's simple `make W=1`,
     what the additional information you wanna see here?
     Care to provide a template?

-- 
With Best Regards,
Andy Shevchenko