linux-kernel - Re: [PATCH] lib/clz_ctz.c: Fix __clzdi2() and _

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: <CAKwvOdnrg9b3nF4mRWGYJSKytnEtGdKGz+aHPWSX3aHUm5eAYg@mail.gmail.com>
Date:   Mon, 28 Aug 2023 13:10:46 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Helge Deller <deller@....de>,
        Nathan Chancellor <nathan@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-parisc@...r.kernel.org" <linux-parisc@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Chanho Min <chanho.min@....com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] lib/clz_ctz.c: Fix __clzdi2() and __ctzdi2() for 32-bit kernels

On Mon, Aug 28, 2023 at 3:53 AM David Laight <David.Laight@...lab.com> wrote:
>
> From: Linus Torvalds
> > Sent: 25 August 2023 21:43
> ....
> > Clang turns this:
> >
> >         return __ffs64(val);
> >
> > into this horror:
> >
> >         pushq   %rax
> >         movq    %rdi, (%rsp)
> >         #APP
> >         rep
> >         bsfq    (%rsp), %rax
> >         #NO_APP
> >         popq    %rcx
> >
> > which is just incredibly broken on so many levels. It *should* be a
> > single instruction, like gcc does:
> >
> >         rep; bsf %rdi,%rax      # tmp87, word
> >
> > but clang decides that it really wants to put the argument on the
> > stack, and apparently also wants to do that nonsensical stack
> > alignment thing to make things even worse.
> >
> > We use this:
> >
> >   static __always_inline unsigned long variable__ffs(unsigned long word)
> >   {
> >         asm("rep; bsf %1,%0"
> >                 : "=r" (word)
> >                 : "rm" (word));
> >         return word;
> >   }
> >
> > for the definition, and it looks like clang royally just screws up
> > here. Yes, "m" is _allowed_ in that input set, but it damn well
> > shouldn't be used for something that is already in a register, since
> > "r" is also allowed, and is the first choice.
>
> Why don't we just remove the "m" option?
>
> Pretty much the only time it will be worse is it the value
> is in memory and loading it into a register causes a spill
> to stack.
>
> While it is possible to generate code where that happens it
> is pretty unlikely.

As Linus expressed below, register exhaustion could occur.  Besides,
this is a bug in clang that we acknowledge, and should fix.  I have
the general idea where things are going wrong, I just don't yet have
the muscle memory (or time) to dive into the register allocator.

>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)



-- 
Thanks,
~Nick Desaulniers