* [PATCH] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()
@ 2016-05-12 17:29 ` Borislav Petkov
0 siblings, 0 replies; 11+ messages in thread
From: Borislav Petkov @ 2016-05-12 17:29 UTC (permalink / raw)
To: Guenter Roeck, Ingo Molnar, Peter Zijlstra
Cc: linux-next@vger.kernel.org, linux-kernel@vger.kernel.org
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)
--
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()
2016-05-12 17:29 ` Borislav Petkov
(?)
@ 2016-05-13 2:49 ` Guenter Roeck
-1 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2016-05-13 2:49 UTC (permalink / raw)
To: Borislav Petkov, Ingo Molnar, Peter Zijlstra
Cc: linux-next@vger.kernel.org, linux-kernel@vger.kernel.org
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. */
>
^ permalink raw reply [flat|nested] 11+ messages in thread* [tip:locking/rwsem] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()
2016-05-12 17:29 ` Borislav Petkov
(?)
(?)
@ 2016-05-13 9:21 ` tip-bot for Borislav Petkov
-1 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Borislav Petkov @ 2016-05-13 9:21 UTC (permalink / raw)
To: linux-tip-commits
Cc: luto, alexander.shishkin, linux-kernel, paulmck, torvalds, bp,
eranian, akpm, brgerst, bp, mingo, jolsa, linux, hpa, acme,
peterz, tglx, vincent.weaver, dvlasenk
Commit-ID: 9da6e1cf7f044569a7e8a607ecdea6f5192c6bce
Gitweb: http://git.kernel.org/tip/9da6e1cf7f044569a7e8a607ecdea6f5192c6bce
Author: Borislav Petkov <bp@suse.de>
AuthorDate: Thu, 12 May 2016 19:29:38 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 13 May 2016 11:15:19 +0200
x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()
____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>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: linux-next@vger.kernel.org
Link: http://lkml.kernel.org/r/20160512172938.GB14245@pd.tnic
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
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 a37462a..0224080 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. */
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()
2016-05-12 17:29 ` Borislav Petkov
` (2 preceding siblings ...)
(?)
@ 2016-05-13 17:03 ` Linus Torvalds
2016-05-13 17:19 ` Borislav Petkov
-1 siblings, 1 reply; 11+ messages in thread
From: Linus Torvalds @ 2016-05-13 17:03 UTC (permalink / raw)
To: Borislav Petkov
Cc: Guenter Roeck, Ingo Molnar, Peter Zijlstra,
linux-next@vger.kernel.org, linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down_write()
2016-05-13 17:03 ` [PATCH] " Linus Torvalds
@ 2016-05-13 17:19 ` Borislav Petkov
2016-05-16 9:34 ` [PATCH] locking/rwsem: Fix comment on register clobbering Borislav Petkov
0 siblings, 1 reply; 11+ messages in thread
From: Borislav Petkov @ 2016-05-13 17:19 UTC (permalink / raw)
To: Linus Torvalds
Cc: Guenter Roeck, Ingo Molnar, Peter Zijlstra,
linux-next@vger.kernel.org, linux-kernel@vger.kernel.org
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)
--
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH] locking/rwsem: Fix comment on register clobbering
2016-05-13 17:19 ` Borislav Petkov
@ 2016-05-16 9:34 ` Borislav Petkov
2016-05-16 10:39 ` [tip:locking/rwsem] " tip-bot for Borislav Petkov
0 siblings, 1 reply; 11+ messages in thread
From: Borislav Petkov @ 2016-05-16 9:34 UTC (permalink / raw)
To: x86-ml
Cc: Linus Torvalds, Guenter Roeck, Ingo Molnar, Peter Zijlstra,
linux-next@vger.kernel.org, linux-kernel@vger.kernel.org
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)
--
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [tip:locking/rwsem] locking/rwsem: Fix comment on register clobbering
2016-05-16 9:34 ` [PATCH] locking/rwsem: Fix comment on register clobbering Borislav Petkov
@ 2016-05-16 10:39 ` tip-bot for Borislav Petkov
0 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Borislav Petkov @ 2016-05-16 10:39 UTC (permalink / raw)
To: linux-tip-commits
Cc: jolsa, alexander.shishkin, luto, linux-kernel, linux, tglx,
dvlasenk, brgerst, hpa, acme, bp, bp, vincent.weaver, torvalds,
peterz, mingo, eranian
Commit-ID: 4544ba8c6b1743499cabb682897a469911845f15
Gitweb: http://git.kernel.org/tip/4544ba8c6b1743499cabb682897a469911845f15
Author: Borislav Petkov <bp@suse.de>
AuthorDate: Mon, 16 May 2016 11:34:28 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 16 May 2016 12:35:40 +0200
locking/rwsem: Fix comment on register clobbering
Document explicitly that %edx can get clobbered on the slow path, on
32-bit kernels. Something I learned the hard way. :-\
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: linux-next@vger.kernel.org
Link: http://lkml.kernel.org/r/20160516093428.GA26108@pd.tnic
Signed-off-by: Ingo Molnar <mingo@kernel.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 a37462a..bf2c607 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 gets 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 \
^ permalink raw reply related [flat|nested] 11+ messages in thread