linux-kernel - Re: [PATCH v2 4/5] math64: Silence a clang -Wshorten-64-to-32 warning

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: <20250501212659.7e642411@pumpkin>
Date: Thu, 1 May 2025 21:26:59 +0100
From: David Laight <david.laight.linux@...il.com>
To: Ian Rogers <irogers@...gle.com>
Cc: Yury Norov <yury.norov@...il.com>, Rasmus Villemoes
 <linux@...musvillemoes.dk>, Arnd Bergmann <arnd@...db.de>, Nathan
 Chancellor <nathan@...nel.org>, Nick Desaulniers
 <nick.desaulniers+lkml@...il.com>, Bill Wendling <morbo@...gle.com>, Justin
 Stitt <justinstitt@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>,
 Thomas Gleixner <tglx@...utronix.de>, Jakub Kicinski <kuba@...nel.org>,
 Jacob Keller <jacob.e.keller@...el.com>, linux-arch@...r.kernel.org,
 linux-kernel@...r.kernel.org, llvm@...ts.linux.dev, Leo Yan
 <leo.yan@....com>
Subject: Re: [PATCH v2 4/5] math64: Silence a clang -Wshorten-64-to-32
 warning

On Thu, 1 May 2025 13:15:30 -0700
Ian Rogers <irogers@...gle.com> wrote:

> On Thu, May 1, 2025 at 1:07 PM David Laight
> <david.laight.linux@...il.com> wrote:
> >
> > On Wed, 30 Apr 2025 10:15:33 -0700
> > Ian Rogers <irogers@...gle.com> wrote:
> >  
> > > The clang warning -Wshorten-64-to-32 can be useful to catch
> > > inadvertent truncation. In some instances this truncation can lead to
> > > changing the sign of a result, for example, truncation to return an
> > > int to fit a sort routine. Silence the warning by making the implicit
> > > truncation explicit. This isn't to say the code is currently incorrect
> > > but without silencing the warning it is hard to spot the erroneous
> > > cases.  
> >
> > Except that the extra casts make the reader think something 'extra'
> > is going on.
> > For readability you want as few casts as possible.  
> 
> Agreed except when not having the cast can introduce bugs which is why
> the cast is always required in other languages. Consider in Java:
> ```
> class a {
>   public static void main(String args[]) {
>      long x = args.length;
>      int y = x;
>  }
> }
> $ javac a.java
> a.java:4: error: incompatible types: possible lossy conversion from long to int
>      int y = x;
>              ^
> 1 error

I'm not a java expert, but I suspect it has 'softer' type conversions
for integers than C casts.
I've been badly bitten by C casts that make code compile when it really
shouldn't, or incorrectly mask off high bits.
There are actually loads of them in the Linux kernel.
As well as all the dubious min_t(u16,...) there are the (__force ...)
where the compiler shouldn't see a cast at all (it is for sparse).

> ```
> Having -Wshorten-64-to-32 enabled for building with clang would allow
> possible mistakes to be spotted, but that's not currently possible
> without wading through warnings that this series cleans up.
> 
> I also don't really think anyone will be confused about the purpose of
> the cast in something like:
> ```
> al = (u32)a;

And no one is confused by what the code is doing without the cast.

We live with the 'integer to pointer of differ size' warning,
but even that was only really useful 30 years ago and is well
past its 'best before' date.

	David

> ```
> 
> Thanks,
> Ian
> 
> >         David
> >
> >  
> > >
> > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > > ---
> > >  include/linux/math64.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/math64.h b/include/linux/math64.h
> > > index 6aaccc1626ab..f32fcb2a2331 100644
> > > --- a/include/linux/math64.h
> > > +++ b/include/linux/math64.h
> > > @@ -179,7 +179,7 @@ static __always_inline u64 mul_u64_u64_shr(u64 a, u64 mul, unsigned int shift)
> > >  #ifndef mul_u64_u32_shr
> > >  static __always_inline u64 mul_u64_u32_shr(u64 a, u32 mul, unsigned int shift)
> > >  {
> > > -     u32 ah = a >> 32, al = a;
> > > +     u32 ah = a >> 32, al = (u32)a;
> > >       u64 ret;
> > >
> > >       ret = mul_u32_u32(al, mul) >> shift;  
> >