next: Crashes in x86 images due to 'locking/rwsem, x86: Clean up ____down_write()'

next: Crashes in x86 images due to 'locking/rwsem, x86: Clean up ____down_write()'

[Guenter Roeck]

Borislav,

your patch 'locking/rwsem, x86: Clean up ____down_write()' causes various
crashes in x86 qemu tests.

BUG: unable to handle kernel paging request at 80000015
IP: [<c1846353>] down_write+0x23/0x30

Call Trace:
  [<c1374cd5>] crypto_larval_kill+0x15/0x70
  [<c13761c6>] crypto_wait_for_test+0x46/0x70
  [<c137660c>] crypto_register_alg+0x5c/0x70
  [<c1376655>] crypto_register_algs+0x35/0x80
  [<c1c1e815>] ? hmac_module_init+0xf/0xf
  [<c1c1e828>] crypto_null_mod_init+0x13/0x41
  [<c100047b>] do_one_initcall+0x3b/0x150
  [ ... ]


BUG: unable to handle kernel paging request at 80000015
IP: [<c1848e43>] down_write+0x23/0x30

Call Trace:
  [<c10530d5>] copy_process.part.52+0xba5/0x15c0
  [<c184a458>] ? _raw_spin_unlock_bh+0x18/0x20
  [<c1053c9d>] _do_fork+0xcd/0x380
  [<c105403c>] SyS_clone+0x2c/0x30
  [<c100174a>] do_int80_syscall_32+0x5a/0xa0
  [<c184ac5e>] entry_INT80_32+0x2a/0x2a

BUG: unable to handle kernel paging request at 80000015
IP: [<c1848e43>] down_write+0x23/0x30

Call Trace:
  [<c1155f30>] unlink_file_vma+0x30/0x50
  [<c1150ed3>] free_pgtables+0x33/0xc0
  [<c115719e>] exit_mmap+0x8e/0xe0
  [<c105220b>] mmput+0x2b/0xa0
  [<c117f452>] flush_old_exec+0x312/0x650
  [<c11c3c0d>] load_elf_binary+0x2ad/0x1080
  [<c114f00c>] ? __get_user_pages+0x3c/0x60
  [<c114f08f>] ? get_user_pages_remote+0x5f/0x70
  [<c13c6a11>] ? _copy_from_user+0x31/0x50
  [<c117f00c>] search_binary_handler+0x7c/0x1b0
  [<c118059b>] do_execveat_common+0x4bb/0x650
  [<c1180754>] do_execve+0x24/0x30
  [<c118094b>] SyS_execve+0x1b/0x20
  [<c100174a>] do_int80_syscall_32+0x5a/0xa0
  [<c184ac5e>] entry_INT80_32+0x2a/0x2a

Reverting the patch fixes the problem.

Complete logs are available at http://kerneltests.org/builders/qemu-x86-next.
Scripts, configuration, and root file system used are available at
	https://github.com/groeck/linux-build-test/tree/master/rootfs/x86

Bisect log is attached.

Guenter

---
Bisect log:

# bad: [13425f1bf7f9f28fcef53057303319d5afb907f7] Add linux-next specific files for 20160511
# good: [44549e8f5eea4e0a41b487b63e616cb089922b99] Linux 4.6-rc7
git bisect start 'HEAD' 'v4.6-rc7'
# good: [b6cf27d48f370bf2d42888921632ae05340aaca9] Merge remote-tracking branch 'crypto/master'
git bisect good b6cf27d48f370bf2d42888921632ae05340aaca9
# bad: [607563e7793b7c4f3ab134dc200552a555ca5cb2] Merge remote-tracking branch 'tip/auto-latest'
git bisect bad 607563e7793b7c4f3ab134dc200552a555ca5cb2
# good: [05454bc3dd6d8c4cff684ea881d79db952030075] Merge remote-tracking branch 'block/for-next'
git bisect good 05454bc3dd6d8c4cff684ea881d79db952030075
# good: [3ed15da0d55d9147f6434fe57db60a5b4059cbfd] Merge remote-tracking branch 'spi/for-next'
git bisect good 3ed15da0d55d9147f6434fe57db60a5b4059cbfd
# bad: [25ea4e608611c03ad9829a727f6cc198db952d06] Merge branch 'perf/core'
git bisect bad 25ea4e608611c03ad9829a727f6cc198db952d06
# good: [f127fa098d76444c7a47b2f009356979492d77cd] perf/x86/intel/pt: Add IP filtering register/CPUID bits
git bisect good f127fa098d76444c7a47b2f009356979492d77cd
# good: [13a00bc35b2a9f8b750a292dcc84111bdfb1edd4] Merge branch 'efi/core'
git bisect good 13a00bc35b2a9f8b750a292dcc84111bdfb1edd4
# bad: [0b749d9316aa32dc3fe67d7c0b4fb650873709aa] Merge branch 'locking/rwsem'
git bisect bad 0b749d9316aa32dc3fe67d7c0b4fb650873709aa
# good: [a1cc5bcfcfca0b99f009b117785142dbdc3b87a3] locking/atomics: Flip atomic_fetch_or() arguments
git bisect good a1cc5bcfcfca0b99f009b117785142dbdc3b87a3
# good: [00fb16e26ac8559e69c3bb14284f4a548d28ee0d] locking/rwsem, x86: Add frame annotation for call_rwsem_down_write_failed_killable()
git bisect good 00fb16e26ac8559e69c3bb14284f4a548d28ee0d
# good: [e3825ba1af3a27d7522c9f5f929f5a13b8b138ae] irqchip/gic-v3: Add support for partitioned PPIs
git bisect good e3825ba1af3a27d7522c9f5f929f5a13b8b138ae
# good: [5e4c588054d52d1b25633c6074c5f3c6228e26ed] Merge branch 'irq/core'
git bisect good 5e4c588054d52d1b25633c6074c5f3c6228e26ed
# bad: [71c01930b42e5dd65d4820dea116bcbe95a0b768] locking/rwsem, x86: Clean up ____down_write()
git bisect bad 71c01930b42e5dd65d4820dea116bcbe95a0b768
# first bad commit: [71c01930b42e5dd65d4820dea116bcbe95a0b768] locking/rwsem, x86: Clean up ____down_write()

Re: next: Crashes in x86 images due to 'locking/rwsem, x86: Clean up ____down_write()'

[Borislav Petkov]

On Thu, May 12, 2016 at 06:34:29AM -0700, Guenter Roeck wrote:
> Borislav,
> 
> your patch 'locking/rwsem, x86: Clean up ____down_write()' causes various
> crashes in x86 qemu tests.

Thanks for the report, let me take a look.

@Ingo: can you please back this one out of the lineup for the merge
window until I've sorted out the issue?

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: next: Crashes in x86 images due to 'locking/rwsem, x86: Clean up ____down_write()'

[Borislav Petkov]

On Thu, May 12, 2016 at 03:51:31PM +0200, Borislav Petkov wrote:
> On Thu, May 12, 2016 at 06:34:29AM -0700, Guenter Roeck wrote:
> > Borislav,
> > 
> > your patch 'locking/rwsem, x86: Clean up ____down_write()' causes various
> > crashes in x86 qemu tests.
> 
> Thanks for the report, let me take a look.
> 
> @Ingo: can you please back this one out of the lineup for the merge
> window until I've sorted out the issue?

Ok, I was able to reproduce:

BUG: unable to handle kernel NULL pointer dereference at 00000015
IP: [<c185e094>] down_write+0x24/0x30
*pde = 00000000 
Oops: 0002 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S      W       4.6.0-rc7-next-20160511-yocto-standard #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
task: f4d00000 ti: f4d08000 task.ti: f4d08000
EIP: 0060:[<c185e094>] EFLAGS: 00210282 CPU: 0
EIP is at down_write+0x24/0x30
EAX: f4d00000 EBX: f4f6d600 ECX: ffff0001 EDX: 00000001
ESI: 00000168 EDI: c1c2eb68 EBP: f4d09ef4 ESP: f4d09eec
 DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
CR0: 80050033 CR2: 00000015 CR3: 01ccb000 CR4: 000406d0

We fault here:

c185e070 <down_write>:
c185e070:       55                      push   %ebp
c185e071:       89 e5                   mov    %esp,%ebp
c185e073:       e8 20 2b 00 00          call   c1860b98 <mcount>
c185e078:       b9 01 00 ff ff          mov    $0xffff0001,%ecx
c185e07d:       89 c2                   mov    %eax,%edx
c185e07f:       f0 0f c1 08             lock xadd %ecx,(%eax)
c185e083:       66 85 c9                test   %cx,%cx
c185e086:       74 05                   je     c185e08d <down_write+0x1d>
c185e088:       e8 f7 31 b7 ff          call   c13d1284 <call_rwsem_down_write_failed>
c185e08d:       64 a1 48 59 cb c1       mov    %fs:0xc1cb5948,%eax
c185e093:       5d                      pop    %ebp
c185e094:       89 42 14                mov    %eax,0x14(%edx)		<--- HERE
c185e097:       c3                      ret
c185e098:       90                      nop
c185e099:       8d b4 26 00 00 00 00    lea    0x0(%esi,%eiz,1),%esi

and %edx is 1 (+ 0x14 gives the 00000015 deref addr).

But edx should contain sem. The code does:

        .loc 1 47 0
        movl    %eax, %edx      # sem, sem

	lock;   xadd      %ecx,(%eax)   # tmp91, sem

	call call_rwsem_down_write_failed

	mov    %eax,0x14(%edx)

and if something in that call clobbers %edx, boom! Now I need to think
about how to make gcc reload sem after

	LOCK_CONTENDED(sem, __down_write_trylock, __down_write);

for
	rwsem_set_owner(sem);

Btw, the hunk below seems to fix it. And the comment above those
{save,restore}_common_regs talk about "Save the C-clobbered registers
(%eax, %edx and %ecx)" but the only reg we're stashing is ecx.

Why aren't we stashing edx too?

Ingo, Peter?

---
diff --git a/arch/x86/lib/rwsem.S b/arch/x86/lib/rwsem.S
index a37462a23546..02240807e97a 100644
--- a/arch/x86/lib/rwsem.S
+++ b/arch/x86/lib/rwsem.S
@@ -33,10 +33,12 @@
  * value or just clobbered..
  */
 
-#define save_common_regs \
-       pushl %ecx
+#define save_common_regs       \
+       pushl %ecx;             \
+       pushl %edx
 
-#define restore_common_regs \
+#define restore_common_regs    \
+       popl %edx;              \
        popl %ecx
 
        /* Avoid uglifying the argument copying x86-64 needs to do. */

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[PATCH] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()

[Borislav Petkov]

Anyway, here's an actual patch with a commit message. Guenter, can you
give it a run please?

It does fix the issue here with your .config but I'd appreciate a
confirmation.

Thanks.

---
From: Borislav Petkov <bp@suse.de>

____down_write() calls a function to handle the slow path when the lock
is contended. But in order to be able to call a C function, one has to
stash all callee-clobbered registers. The 32-bit path saves only %ecx
for a reason unknown to me. However, after

  71c01930b42e ("locking/rwsem, x86: Clean up ____down_write()")

the useless dependency on edx was removed and this caused the following
splat:

  BUG: unable to handle kernel NULL pointer dereference at 00000015
  IP: [<c185e094>] down_write+0x24/0x30
  *pde = 00000000
  Oops: 0002 [#1] PREEMPT SMP
  Modules linked in:
  CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S      W       4.6.0-rc7-next-20160511-yocto-standard #1
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
  task: f4d00000 ti: f4d08000 task.ti: f4d08000
  EIP: 0060:[<c185e094>] EFLAGS: 00210282 CPU: 0
  EIP is at down_write+0x24/0x30
  EAX: f4d00000 EBX: f4f6d600 ECX: ffff0001 EDX: 00000001
  ESI: 00000168 EDI: c1c2eb68 EBP: f4d09ef4 ESP: f4d09eec
   DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
  CR0: 80050033 CR2: 00000015 CR3: 01ccb000 CR4: 000406d0

This happens because gcc decided to stash the pointer to @sem in edx (it
is not used in the inline asm anymore, thus free):

  movl    %eax, %edx      # sem, sem

  lock;   xadd      %ecx,(%eax)   # tmp91, sem
  test ...

  call call_rwsem_down_write_failed

  mov    %eax,0x14(%edx)

*before* the slow path happens and if we hit it on 32-bit, it can
clobber edx and we're staring at garbage value at deref time.

The simple fix is to save/restore edx too, around the slow path. We
don't need to stash eax because it is used in the slow path as the @sem
arg.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: x86@kernel.org
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/lib/rwsem.S | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/lib/rwsem.S b/arch/x86/lib/rwsem.S
index a37462a23546..02240807e97a 100644
--- a/arch/x86/lib/rwsem.S
+++ b/arch/x86/lib/rwsem.S
@@ -33,10 +33,12 @@
  * value or just clobbered..
  */
 
-#define save_common_regs \
-	pushl %ecx
+#define save_common_regs	\
+	pushl %ecx;		\
+	pushl %edx
 
-#define restore_common_regs \
+#define restore_common_regs	\
+	popl %edx;		\
 	popl %ecx
 
 	/* Avoid uglifying the argument copying x86-64 needs to do. */
-- 
2.7.3

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()

[Guenter Roeck]

On 05/12/2016 10:29 AM, Borislav Petkov wrote:
> Anyway, here's an actual patch with a commit message. Guenter, can you
> give it a run please?
>
> It does fix the issue here with your .config but I'd appreciate a
> confirmation.
>
> Thanks.
>
> ---
> From: Borislav Petkov <bp@suse.de>
>
> ____down_write() calls a function to handle the slow path when the lock
> is contended. But in order to be able to call a C function, one has to
> stash all callee-clobbered registers. The 32-bit path saves only %ecx
> for a reason unknown to me. However, after
>
>    71c01930b42e ("locking/rwsem, x86: Clean up ____down_write()")
>
> the useless dependency on edx was removed and this caused the following
> splat:
>
>    BUG: unable to handle kernel NULL pointer dereference at 00000015
>    IP: [<c185e094>] down_write+0x24/0x30
>    *pde = 00000000
>    Oops: 0002 [#1] PREEMPT SMP
>    Modules linked in:
>    CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S      W       4.6.0-rc7-next-20160511-yocto-standard #1
>    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
>    task: f4d00000 ti: f4d08000 task.ti: f4d08000
>    EIP: 0060:[<c185e094>] EFLAGS: 00210282 CPU: 0
>    EIP is at down_write+0x24/0x30
>    EAX: f4d00000 EBX: f4f6d600 ECX: ffff0001 EDX: 00000001
>    ESI: 00000168 EDI: c1c2eb68 EBP: f4d09ef4 ESP: f4d09eec
>     DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
>    CR0: 80050033 CR2: 00000015 CR3: 01ccb000 CR4: 000406d0
>
> This happens because gcc decided to stash the pointer to @sem in edx (it
> is not used in the inline asm anymore, thus free):
>
>    movl    %eax, %edx      # sem, sem
>
>    lock;   xadd      %ecx,(%eax)   # tmp91, sem
>    test ...
>
>    call call_rwsem_down_write_failed
>
>    mov    %eax,0x14(%edx)
>
> *before* the slow path happens and if we hit it on 32-bit, it can
> clobber edx and we're staring at garbage value at deref time.
>
> The simple fix is to save/restore edx too, around the slow path. We
> don't need to stash eax because it is used in the slow path as the @sem
> arg.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: x86@kernel.org
> Signed-off-by: Borislav Petkov <bp@suse.de>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>   arch/x86/lib/rwsem.S | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/lib/rwsem.S b/arch/x86/lib/rwsem.S
> index a37462a23546..02240807e97a 100644
> --- a/arch/x86/lib/rwsem.S
> +++ b/arch/x86/lib/rwsem.S
> @@ -33,10 +33,12 @@
>    * value or just clobbered..
>    */
>
> -#define save_common_regs \
> -	pushl %ecx
> +#define save_common_regs	\
> +	pushl %ecx;		\
> +	pushl %edx
>
> -#define restore_common_regs \
> +#define restore_common_regs	\
> +	popl %edx;		\
>   	popl %ecx
>
>   	/* Avoid uglifying the argument copying x86-64 needs to do. */
>

Re: [PATCH] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()

[Linus Torvalds]

On Thu, May 12, 2016 at 10:29 AM, Borislav Petkov <bp@suse.de> wrote:
> Anyway, here's an actual patch with a commit message. Guenter, can you
> give it a run please?

I think the commit message is misleading.

> ____down_write() calls a function to handle the slow path when the lock
> is contended. But in order to be able to call a C function, one has to
> stash all callee-clobbered registers. The 32-bit path saves only %ecx
> for a reason unknown to me.

Why claim that it's unknown? You know exactly what the reason was:

> the useless dependency on edx was removed and this caused the following
> splat:

The dependency on %edx was clearly exactly because the calling
convention for the slow-path was that %eax and %edx were clobbered,
and %edx was used as a temporary, so clobbering it had no downside.

So it wasn't useless, it was explicit, and commit 71c01930b42e was just broken.

I think your fix is wrong. Your fix adds the pointless push/pop that
doesn't help any, since you might as well just force the temporary
back to %edx.

The correct fix is to revert the broken commit.

If commit 71c01930b42e had actually generated better code, that would
be different. But it doesn't. So as it is, this is all just worse than
it used to be, and I don't see the point of "fixing" things by making
them worse.

Revert back to the old "use %edx as a temporary", together with a
comment so that this doesn't happen again.

                     Linus

Re: [PATCH] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()

[Borislav Petkov]

On Fri, May 13, 2016 at 10:03:26AM -0700, Linus Torvalds wrote:
> I think your fix is wrong. Your fix adds the pointless push/pop that
> doesn't help any, since you might as well just force the temporary
> back to %edx.

But if we do this, then everything using the slow path
call_rwsem_down_write_failed et al, which then calls
{save,restore}_common_regs, would have to remember to use %edx as
temporary because {save,restore}_common_regs won't protect it and gcc
might clobber it.

OTOH, the 64-bit versions {save,restore}_common_regs don't stash away
%rdx either so I guess that mechanism was supposed to not save the ABI
return registers rAX and rDX.

The only thing that needs to be corrected then is the misleading comment
above the 32-bit version "... Save the C-clobbered registers (%eax, %edx
and %ecx) .." - the 64-bit version comment is correct AFAICT.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[PATCH] locking/rwsem: Fix comment on register clobbering

[Borislav Petkov]

On Fri, May 13, 2016 at 07:19:15PM +0200, Borislav Petkov wrote:
> The only thing that needs to be corrected then is the misleading comment
> above the 32-bit version "... Save the C-clobbered registers (%eax, %edx
> and %ecx) .." - the 64-bit version comment is correct AFAICT.

---
From: Borislav Petkov <bp@suse.de>
Date: Mon, 16 May 2016 11:29:22 +0200
Subject: [PATCH] locking/rwsem: Fix comment on register clobbering

Document explicitly that %edx can get clobbered on the slow path, on
32-bit. Something I learned the hard way. :-\

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/lib/rwsem.S | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/lib/rwsem.S b/arch/x86/lib/rwsem.S
index a37462a23546..bb49caa4dd4c 100644
--- a/arch/x86/lib/rwsem.S
+++ b/arch/x86/lib/rwsem.S
@@ -29,8 +29,10 @@
  * there is contention on the semaphore.
  *
  * %eax contains the semaphore pointer on entry. Save the C-clobbered
- * registers (%eax, %edx and %ecx) except %eax whish is either a return
- * value or just clobbered..
+ * registers (%eax, %edx and %ecx) except %eax which is either a return
+ * value or just clobbered. Same is true for %edx so make sure gcc
+ * reloads it after the slow path, by making it hold a temporary, for
+ * example; see ____down_write().
  */
 
 #define save_common_regs \
-- 
2.7.3

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--