Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1741698 > unrolled thread

Re: [lkp-robot] [x86/asm] f5caf621ee: PANIC:double_fault

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [lkp-robot] [x86/asm] f5caf621ee: PANIC:double_fault Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-28 18:30 +0200
    Re: [lkp-robot] [x86/asm] f5caf621ee: PANIC:double_fault Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-28 18:50 +0200
      Re: [lkp-robot] [x86/asm] f5caf621ee: PANIC:double_fault Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-28 19:10 +0200
        Re: [lkp-robot] [x86/asm] f5caf621ee: PANIC:double_fault Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-28 21:20 +0200
          [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4 Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-29 00:00 +0200
            Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4 Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-29 02:00 +0200
              Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4 Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-29 03:50 +0200
                Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4 Ingo Molnar <mingo@kernel.org> - 2017-09-29 10:10 +0200
                  Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4 Ye Xiaolong <xiaolong.ye@intel.com> - 2017-09-29 12:40 +0200
              Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4 Ingo Molnar <mingo@kernel.org> - 2017-09-29 10:00 +0200
                Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4 Arnd Bergmann <arnd@arndb.de> - 2017-09-29 17:30 +0200
            [tip:x86/urgent] x86/asm: Fix inline asm call constraints for GCC  4.4 tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2017-09-29 11:40 +0200
            [tip:x86/urgent] x86/asm: Fix inline asm call constraints for GCC  4.4 tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2017-09-29 13:30 +0200

#1741698 — Re: [lkp-robot] [x86/asm] f5caf621ee: PANIC:double_fault

On Thu, Sep 28, 2017 at 12:47 AM, kernel test robot
<xiaolong.ye@intel.com> wrote:
>
> [   10.587519] RIP: 0010:compat_sock_ioctl+0xfea/0x103e
> [   10.587974] RSP: 0000:0000000000277d78 EFLAGS: 00010283
> [   10.588448] RAX: 0000000000277d78 RBX: 0000000000008933 RCX: ffff8800141a8000
> [   10.589103] RDX: 0000000000000020 RSI: 00000000fffbea00 RDI: 00000000fffbea50
> [   10.589757] RBP: ffffc90000277e18 R08: fffbea50fffbea34 R09: ffffffff814a68c9
> [   10.590407] R10: ffffff9c00000002 R11: 00000000fffbea50 R12: 0000000000000000
> [   10.591056] R13: ffff880012c8c880 R14: 00000000fffbea50 R15: 00000000fffbea00
> [   10.591708] FS:  0000000000000000(0000) GS:ffff880019a00000(0063) knlGS:00000000f7fab9a0
> [   10.592446] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> [   10.592973] CR2: 0000000000277d68 CR3: 000000001807f000 CR4: 00000000000006b0
> [   10.593623] Call Trace:
> [   10.593858] Code: 02 0f ff 65 48 8b 04 25 80 d1 00 00 48 8b 80 28 25 00 00 48 83 e8 20 49 39 c7 77 34 89 e0 4c 89 f7 4c 89 fe ba 20 00 00 00 89 c4 <e8> b3 52 05 00 85 c0 74 22 eb 1a 4c 89 fa 89 de 4c 89 ef e8 c6
> [   10.595705] Kernel panic - not syncing: Machine halted.

That is some _funky_ code, and yes, this may well be triggered by the
inline asm changes.

The code decodes to (after ignoring a few bytes at the beginning that
were in the middle of an instruction)

   0: 65 48 8b 04 25 80 d1 mov    %gs:0xd180,%rax
   7: 00 00
   9: 48 8b 80 28 25 00 00 mov    0x2528(%rax),%rax
  10: 48 83 e8 20          sub    $0x20,%rax
  14: 49 39 c7              cmp    %rax,%r15
  17: 77 34                ja     0x4d
  19: 89 e0                mov    %esp,%eax
  1b: 4c 89 f7              mov    %r14,%rdi
  1e: 4c 89 fe              mov    %r15,%rsi
  21: ba 20 00 00 00        mov    $0x20,%edx
  26: 89 c4                mov    %eax,%esp
  28:* e8 b3 52 05 00        callq  0x552e0 <-- trapping instruction
  2d: 85 c0                test   %eax,%eax
  2f: 74 22                je     0x53
  31: eb 1a                jmp    0x4d
  33: 4c 89 fa              mov    %r15,%rdx
  36: 89 de                mov    %ebx,%esi
  38: 4c 89 ef              mov    %r13,%rdi

and it's worth noting that insane

     mov    %eax,%esp

instruction, and how RAX (and RSP) both have that bad value of
0000000000277d78 in them.

So double fault is correct - we've corrupted the stack.

And NOTE! It's reloading 32 bits, not 64 bits, and that's the basic bug there.

I do note that when I build a kernel, I do see that pattern of

    movl    $32, %edx
    call <something>

and in every case it's a a call to a user copy. One is "call
_copy_from_user", while the other ones are all the
alternative_call_2() in copy_user_generic().

Judging by the offset within the function, and judging by the bug,
it's almost certainly that alternative_call_2() case.

So it does sound like the clang fix has now introduced a gcc regression.

And yes, in both cases it seems to be a compiler bug, but I'm not
convinced it's a good idea to fix a clang bug by introducing a gcc
one.

Anyway, I think the real hint here is that 32-bit reload.

Lookie here:

  register unsigned int __asm_call_sp asm("esp");
  #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)

yeah, that's just garbage. It sure as hell should not be "unsigned int".

Yeah. yeah, gcc shouldn't do that insane reload in the first place,
but once that gcc bug has triggered, then the "unsigned int" is what
makes the code go really bad.

I bet that changing it to "unsigned long" will just fix things.

Josh?

            Linus

[toc] | [next] | [standalone]

#1741705

On Thu, Sep 28, 2017 at 09:21:07AM -0700, Linus Torvalds wrote:
> On Thu, Sep 28, 2017 at 12:47 AM, kernel test robot
> <xiaolong.ye@intel.com> wrote:
> >
> > [   10.587519] RIP: 0010:compat_sock_ioctl+0xfea/0x103e
> > [   10.587974] RSP: 0000:0000000000277d78 EFLAGS: 00010283
> > [   10.588448] RAX: 0000000000277d78 RBX: 0000000000008933 RCX: ffff8800141a8000
> > [   10.589103] RDX: 0000000000000020 RSI: 00000000fffbea00 RDI: 00000000fffbea50
> > [   10.589757] RBP: ffffc90000277e18 R08: fffbea50fffbea34 R09: ffffffff814a68c9
> > [   10.590407] R10: ffffff9c00000002 R11: 00000000fffbea50 R12: 0000000000000000
> > [   10.591056] R13: ffff880012c8c880 R14: 00000000fffbea50 R15: 00000000fffbea00
> > [   10.591708] FS:  0000000000000000(0000) GS:ffff880019a00000(0063) knlGS:00000000f7fab9a0
> > [   10.592446] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> > [   10.592973] CR2: 0000000000277d68 CR3: 000000001807f000 CR4: 00000000000006b0
> > [   10.593623] Call Trace:
> > [   10.593858] Code: 02 0f ff 65 48 8b 04 25 80 d1 00 00 48 8b 80 28 25 00 00 48 83 e8 20 49 39 c7 77 34 89 e0 4c 89 f7 4c 89 fe ba 20 00 00 00 89 c4 <e8> b3 52 05 00 85 c0 74 22 eb 1a 4c 89 fa 89 de 4c 89 ef e8 c6
> > [   10.595705] Kernel panic - not syncing: Machine halted.
> 
> That is some _funky_ code, and yes, this may well be triggered by the
> inline asm changes.
> 
> The code decodes to (after ignoring a few bytes at the beginning that
> were in the middle of an instruction)
> 
>    0: 65 48 8b 04 25 80 d1 mov    %gs:0xd180,%rax
>    7: 00 00
>    9: 48 8b 80 28 25 00 00 mov    0x2528(%rax),%rax
>   10: 48 83 e8 20          sub    $0x20,%rax
>   14: 49 39 c7              cmp    %rax,%r15
>   17: 77 34                ja     0x4d
>   19: 89 e0                mov    %esp,%eax
>   1b: 4c 89 f7              mov    %r14,%rdi
>   1e: 4c 89 fe              mov    %r15,%rsi
>   21: ba 20 00 00 00        mov    $0x20,%edx
>   26: 89 c4                mov    %eax,%esp
>   28:* e8 b3 52 05 00        callq  0x552e0 <-- trapping instruction
>   2d: 85 c0                test   %eax,%eax
>   2f: 74 22                je     0x53
>   31: eb 1a                jmp    0x4d
>   33: 4c 89 fa              mov    %r15,%rdx
>   36: 89 de                mov    %ebx,%esi
>   38: 4c 89 ef              mov    %r13,%rdi
> 
> and it's worth noting that insane
> 
>      mov    %eax,%esp
> 
> instruction, and how RAX (and RSP) both have that bad value of
> 0000000000277d78 in them.
> 
> So double fault is correct - we've corrupted the stack.
> 
> And NOTE! It's reloading 32 bits, not 64 bits, and that's the basic bug there.
> 
> I do note that when I build a kernel, I do see that pattern of
> 
>     movl    $32, %edx
>     call <something>
> 
> and in every case it's a a call to a user copy. One is "call
> _copy_from_user", while the other ones are all the
> alternative_call_2() in copy_user_generic().
> 
> Judging by the offset within the function, and judging by the bug,
> it's almost certainly that alternative_call_2() case.
> 
> So it does sound like the clang fix has now introduced a gcc regression.
> 
> And yes, in both cases it seems to be a compiler bug, but I'm not
> convinced it's a good idea to fix a clang bug by introducing a gcc
> one.
> 
> Anyway, I think the real hint here is that 32-bit reload.
> 
> Lookie here:
> 
>   register unsigned int __asm_call_sp asm("esp");
>   #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)
> 
> yeah, that's just garbage. It sure as hell should not be "unsigned int".
> 
> Yeah. yeah, gcc shouldn't do that insane reload in the first place,
> but once that gcc bug has triggered, then the "unsigned int" is what
> makes the code go really bad.
> 
> I bet that changing it to "unsigned long" will just fix things.
> 
> Josh?

Agreed, changing it to "unsigned long" and "rsp" will probably fix it.

I had made it "unsigned int" because of a clang issue with "unsigned
long":

    CC      arch/x86/entry/vdso/vdso32/vclock_gettime.o
  In file included from arch/x86/entry/vdso/vdso32/vclock_gettime.c:32:
  In file included from arch/x86/entry/vdso/vdso32/../vclock_gettime.c:15:
  In file included from ./arch/x86/include/asm/vgtod.h:5:
  In file included from ./include/linux/clocksource.h:12:
  In file included from ./include/linux/timex.h:56:
  In file included from ./include/uapi/linux/timex.h:56:
  In file included from ./include/linux/time.h:5:
  In file included from ./include/linux/seqlock.h:35:
  In file included from ./include/linux/spinlock.h:50:
  In file included from ./include/linux/preempt.h:10:
  In file included from ./include/linux/list.h:8:
  In file included from ./include/linux/kernel.h:10:
  In file included from ./include/linux/bitops.h:37:
  In file included from ./arch/x86/include/asm/bitops.h:16:
  In file included from ./arch/x86/include/asm/alternative.h:9:
  ./arch/x86/include/asm/asm.h:142:42: error: register 'rsp' unsuitable for global register variables on this target
  register unsigned long __asm_call_sp asm("rsp");

And I think we saw the same error in the realmode code.

So we may need to tweak the macro a bit.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1741711

On Thu, Sep 28, 2017 at 11:44:22AM -0500, Josh Poimboeuf wrote:
> Agreed, changing it to "unsigned long" and "rsp" will probably fix it.
> 
> I had made it "unsigned int" because of a clang issue with "unsigned
> long":
> 
>     CC      arch/x86/entry/vdso/vdso32/vclock_gettime.o
>   In file included from arch/x86/entry/vdso/vdso32/vclock_gettime.c:32:
>   In file included from arch/x86/entry/vdso/vdso32/../vclock_gettime.c:15:
>   In file included from ./arch/x86/include/asm/vgtod.h:5:
>   In file included from ./include/linux/clocksource.h:12:
>   In file included from ./include/linux/timex.h:56:
>   In file included from ./include/uapi/linux/timex.h:56:
>   In file included from ./include/linux/time.h:5:
>   In file included from ./include/linux/seqlock.h:35:
>   In file included from ./include/linux/spinlock.h:50:
>   In file included from ./include/linux/preempt.h:10:
>   In file included from ./include/linux/list.h:8:
>   In file included from ./include/linux/kernel.h:10:
>   In file included from ./include/linux/bitops.h:37:
>   In file included from ./arch/x86/include/asm/bitops.h:16:
>   In file included from ./arch/x86/include/asm/alternative.h:9:
>   ./arch/x86/include/asm/asm.h:142:42: error: register 'rsp' unsuitable for global register variables on this target
>   register unsigned long __asm_call_sp asm("rsp");
> 
> And I think we saw the same error in the realmode code.
> 
> So we may need to tweak the macro a bit.

Going to try the following patch.

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index c1eadbaf1115..30c3c9ac784a 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -11,10 +11,12 @@
 # define __ASM_FORM_COMMA(x) " " #x ","
 #endif
 
-#ifdef CONFIG_X86_32
+#ifndef __x86_64__
+/* 32 bit */
 # define __ASM_SEL(a,b) __ASM_FORM(a)
 # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(a)
 #else
+/* 64 bit */
 # define __ASM_SEL(a,b) __ASM_FORM(b)
 # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(b)
 #endif
@@ -139,7 +141,7 @@
  * gets set up by the containing function.  If you forget to do this, objtool
  * may print a "call without frame pointer save/setup" warning.
  */
-register unsigned int __asm_call_sp asm("esp");
+register unsigned long __asm_call_sp asm(_ASM_SP);
 #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)
 #endif
 

[toc] | [prev] | [next] | [standalone]

#1741786

On Thu, Sep 28, 2017 at 12:01:21PM -0500, Josh Poimboeuf wrote:
> On Thu, Sep 28, 2017 at 11:44:22AM -0500, Josh Poimboeuf wrote:
> > Agreed, changing it to "unsigned long" and "rsp" will probably fix it.
> > 
> > I had made it "unsigned int" because of a clang issue with "unsigned
> > long":
> > 
> >     CC      arch/x86/entry/vdso/vdso32/vclock_gettime.o
> >   In file included from arch/x86/entry/vdso/vdso32/vclock_gettime.c:32:
> >   In file included from arch/x86/entry/vdso/vdso32/../vclock_gettime.c:15:
> >   In file included from ./arch/x86/include/asm/vgtod.h:5:
> >   In file included from ./include/linux/clocksource.h:12:
> >   In file included from ./include/linux/timex.h:56:
> >   In file included from ./include/uapi/linux/timex.h:56:
> >   In file included from ./include/linux/time.h:5:
> >   In file included from ./include/linux/seqlock.h:35:
> >   In file included from ./include/linux/spinlock.h:50:
> >   In file included from ./include/linux/preempt.h:10:
> >   In file included from ./include/linux/list.h:8:
> >   In file included from ./include/linux/kernel.h:10:
> >   In file included from ./include/linux/bitops.h:37:
> >   In file included from ./arch/x86/include/asm/bitops.h:16:
> >   In file included from ./arch/x86/include/asm/alternative.h:9:
> >   ./arch/x86/include/asm/asm.h:142:42: error: register 'rsp' unsuitable for global register variables on this target
> >   register unsigned long __asm_call_sp asm("rsp");
> > 
> > And I think we saw the same error in the realmode code.
> > 
> > So we may need to tweak the macro a bit.
> 
> Going to try the following patch.
> 
> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
> index c1eadbaf1115..30c3c9ac784a 100644
> --- a/arch/x86/include/asm/asm.h
> +++ b/arch/x86/include/asm/asm.h
> @@ -11,10 +11,12 @@
>  # define __ASM_FORM_COMMA(x) " " #x ","
>  #endif
>  
> -#ifdef CONFIG_X86_32
> +#ifndef __x86_64__
> +/* 32 bit */
>  # define __ASM_SEL(a,b) __ASM_FORM(a)
>  # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(a)
>  #else
> +/* 64 bit */
>  # define __ASM_SEL(a,b) __ASM_FORM(b)
>  # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(b)
>  #endif
> @@ -139,7 +141,7 @@
>   * gets set up by the containing function.  If you forget to do this, objtool
>   * may print a "call without frame pointer save/setup" warning.
>   */
> -register unsigned int __asm_call_sp asm("esp");
> +register unsigned long __asm_call_sp asm(_ASM_SP);
>  #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)
>  #endif

Confirmed that this patch works on both compilers, and fixes GCC 4.4.

GCC 4.4 before:

  ffffffff8147461d:       89 e0                   mov    %esp,%eax
  ffffffff8147461f:       4c 89 f7                mov    %r14,%rdi
  ffffffff81474622:       4c 89 fe                mov    %r15,%rsi
  ffffffff81474625:       ba 20 00 00 00          mov    $0x20,%edx
  ffffffff8147462a:       89 c4                   mov    %eax,%esp
  ffffffff8147462c:       e8 bf 52 05 00          callq  ffffffff814c98f0 <copy_user_generic_unrolled>

after:

  ffffffff8147461e:       48 89 e0                mov    %rsp,%rax
  ffffffff81474621:       4c 89 f7                mov    %r14,%rdi
  ffffffff81474624:       4c 89 fe                mov    %r15,%rsi
  ffffffff81474627:       ba 20 00 00 00          mov    $0x20,%edx
  ffffffff8147462c:       48 89 c4                mov    %rax,%rsp
  ffffffff8147462f:       e8 cc 52 05 00          callq  ffffffff814c9900 <copy_user_generic_unrolled>

It still has the "back up and restore the stack pointer just for the fun
of it" thing, but at least the corruption is gone.

Will finalize the patch and send it along to tip.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1741826 — [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4

The kernel test bot reported that

  f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")

is causing double faults in a kernel compiled with GCC 4.4.  The issue
is with this code:

  register unsigned int __asm_call_sp asm("esp");
  #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)

Even on a 64-bit kernel, it's using ESP instead of RSP.  That causes GCC
to produce the following:

  ffffffff8147461d:       89 e0                   mov    %esp,%eax
  ffffffff8147461f:       4c 89 f7                mov    %r14,%rdi
  ffffffff81474622:       4c 89 fe                mov    %r15,%rsi
  ffffffff81474625:       ba 20 00 00 00          mov    $0x20,%edx
  ffffffff8147462a:       89 c4                   mov    %eax,%esp
  ffffffff8147462c:       e8 bf 52 05 00          callq  ffffffff814c98f0 <copy_user_generic_unrolled>

Despite the absurdity of it backing up and restoring the stack pointer
for no reason, the bug is actually the fact that it's only backing up
and restoring the lower 32 bits of the stack pointer.  The upper 32 bits
are getting cleared out, corrupting the stack pointer.

So change the '__asm_call_sp' register variable to be associated with
the actual full-size stack pointer.

This also requires changing the __ASM_SEL() macro to be based on the
actual compiled arch size, rather than the CONFIG value, because
CONFIG_X86_64 compiles some files with '-m32' (e.g., realmode and vdso).
Otherwise Clang fails to build the kernel because it complains about the
use of a 64-bit register (RSP) in a 32-bit file.

Reported-by: kernel test robot <xiaolong.ye@intel.com>
Fixes: f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/asm.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index c1eadbaf1115..30c3c9ac784a 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -11,10 +11,12 @@
 # define __ASM_FORM_COMMA(x) " " #x ","
 #endif
 
-#ifdef CONFIG_X86_32
+#ifndef __x86_64__
+/* 32 bit */
 # define __ASM_SEL(a,b) __ASM_FORM(a)
 # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(a)
 #else
+/* 64 bit */
 # define __ASM_SEL(a,b) __ASM_FORM(b)
 # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(b)
 #endif
@@ -139,7 +141,7 @@
  * gets set up by the containing function.  If you forget to do this, objtool
  * may print a "call without frame pointer save/setup" warning.
  */
-register unsigned int __asm_call_sp asm("esp");
+register unsigned long __asm_call_sp asm(_ASM_SP);
 #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)
 #endif
 
-- 
2.13.5

[toc] | [prev] | [next] | [standalone]

#1741869 — Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4

On Thu, Sep 28, 2017 at 2:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Reported-by: kernel test robot <xiaolong.ye@intel.com>
> Fixes: f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

Side note: it's not like I personally need the credit, but in general
I really want people to pick up on who debugged the code and pointed
to the solution. That's often more of the work than the fix itself.

The kernel test robot report looked to be ignored as a "gcc-4.4 is too
old to worry about" thing. People who then step up and analyze the
problem are rare as it is. They need to be credited in the commit
logs.

We don't have any fixed format for that, but it's pretty free-form. So
we have tags like

  Root-caused-by:
  Diagnosed-by:
  Analyzed-by:
  Debugged-by:
  Bisected-by:
  Fix-suggested-by:

etc for giving credit to people who figured out some part of a bug
(and, having grepped for this, we also a _shitload_ of miss-spellings
of various things ;)

                 Linus

[toc] | [prev] | [next] | [standalone]

#1741892 — Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4

On Thu, Sep 28, 2017 at 04:53:09PM -0700, Linus Torvalds wrote:
> On Thu, Sep 28, 2017 at 2:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > Reported-by: kernel test robot <xiaolong.ye@intel.com>
> > Fixes: f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Side note: it's not like I personally need the credit, but in general
> I really want people to pick up on who debugged the code and pointed
> to the solution. That's often more of the work than the fix itself.
> 
> The kernel test robot report looked to be ignored as a "gcc-4.4 is too
> old to worry about" thing. People who then step up and analyze the
> problem are rare as it is. They need to be credited in the commit
> logs.
> 
> We don't have any fixed format for that, but it's pretty free-form. So
> we have tags like
> 
>   Root-caused-by:
>   Diagnosed-by:
>   Analyzed-by:
>   Debugged-by:
>   Bisected-by:
>   Fix-suggested-by:
> 
> etc for giving credit to people who figured out some part of a bug
> (and, having grepped for this, we also a _shitload_ of miss-spellings
> of various things ;)

Indeed, credit is important and I try to give it where it's due.  Sorry
for the snub!  I anoint you with:

Debugged-by: Linus Torvalds <torvalds@linux-foundation.org>

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1741961 — Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Thu, Sep 28, 2017 at 04:53:09PM -0700, Linus Torvalds wrote:
> > On Thu, Sep 28, 2017 at 2:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > Reported-by: kernel test robot <xiaolong.ye@intel.com>
> > > Fixes: f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > 
> > Side note: it's not like I personally need the credit, but in general
> > I really want people to pick up on who debugged the code and pointed
> > to the solution. That's often more of the work than the fix itself.
> > 
> > The kernel test robot report looked to be ignored as a "gcc-4.4 is too
> > old to worry about" thing. People who then step up and analyze the
> > problem are rare as it is. They need to be credited in the commit
> > logs.
> > 
> > We don't have any fixed format for that, but it's pretty free-form. So
> > we have tags like
> > 
> >   Root-caused-by:
> >   Diagnosed-by:
> >   Analyzed-by:
> >   Debugged-by:
> >   Bisected-by:
> >   Fix-suggested-by:
> > 
> > etc for giving credit to people who figured out some part of a bug
> > (and, having grepped for this, we also a _shitload_ of miss-spellings
> > of various things ;)
> 
> Indeed, credit is important and I try to give it where it's due.  Sorry
> for the snub!  I anoint you with:
> 
> Debugged-by: Linus Torvalds <torvalds@linux-foundation.org>

When applying the fix I went with:

  Diagnosed-by: Linus Torvalds <torvalds@linux-foundation.org>

Because I think 'diagnosing' a bug is a higher category, which implies debugging. 

( Sometimes we refer to 'debugging a bug' as the reporter adding printks on 
  request and printing out key state that helps understand the bug. It does not 
  necessarily imply root-causing the bug. )

Also note that I added a "Reported-and-Bisected-by:" tag for the ktest robot, to 
further credit the fact that in addition to reporting a kernel crash, a specific 
commit was bisected to as well.

I'll wait for another round of ktest robot testing to make sure the crash is 
indeed fixed.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1742041 — Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4

On 09/29, Ingo Molnar wrote:
>
>* Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
>> On Thu, Sep 28, 2017 at 04:53:09PM -0700, Linus Torvalds wrote:
>> > On Thu, Sep 28, 2017 at 2:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > >
>> > > Reported-by: kernel test robot <xiaolong.ye@intel.com>
>> > > Fixes: f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")
>> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> > 
>> > Side note: it's not like I personally need the credit, but in general
>> > I really want people to pick up on who debugged the code and pointed
>> > to the solution. That's often more of the work than the fix itself.
>> > 
>> > The kernel test robot report looked to be ignored as a "gcc-4.4 is too
>> > old to worry about" thing. People who then step up and analyze the
>> > problem are rare as it is. They need to be credited in the commit
>> > logs.
>> > 
>> > We don't have any fixed format for that, but it's pretty free-form. So
>> > we have tags like
>> > 
>> >   Root-caused-by:
>> >   Diagnosed-by:
>> >   Analyzed-by:
>> >   Debugged-by:
>> >   Bisected-by:
>> >   Fix-suggested-by:
>> > 
>> > etc for giving credit to people who figured out some part of a bug
>> > (and, having grepped for this, we also a _shitload_ of miss-spellings
>> > of various things ;)
>> 
>> Indeed, credit is important and I try to give it where it's due.  Sorry
>> for the snub!  I anoint you with:
>> 
>> Debugged-by: Linus Torvalds <torvalds@linux-foundation.org>
>
>When applying the fix I went with:
>
>  Diagnosed-by: Linus Torvalds <torvalds@linux-foundation.org>
>
>Because I think 'diagnosing' a bug is a higher category, which implies debugging. 
>
>( Sometimes we refer to 'debugging a bug' as the reporter adding printks on 
>  request and printing out key state that helps understand the bug. It does not 
>  necessarily imply root-causing the bug. )
>
>Also note that I added a "Reported-and-Bisected-by:" tag for the ktest robot, to 
>further credit the fact that in addition to reporting a kernel crash, a specific 
>commit was bisected to as well.
>
>I'll wait for another round of ktest robot testing to make sure the crash is 
>indeed fixed.

The panic is gone with the fix patch for 4 times test.

Tested-by: Ye Xiaolong <xiaolong.ye@intel.com>

Thanks,
Xiaolong
>
>Thanks,
>
>	Ingo

[toc] | [prev] | [next] | [standalone]

#1741957 — Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Sep 28, 2017 at 2:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > Reported-by: kernel test robot <xiaolong.ye@intel.com>
> > Fixes: f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Side note: it's not like I personally need the credit, but in general
> I really want people to pick up on who debugged the code and pointed
> to the solution. That's often more of the work than the fix itself.
> 
> The kernel test robot report looked to be ignored as a "gcc-4.4 is too
> old to worry about" thing. [...]

No, and sorry if my first reply grumbling about how old GCC 4.4 is sounded that 
way! We have to live with compiler bugs no matter how old the compiler is, the 
release cycles are decoupled to such a degree and external tooling propagates with 
such high latencies that that's the only sane thing to do.

We also officially support GCC 3.2 and later compilers. Had this regression not 
been resolved within a week or so I was fully ready to queue up a revert commit, 
no questions asked.

Plus it's not just that it's a regression, but adding support for a different 
compiler is about the _worst_ possible reason to break working compilers ...

> [...] People who then step up and analyze the problem are rare as it is. They 
> need to be credited in the commit logs.
> 
> We don't have any fixed format for that, but it's pretty free-form. So
> we have tags like
> 
>   Root-caused-by:
>   Diagnosed-by:
>   Analyzed-by:
>   Debugged-by:
>   Bisected-by:
>   Fix-suggested-by:
> 
> etc for giving credit to people who figured out some part of a bug
> (and, having grepped for this, we also a _shitload_ of miss-spellings
> of various things ;)

Yeah, I sometimes add such tags, but not routinely. I'll lower the threshold for 
adding such tags, to create further incentives for people to help debug crashes.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1742202 — Re: [PATCH] x86/asm: Fix inline asm call constraints for GCC 4.4

On Fri, Sep 29, 2017 at 12:51 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Thu, Sep 28, 2017 at 2:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> The kernel test robot report looked to be ignored as a "gcc-4.4 is too
>> old to worry about" thing. [...]
>
> No, and sorry if my first reply grumbling about how old GCC 4.4 is sounded that
> way! We have to live with compiler bugs no matter how old the compiler is, the
> release cycles are decoupled to such a degree and external tooling propagates with
> such high latencies that that's the only sane thing to do.
>
> We also officially support GCC 3.2 and later compilers. Had this regression not
> been resolved within a week or so I was fully ready to queue up a revert commit,
> no questions asked.

FWIW, we had a discussion about which compiler versions actually still work
earlier this year: We concluded that gcc-4.0 and earlier have been broken
for a while without anyone caring. gcc-4.1 support is kept working in certain
configurations primarily due to Geert using it for build testing, but my tests
across multiple architectures showed that gcc-4.3 is needed on most
other architectures already. Some architectures need  even newer ones, and
some features need compiler support that was added much later of course.

         Arnd

[toc] | [prev] | [next] | [standalone]

#1742014 — [tip:x86/urgent] x86/asm: Fix inline asm call constraints for GCC 4.4

Commit-ID:  b9545e75894b4866c62b36682527f5df1394ac58
Gitweb:     https://git.kernel.org/tip/b9545e75894b4866c62b36682527f5df1394ac58
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Thu, 28 Sep 2017 16:58:26 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 29 Sep 2017 09:59:17 +0200

x86/asm: Fix inline asm call constraints for GCC 4.4

The kernel test bot (run by Xiaolong Ye) reported that the following commit:

  f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")

is causing double faults in a kernel compiled with GCC 4.4.

Linus subsequently diagnosed the crash pattern and the buggy commit and found that
the issue is with this code:

  register unsigned int __asm_call_sp asm("esp");
  #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)

Even on a 64-bit kernel, it's using ESP instead of RSP.  That causes GCC
to produce the following bogus code:

  ffffffff8147461d:       89 e0                   mov    %esp,%eax
  ffffffff8147461f:       4c 89 f7                mov    %r14,%rdi
  ffffffff81474622:       4c 89 fe                mov    %r15,%rsi
  ffffffff81474625:       ba 20 00 00 00          mov    $0x20,%edx
  ffffffff8147462a:       89 c4                   mov    %eax,%esp
  ffffffff8147462c:       e8 bf 52 05 00          callq  ffffffff814c98f0 <copy_user_generic_unrolled>

Despite the absurdity of it backing up and restoring the stack pointer
for no reason, the bug is actually the fact that it's only backing up
and restoring the lower 32 bits of the stack pointer.  The upper 32 bits
are getting cleared out, corrupting the stack pointer.

So change the '__asm_call_sp' register variable to be associated with
the actual full-size stack pointer.

This also requires changing the __ASM_SEL() macro to be based on the
actual compiled arch size, rather than the CONFIG value, because
CONFIG_X86_64 compiles some files with '-m32' (e.g., realmode and vdso).
Otherwise Clang fails to build the kernel because it complains about the
use of a 64-bit register (RSP) in a 32-bit file.

Reported-and-Bisected-by: kernel test robot <xiaolong.ye@intel.com>
Diagnosed-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dmitriy Vyukov <dvyukov@google.com>
Cc: LKP <lkp@01.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Miguel Bernal Marin <miguel.bernal.marin@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")
Link: http://lkml.kernel.org/r/20170928215826.6sdpmwtkiydiytim@treble
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/asm.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index c1eadba..30c3c9a 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -11,10 +11,12 @@
 # define __ASM_FORM_COMMA(x) " " #x ","
 #endif
 
-#ifdef CONFIG_X86_32
+#ifndef __x86_64__
+/* 32 bit */
 # define __ASM_SEL(a,b) __ASM_FORM(a)
 # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(a)
 #else
+/* 64 bit */
 # define __ASM_SEL(a,b) __ASM_FORM(b)
 # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(b)
 #endif
@@ -139,7 +141,7 @@
  * gets set up by the containing function.  If you forget to do this, objtool
  * may print a "call without frame pointer save/setup" warning.
  */
-register unsigned int __asm_call_sp asm("esp");
+register unsigned long __asm_call_sp asm(_ASM_SP);
 #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)
 #endif
 

[toc] | [prev] | [next] | [standalone]

#1742056 — [tip:x86/urgent] x86/asm: Fix inline asm call constraints for GCC 4.4

Commit-ID:  520a13c530aeb5f63e011d668c42db1af19ed349
Gitweb:     https://git.kernel.org/tip/520a13c530aeb5f63e011d668c42db1af19ed349
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Thu, 28 Sep 2017 16:58:26 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 29 Sep 2017 13:15:44 +0200

x86/asm: Fix inline asm call constraints for GCC 4.4

The kernel test bot (run by Xiaolong Ye) reported that the following commit:

  f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")

is causing double faults in a kernel compiled with GCC 4.4.

Linus subsequently diagnosed the crash pattern and the buggy commit and found that
the issue is with this code:

  register unsigned int __asm_call_sp asm("esp");
  #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)

Even on a 64-bit kernel, it's using ESP instead of RSP.  That causes GCC
to produce the following bogus code:

  ffffffff8147461d:       89 e0                   mov    %esp,%eax
  ffffffff8147461f:       4c 89 f7                mov    %r14,%rdi
  ffffffff81474622:       4c 89 fe                mov    %r15,%rsi
  ffffffff81474625:       ba 20 00 00 00          mov    $0x20,%edx
  ffffffff8147462a:       89 c4                   mov    %eax,%esp
  ffffffff8147462c:       e8 bf 52 05 00          callq  ffffffff814c98f0 <copy_user_generic_unrolled>

Despite the absurdity of it backing up and restoring the stack pointer
for no reason, the bug is actually the fact that it's only backing up
and restoring the lower 32 bits of the stack pointer.  The upper 32 bits
are getting cleared out, corrupting the stack pointer.

So change the '__asm_call_sp' register variable to be associated with
the actual full-size stack pointer.

This also requires changing the __ASM_SEL() macro to be based on the
actual compiled arch size, rather than the CONFIG value, because
CONFIG_X86_64 compiles some files with '-m32' (e.g., realmode and vdso).
Otherwise Clang fails to build the kernel because it complains about the
use of a 64-bit register (RSP) in a 32-bit file.

Reported-and-Bisected-and-Tested-by: kernel test robot <xiaolong.ye@intel.com>
Diagnosed-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dmitriy Vyukov <dvyukov@google.com>
Cc: LKP <lkp@01.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Miguel Bernal Marin <miguel.bernal.marin@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")
Link: http://lkml.kernel.org/r/20170928215826.6sdpmwtkiydiytim@treble
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/asm.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index c1eadba..30c3c9a 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -11,10 +11,12 @@
 # define __ASM_FORM_COMMA(x) " " #x ","
 #endif
 
-#ifdef CONFIG_X86_32
+#ifndef __x86_64__
+/* 32 bit */
 # define __ASM_SEL(a,b) __ASM_FORM(a)
 # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(a)
 #else
+/* 64 bit */
 # define __ASM_SEL(a,b) __ASM_FORM(b)
 # define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(b)
 #endif
@@ -139,7 +141,7 @@
  * gets set up by the containing function.  If you forget to do this, objtool
  * may print a "call without frame pointer save/setup" warning.
  */
-register unsigned int __asm_call_sp asm("esp");
+register unsigned long __asm_call_sp asm(_ASM_SP);
 #define ASM_CALL_CONSTRAINT "+r" (__asm_call_sp)
 #endif
 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web