linux-kernel - Re: [PATCH] x86-32: fix cmpxchg8b

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]

Date: Wed, 26 Jun 2024 11:07:06 +0200
From: Uros Bizjak <ubizjak@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...en8.de>, 
	Peter Zijlstra <peterz@...radead.org>, linux-kernel@...r.kernel.org, x86@...nel.org, 
	kernel test robot <lkp@...el.com>
Subject: Re: [PATCH] x86-32: fix cmpxchg8b_emu build error with clang

On Wed, Jun 26, 2024 at 9:39 AM Uros Bizjak <ubizjak@...il.com> wrote:
>
> On Wed, Jun 26, 2024 at 3:13 AM Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
> >
> > The kernel test robot reported that clang no longer compiles the 32-bit
> > x86 kernel in some configurations due to commit 95ece48165c1
> > ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}()
> > functions").
> >
> > The build fails with
> >
> >   arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
> >
> > and the reason seems to be that not only does the cmpxchg8b instruction
> > need four fixed registers (EDX:EAX and ECX:EBX), with the emulation
> > fallback the inline asm also wants a fifth fixed register for the
> > address (it uses %esi for that, but that's just a software convention
> > with cmpxchg8b_emu).
> >
> > Avoiding using another pointer input to the asm (and just forcing it to
> > use the "0(%esi)" addressing that we end up requiring for the sw

A nit: offset 0 is required only for %ebp, so the above should read "(%esi)".

> > fallback) seems to fix the issue.
> >
> > Reported-by: kernel test robot <lkp@...el.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202406230912.F6XFIyA6-lkp@intel.com/
> > Fixes: 95ece48165c1 ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}() functions")
> > Link: https://lore.kernel.org/all/202406230912.F6XFIyA6-lkp@intel.com/
> > Suggested-by: Uros Bizjak <ubizjak@...il.com>
> > Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
> > ---
> >
> > Added commit message, and updated the asm to use '%a[ptr]' instead of
> > writing out the addressing by hand.
> >
> > Still doing the 'oldp' writeback unconmditionally.  The code generation
> > for the case I checked were the same for both clang and gcc, but until
> > Uros hits me with the big clue-hammer, I think it's the simpler code
> > that leaves room for potentially better optimizations too.
>
> You probably want to look at 44fe84459faf1 ("locking/atomic: Fix
> atomic_try_cmpxchg() semantics") [1] and the long LKML discussion at
> [2].
>
> --quote--
> This code is broken with the current implementation, the problem is
> with unconditional update of *__po.
>
> In case of success it writes the same value back into *__po, but in
> case of cmpxchg success we might have lose ownership of some memory
> locations and potentially over what __po has pointed to. The same
> holds for the re-read of *__po. "
> --/quote--
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=44fe84459faf1a7781595b7c64cd36daf2f2827d
> [2] https://lore.kernel.org/lkml/CACT4Y+bG+a0w6j6v1AmBE7fqqMSPyPEm4QimCzCouicmHT8FqA@mail.gmail.com/
>
> Uros.
>
> >
> > This falls solidly in the "looks ok to me, but still untested" category
> > for me.  It fixes the clang build issue in my build testing, but I no
> > longer have a 32-bit test environment, so no actual runtime testing.                                     \

...

> >                                                                         \
> > -       if (unlikely(!ret))                                             \
> > -               *(_oldp) = o.full;                                      \
> > +       *(_oldp) = o;                                                   \

With the above part changed to:

       if (unlikely(!ret))                                             \
-               *(_oldp) = o.full;                                      \
+               *(_oldp) = o;                                           \

Reviewed-and-Tested-by: Uros Bizjak <ubizjak@...il.com>

Runtime tested with .config provided by test robot on qemu-i386 with
both clang and GCC compiler:

LKP: ttyS0: 229: Kernel tests: Boot OK!

Thanks,
Uros.