linux-kernel - Re: [PATCH 01/44] x86/asm/bitops: Change the return type of variable_

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251124181859.5ead20fc@pumpkin>
Date: Mon, 24 Nov 2025 18:18:59 +0000
From: David Laight <david.laight.linux@...il.com>
To: Yury Norov <yury.norov@...il.com>
Cc: linux-kernel@...r.kernel.org, Borislav Petkov <bp@...en8.de>, Dave
 Hansen <dave.hansen@...ux.intel.com>, Ingo Molnar <mingo@...hat.com>,
 Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org
Subject: Re: [PATCH 01/44] x86/asm/bitops: Change the return type of
 variable__ffs() to unsigned int

On Mon, 24 Nov 2025 09:58:51 -0500
Yury Norov <yury.norov@...il.com> wrote:

> On Thu, Nov 20, 2025 at 09:18:10PM +0000, David Laight wrote:
> > On Thu, 20 Nov 2025 13:33:11 -0500
> > Yury Norov <yury.norov@...il.com> wrote:
> >   
> > > On Thu, Nov 20, 2025 at 06:29:09PM +0000, David Laight wrote:  
> > > > On Thu, 20 Nov 2025 10:54:01 -0500
> > > > Yury Norov <yury.norov@...il.com> wrote:
> > > >     
> > > > > On Wed, Nov 19, 2025 at 10:40:57PM +0000, david.laight.linux@...il.com wrote:    
> > > > > > From: David Laight <david.laight.linux@...il.com>
> > > > > > 
> > > > > > The return type of variable__ffs() is currently 'unsigned long'.
> > > > > > This makes the x86 __ffs() be 'unsigned long' whereas the generic
> > > > > > version is 'unsigned int'.
> > > > > > 
> > > > > > Similarly change variable_ffz() and ffz().
> > > > > > 
> > > > > > This may save some REX prefix on 64bit.
> > > > > > 
> > > > > > Detected by some extra checks added to min_t() to detect possible
> > > > > > truncation of large values.
> > > > > > 
> > > > > > Signed-off-by: David Laight <david.laight.linux@...il.com>
> > > > > > ---
> > > > > >  arch/x86/include/asm/bitops.h | 18 +++++++-----------
> > > > > >  1 file changed, 7 insertions(+), 11 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> > > > > > index c2ce213f2b9b..2e8a954d2e2d 100644
> > > > > > --- a/arch/x86/include/asm/bitops.h
> > > > > > +++ b/arch/x86/include/asm/bitops.h
> > > > > > @@ -240,7 +240,7 @@ arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)
> > > > > >  					  variable_test_bit(nr, addr);
> > > > > >  }
> > > > > >  
> > > > > > -static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word)
> > > > > > +static __always_inline __attribute_const__ unsigned int variable__ffs(unsigned long word)      
> > > > > 
> > > > > There's a mismatch with the generic ffs() in asm-generic/bitops/ffs.h.
> > > > > 
> > > > > The generic_ffs() returns int. There is another variable__ffs() defined
> > > > > in arch/risk, also returning int.
> > > > > 
> > > > > So I believe, the correct fix would be to switch x86 to int as well.
> > > > > And anyways, I believe this deserves a separate series.    
> > > > 
> > > > It is a single patch, do you want me to resend the patch on its own?
> > > > (With a different commit message)    
> > > 
> > > The patch looks wrong to me. All the flavors of ffs(), ffz() and
> > > others should have the same signature.  
> > 
> > I only changed 'long' to 'int', all the generic (and builtin) ones are 'int'.
> > So I'm not sure which difference you are talking about.
> > IIRC the __attribute_const__ just lets the compiler do CSE.  
> 
> All the versions of ffs(), ffz() and others should in the better world
> have the same signatures. This is unfortunately not what we have now.
> 
> for __ffs() we've got signatures:
> 
> RISCV:
> static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word)
> 
> x86:
> static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word)
> 
> Generic:
> static __always_inline __attribute_const__ unsigned int generic___ffs(unsigned long word)
> 
> You're changing x86 implementation to match the generic one, but don't
> change RISCV, and this is wrong.

More like 'incomplete'.
It looks like chunks of the RISCV file are a straight C&P of the x86 one.

> Can you make them all matching, and check the same for ffz() please?
> This would require a small separate series, I guess.

I can do a patch for RISCV - but I'm not setup to compile it.
But it should be just changing the function return types.

> 
> Moving forward, it would be nice to make all that helpers looking more
> unified across different arches and flavors. Although, out of scope of
> your series.

I tripped up this one as part of my min_t() checks.
Basically if you do any maths with the result of the x86 'ffs' functions
the other operand (which is likely to be 32bits due to the domain of the
value) has to be promoted to 64bits.
This is likely to happen more often than the opposite.

	David

> 
> Thanks,
> Yury