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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <aSRyo0LLgChJFrYD@yury>
Date: Mon, 24 Nov 2025 09:58:51 -0500
From: Yury Norov <yury.norov@...il.com>
To: David Laight <david.laight.linux@...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 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.

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

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.

Thanks,
Yury