[PATCH] ARM: SWP emulation: Restore original *data when failed

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Shengjiu Wang]

__user_swpX_asm maybe failed in first STREX operation, emulate_swpX
will try again, but the *data has been changed in first time. which
cause the result is wrong. So need to recover the *data when failed.

Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
---
 arch/arm/kernel/swp_emulate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
index 5b26e7e..c61fbf92 100644
--- a/arch/arm/kernel/swp_emulate.c
+++ b/arch/arm/kernel/swp_emulate.c
@@ -41,6 +41,7 @@
 	"1:	strex"B"	%0, %2, [%3]\n"			\
 	"	cmp		%0, #0\n"			\
 	"	movne		%0, %4\n"			\
+	"	movne		%1, %2\n"			\
 	"2:\n"							\
 	"	.section	 .text.fixup,\"ax\"\n"		\
 	"	.align		2\n"				\
-- 
1.9.1

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Russell King - ARM Linux]

On Wed, Oct 14, 2015 at 10:51:17AM +0800, Shengjiu Wang wrote:
> __user_swpX_asm maybe failed in first STREX operation, emulate_swpX
> will try again, but the *data has been changed in first time. which
> cause the result is wrong. So need to recover the *data when failed.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> ---
>  arch/arm/kernel/swp_emulate.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
> index 5b26e7e..c61fbf92 100644
> --- a/arch/arm/kernel/swp_emulate.c
> +++ b/arch/arm/kernel/swp_emulate.c
> @@ -41,6 +41,7 @@
>  	"1:	strex"B"	%0, %2, [%3]\n"			\
>  	"	cmp		%0, #0\n"			\
>  	"	movne		%0, %4\n"			\
> +	"	movne		%1, %2\n"			\
>  	"2:\n"							\
>  	"	.section	 .text.fixup,\"ax\"\n"		\
>  	"	.align		2\n"				\

I think I'd prefer this to be:

	__asm__ __volatile__(					\
	"0:	ldrex"B"	%2, [%3]\n"			\
	"1:	strex"B"	%0, %1, [%3]\n" 		\
	"	cmp		%0, #0\n"			\
	"	moveq		%1, %2\n"			\
	"	movne		%0, %4\n"			\

so that we're not loading into %1 (an in-out non-temporary) but rather
loading it into a temporary - and only overwriting the saved register
value if the swap succeeds.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Shengjiu Wang]

On Thu, Oct 15, 2015 at 09:24:17AM +0100, Russell King - ARM Linux wrote:
> On Wed, Oct 14, 2015 at 10:51:17AM +0800, Shengjiu Wang wrote:
> > __user_swpX_asm maybe failed in first STREX operation, emulate_swpX
> > will try again, but the *data has been changed in first time. which
> > cause the result is wrong. So need to recover the *data when failed.
> > 
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > ---
> >  arch/arm/kernel/swp_emulate.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
> > index 5b26e7e..c61fbf92 100644
> > --- a/arch/arm/kernel/swp_emulate.c
> > +++ b/arch/arm/kernel/swp_emulate.c
> > @@ -41,6 +41,7 @@
> >  	"1:	strex"B"	%0, %2, [%3]\n"			\
> >  	"	cmp		%0, #0\n"			\
> >  	"	movne		%0, %4\n"			\
> > +	"	movne		%1, %2\n"			\
> >  	"2:\n"							\
> >  	"	.section	 .text.fixup,\"ax\"\n"		\
> >  	"	.align		2\n"				\
> 
> I think I'd prefer this to be:
> 
> 	__asm__ __volatile__(					\
> 	"0:	ldrex"B"	%2, [%3]\n"			\
> 	"1:	strex"B"	%0, %1, [%3]\n" 		\
> 	"	cmp		%0, #0\n"			\
> 	"	moveq		%1, %2\n"			\
> 	"	movne		%0, %4\n"			\
> 
> so that we're not loading into %1 (an in-out non-temporary) but rather
> loading it into a temporary - and only overwriting the saved register
> value if the swap succeeds.
> 
> Thanks.

I am ok with your change. Need I send another patch for this change? or will
you send it by yourself?

best regards
wang shengjiu
> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Russell King - ARM Linux]

On Thu, Oct 15, 2015 at 04:36:31PM +0800, Shengjiu Wang wrote:
> On Thu, Oct 15, 2015 at 09:24:17AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Oct 14, 2015 at 10:51:17AM +0800, Shengjiu Wang wrote:
> > > __user_swpX_asm maybe failed in first STREX operation, emulate_swpX
> > > will try again, but the *data has been changed in first time. which
> > > cause the result is wrong. So need to recover the *data when failed.
> > > 
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > ---
> > >  arch/arm/kernel/swp_emulate.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
> > > index 5b26e7e..c61fbf92 100644
> > > --- a/arch/arm/kernel/swp_emulate.c
> > > +++ b/arch/arm/kernel/swp_emulate.c
> > > @@ -41,6 +41,7 @@
> > >  	"1:	strex"B"	%0, %2, [%3]\n"			\
> > >  	"	cmp		%0, #0\n"			\
> > >  	"	movne		%0, %4\n"			\
> > > +	"	movne		%1, %2\n"			\
> > >  	"2:\n"							\
> > >  	"	.section	 .text.fixup,\"ax\"\n"		\
> > >  	"	.align		2\n"				\
> > 
> > I think I'd prefer this to be:
> > 
> > 	__asm__ __volatile__(					\
> > 	"0:	ldrex"B"	%2, [%3]\n"			\
> > 	"1:	strex"B"	%0, %1, [%3]\n" 		\
> > 	"	cmp		%0, #0\n"			\
> > 	"	moveq		%1, %2\n"			\
> > 	"	movne		%0, %4\n"			\
> > 
> > so that we're not loading into %1 (an in-out non-temporary) but rather
> > loading it into a temporary - and only overwriting the saved register
> > value if the swap succeeds.
> > 
> > Thanks.
> 
> I am ok with your change. Need I send another patch for this change? or will
> you send it by yourself?

Please send a new patch, thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Vladimir Murzin]

On 15/10/15 09:57, Russell King - ARM Linux wrote:
> On Thu, Oct 15, 2015 at 04:36:31PM +0800, Shengjiu Wang wrote:
>> On Thu, Oct 15, 2015 at 09:24:17AM +0100, Russell King - ARM Linux wrote:
>>> On Wed, Oct 14, 2015 at 10:51:17AM +0800, Shengjiu Wang wrote:
>>>> __user_swpX_asm maybe failed in first STREX operation, emulate_swpX
>>>> will try again, but the *data has been changed in first time. which
>>>> cause the result is wrong. So need to recover the *data when failed.
>>>>
>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
>>>> ---
>>>>  arch/arm/kernel/swp_emulate.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
>>>> index 5b26e7e..c61fbf92 100644
>>>> --- a/arch/arm/kernel/swp_emulate.c
>>>> +++ b/arch/arm/kernel/swp_emulate.c
>>>> @@ -41,6 +41,7 @@
>>>>  	"1:	strex"B"	%0, %2, [%3]\n"			\
>>>>  	"	cmp		%0, #0\n"			\
>>>>  	"	movne		%0, %4\n"			\
>>>> +	"	movne		%1, %2\n"			\
>>>>  	"2:\n"							\
>>>>  	"	.section	 .text.fixup,\"ax\"\n"		\
>>>>  	"	.align		2\n"				\
>>>
>>> I think I'd prefer this to be:
>>>
>>> 	__asm__ __volatile__(					\
>>> 	"0:	ldrex"B"	%2, [%3]\n"			\
>>> 	"1:	strex"B"	%0, %1, [%3]\n" 		\
>>> 	"	cmp		%0, #0\n"			\
>>> 	"	moveq		%1, %2\n"			\
>>> 	"	movne		%0, %4\n"			\
>>>
>>> so that we're not loading into %1 (an in-out non-temporary) but rather
>>> loading it into a temporary - and only overwriting the saved register
>>> value if the swap succeeds.
>>>
>>> Thanks.
>>
>> I am ok with your change. Need I send another patch for this change? or will
>> you send it by yourself?
> 
> Please send a new patch, thanks.
> 

We might need the same change for arm64 counterpart (see
arch/arm64/kernel/armv8_deprecated.c).

Vladimir

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Will Deacon]

On Thu, Oct 15, 2015 at 10:17:47AM +0100, Vladimir Murzin wrote:
> We might need the same change for arm64 counterpart (see
> arch/arm64/kernel/armv8_deprecated.c).

Something like below?

Will

>From 63c3e83073cfac2e011adf0ed6f335275cc977a7 Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon@arm.com>
Date: Thu, 15 Oct 2015 13:55:53 +0100
Subject: [PATCH] arm64: compat: fix stxr failure case in SWP emulation

If the STXR instruction fails in the SWP emulation code, we leave *data
overwritten with the loaded value, therefore corrupting the data written
by a subsequent, successful attempt.

This patch re-jigs the code so that we only write back to *data once we
know that the update has happened.

Reported-by: Shengjiu Wang <shengjiu.wang@freescale.com>
Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/armv8_deprecated.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index bcee7abac68e..6039d1eb5912 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -284,12 +284,12 @@ static void register_insn_emulation_sysctl(struct ctl_table *table)
 	__asm__ __volatile__(					\
 	ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN,	\
 		    CONFIG_ARM64_PAN)				\
-	"	mov		%w2, %w1\n"			\
-	"0:	ldxr"B"		%w1, [%3]\n"			\
-	"1:	stxr"B"		%w0, %w2, [%3]\n"		\
+	"0:	ldxr"B"		%w2, [%3]\n"			\
+	"1:	stxr"B"		%w0, %w1, [%3]\n"		\
 	"	cbz		%w0, 2f\n"			\
 	"	mov		%w0, %w4\n"			\
 	"2:\n"							\
+	"	mov		%w1, %w2\n"			\
 	"	.pushsection	 .fixup,\"ax\"\n"		\
 	"	.align		2\n"				\
 	"3:	mov		%w0, %w5\n"			\
-- 
2.1.4

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Vladimir Murzin]

On 15/10/15 14:02, Will Deacon wrote:
> On Thu, Oct 15, 2015 at 10:17:47AM +0100, Vladimir Murzin wrote:
>> We might need the same change for arm64 counterpart (see
>> arch/arm64/kernel/armv8_deprecated.c).
> 
> Something like below?

Looks good. Should these two go to stable?

Vladimir

> 
> Will
> 
>>From 63c3e83073cfac2e011adf0ed6f335275cc977a7 Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon@arm.com>
> Date: Thu, 15 Oct 2015 13:55:53 +0100
> Subject: [PATCH] arm64: compat: fix stxr failure case in SWP emulation
> 
> If the STXR instruction fails in the SWP emulation code, we leave *data
> overwritten with the loaded value, therefore corrupting the data written
> by a subsequent, successful attempt.
> 
> This patch re-jigs the code so that we only write back to *data once we
> know that the update has happened.
> 
> Reported-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/armv8_deprecated.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> index bcee7abac68e..6039d1eb5912 100644
> --- a/arch/arm64/kernel/armv8_deprecated.c
> +++ b/arch/arm64/kernel/armv8_deprecated.c
> @@ -284,12 +284,12 @@ static void register_insn_emulation_sysctl(struct ctl_table *table)
>  	__asm__ __volatile__(					\
>  	ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN,	\
>  		    CONFIG_ARM64_PAN)				\
> -	"	mov		%w2, %w1\n"			\
> -	"0:	ldxr"B"		%w1, [%3]\n"			\
> -	"1:	stxr"B"		%w0, %w2, [%3]\n"		\
> +	"0:	ldxr"B"		%w2, [%3]\n"			\
> +	"1:	stxr"B"		%w0, %w1, [%3]\n"		\
>  	"	cbz		%w0, 2f\n"			\
>  	"	mov		%w0, %w4\n"			\
>  	"2:\n"							\
> +	"	mov		%w1, %w2\n"			\
>  	"	.pushsection	 .fixup,\"ax\"\n"		\
>  	"	.align		2\n"				\
>  	"3:	mov		%w0, %w5\n"			\
> 

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Vladimir Murzin]

On 15/10/15 14:25, Vladimir Murzin wrote:
> On 15/10/15 14:02, Will Deacon wrote:
>> On Thu, Oct 15, 2015 at 10:17:47AM +0100, Vladimir Murzin wrote:
>>> We might need the same change for arm64 counterpart (see
>>> arch/arm64/kernel/armv8_deprecated.c).
>>
>> Something like below?
> 
> Looks good. Should these two go to stable?

On the second thought looks like we still update *data in case stxr
fails (or I need more coffee).

Vladimir

> 
> Vladimir
> 
>>
>> Will
>>
>> >From 63c3e83073cfac2e011adf0ed6f335275cc977a7 Mon Sep 17 00:00:00 2001
>> From: Will Deacon <will.deacon@arm.com>
>> Date: Thu, 15 Oct 2015 13:55:53 +0100
>> Subject: [PATCH] arm64: compat: fix stxr failure case in SWP emulation
>>
>> If the STXR instruction fails in the SWP emulation code, we leave *data
>> overwritten with the loaded value, therefore corrupting the data written
>> by a subsequent, successful attempt.
>>
>> This patch re-jigs the code so that we only write back to *data once we
>> know that the update has happened.
>>
>> Reported-by: Shengjiu Wang <shengjiu.wang@freescale.com>
>> Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>>  arch/arm64/kernel/armv8_deprecated.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
>> index bcee7abac68e..6039d1eb5912 100644
>> --- a/arch/arm64/kernel/armv8_deprecated.c
>> +++ b/arch/arm64/kernel/armv8_deprecated.c
>> @@ -284,12 +284,12 @@ static void register_insn_emulation_sysctl(struct ctl_table *table)
>>  	__asm__ __volatile__(					\
>>  	ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN,	\
>>  		    CONFIG_ARM64_PAN)				\
>> -	"	mov		%w2, %w1\n"			\
>> -	"0:	ldxr"B"		%w1, [%3]\n"			\
>> -	"1:	stxr"B"		%w0, %w2, [%3]\n"		\
>> +	"0:	ldxr"B"		%w2, [%3]\n"			\
>> +	"1:	stxr"B"		%w0, %w1, [%3]\n"		\
>>  	"	cbz		%w0, 2f\n"			\
>>  	"	mov		%w0, %w4\n"			\
>>  	"2:\n"							\
>> +	"	mov		%w1, %w2\n"			\
>>  	"	.pushsection	 .fixup,\"ax\"\n"		\
>>  	"	.align		2\n"				\
>>  	"3:	mov		%w0, %w5\n"			\
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Catalin Marinas]

On Fri, Oct 16, 2015 at 08:52:24AM +0100, Vladimir Murzin wrote:
> > On 15/10/15 14:02, Will Deacon wrote:
> >> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> >> index bcee7abac68e..6039d1eb5912 100644
> >> --- a/arch/arm64/kernel/armv8_deprecated.c
> >> +++ b/arch/arm64/kernel/armv8_deprecated.c
> >> @@ -284,12 +284,12 @@ static void register_insn_emulation_sysctl(struct ctl_table *table)
> >>  	__asm__ __volatile__(					\
> >>  	ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN,	\
> >>  		    CONFIG_ARM64_PAN)				\
> >> -	"	mov		%w2, %w1\n"			\
> >> -	"0:	ldxr"B"		%w1, [%3]\n"			\
> >> -	"1:	stxr"B"		%w0, %w2, [%3]\n"		\
> >> +	"0:	ldxr"B"		%w2, [%3]\n"			\
> >> +	"1:	stxr"B"		%w0, %w1, [%3]\n"		\
> >>  	"	cbz		%w0, 2f\n"			\
> >>  	"	mov		%w0, %w4\n"			\
> >>  	"2:\n"							\
> >> +	"	mov		%w1, %w2\n"			\
> >>  	"	.pushsection	 .fixup,\"ax\"\n"		\
> >>  	"	.align		2\n"				\
> >>  	"3:	mov		%w0, %w5\n"			\
> 
> On the second thought looks like we still update *data in case stxr
> fails (or I need more coffee).

I'm on the second cup and I see the same problem. Even if stxr fails, we
fall back through "mov %w1, %w2", so *data is always updated with the
loaded value. Maybe something like below on top of Will's patch:

diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index 6039d1eb5912..3fab37b3bc95 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -286,10 +286,10 @@ static void register_insn_emulation_sysctl(struct ctl_table *table)
 		    CONFIG_ARM64_PAN)				\
 	"0:	ldxr"B"		%w2, [%3]\n"			\
 	"1:	stxr"B"		%w0, %w1, [%3]\n"		\
-	"	cbz		%w0, 2f\n"			\
-	"	mov		%w0, %w4\n"			\
+	"	cmp		%w0, #0\n"			\
+	"	csel		%w0, %w4, wzr, ne\n"		\
+	"	csel		%w1, %w2, %w1, eq\n"		\
 	"2:\n"							\
-	"	mov		%w1, %w2\n"			\
 	"	.pushsection	 .fixup,\"ax\"\n"		\
 	"	.align		2\n"				\
 	"3:	mov		%w0, %w5\n"			\
@@ -303,7 +303,7 @@ static void register_insn_emulation_sysctl(struct ctl_table *table)
 	ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN,	\
 		CONFIG_ARM64_PAN)				\
 	: "=&r" (res), "+r" (data), "=&r" (temp)		\
-	: "r" (addr), "i" (-EAGAIN), "i" (-EFAULT)		\
+	: "r" (addr), "r" (-EAGAIN), "i" (-EFAULT)		\
 	: "memory")
 
 #define __user_swp_asm(data, addr, res, temp) \
@@ -342,7 +342,7 @@ static void set_segfault(struct pt_regs *regs, unsigned long addr)
 static int emulate_swpX(unsigned int address, unsigned int *data,
 			unsigned int type)
 {
-	unsigned int res = 0;
+	unsigned int res;
 
 	if ((type != TYPE_SWPB) && (address & 0x3)) {
 		/* SWP to unaligned address not permitted */


Since Will is away for a week, I'll wait until he's back to push this
patch.

-- 
Catalin

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Will Deacon]

On Fri, Oct 16, 2015 at 11:37:29AM +0100, Catalin Marinas wrote:
> On Fri, Oct 16, 2015 at 08:52:24AM +0100, Vladimir Murzin wrote:
> > > On 15/10/15 14:02, Will Deacon wrote:
> > >> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> > >> index bcee7abac68e..6039d1eb5912 100644
> > >> --- a/arch/arm64/kernel/armv8_deprecated.c
> > >> +++ b/arch/arm64/kernel/armv8_deprecated.c
> > >> @@ -284,12 +284,12 @@ static void register_insn_emulation_sysctl(struct ctl_table *table)
> > >>  	__asm__ __volatile__(					\
> > >>  	ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN,	\
> > >>  		    CONFIG_ARM64_PAN)				\
> > >> -	"	mov		%w2, %w1\n"			\
> > >> -	"0:	ldxr"B"		%w1, [%3]\n"			\
> > >> -	"1:	stxr"B"		%w0, %w2, [%3]\n"		\
> > >> +	"0:	ldxr"B"		%w2, [%3]\n"			\
> > >> +	"1:	stxr"B"		%w0, %w1, [%3]\n"		\
> > >>  	"	cbz		%w0, 2f\n"			\
> > >>  	"	mov		%w0, %w4\n"			\
> > >>  	"2:\n"							\
> > >> +	"	mov		%w1, %w2\n"			\
> > >>  	"	.pushsection	 .fixup,\"ax\"\n"		\
> > >>  	"	.align		2\n"				\
> > >>  	"3:	mov		%w0, %w5\n"			\
> > 
> > On the second thought looks like we still update *data in case stxr
> > fails (or I need more coffee).
> 
> I'm on the second cup and I see the same problem. Even if stxr fails, we
> fall back through "mov %w1, %w2", so *data is always updated with the
> loaded value. Maybe something like below on top of Will's patch:

Yeah, sorry, my original patch was an untested mess. I think we can avoid
the "cc" clobber by adding a branch to the slow-path, as below.

Still needs testing, mind.

Will

--->8

diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index 6039d1eb5912..937f5e58a4d3 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -288,17 +288,19 @@ static void register_insn_emulation_sysctl(struct ctl_table *table)
 	"1:	stxr"B"		%w0, %w1, [%3]\n"		\
 	"	cbz		%w0, 2f\n"			\
 	"	mov		%w0, %w4\n"			\
+	"	b		3f\n"				\
 	"2:\n"							\
 	"	mov		%w1, %w2\n"			\
+	"3:\n"							\
 	"	.pushsection	 .fixup,\"ax\"\n"		\
 	"	.align		2\n"				\
-	"3:	mov		%w0, %w5\n"			\
-	"	b		2b\n"				\
+	"4:	mov		%w0, %w5\n"			\
+	"	b		3b\n"				\
 	"	.popsection"					\
 	"	.pushsection	 __ex_table,\"a\"\n"		\
 	"	.align		3\n"				\
-	"	.quad		0b, 3b\n"			\
-	"	.quad		1b, 3b\n"			\
+	"	.quad		0b, 4b\n"			\
+	"	.quad		1b, 4b\n"			\
 	"	.popsection\n"					\
 	ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN,	\
 		CONFIG_ARM64_PAN)				\

[PATCH] ARM: SWP emulation: Restore original *data when failed

[Will Deacon]

On Tue, Oct 27, 2015 at 03:44:24PM +0000, Will Deacon wrote:
> On Fri, Oct 16, 2015 at 11:37:29AM +0100, Catalin Marinas wrote:
> > On Fri, Oct 16, 2015 at 08:52:24AM +0100, Vladimir Murzin wrote:
> > > > On 15/10/15 14:02, Will Deacon wrote:
> > > >> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> > > >> index bcee7abac68e..6039d1eb5912 100644
> > > >> --- a/arch/arm64/kernel/armv8_deprecated.c
> > > >> +++ b/arch/arm64/kernel/armv8_deprecated.c
> > > >> @@ -284,12 +284,12 @@ static void register_insn_emulation_sysctl(struct ctl_table *table)
> > > >>  	__asm__ __volatile__(					\
> > > >>  	ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN,	\
> > > >>  		    CONFIG_ARM64_PAN)				\
> > > >> -	"	mov		%w2, %w1\n"			\
> > > >> -	"0:	ldxr"B"		%w1, [%3]\n"			\
> > > >> -	"1:	stxr"B"		%w0, %w2, [%3]\n"		\
> > > >> +	"0:	ldxr"B"		%w2, [%3]\n"			\
> > > >> +	"1:	stxr"B"		%w0, %w1, [%3]\n"		\
> > > >>  	"	cbz		%w0, 2f\n"			\
> > > >>  	"	mov		%w0, %w4\n"			\
> > > >>  	"2:\n"							\
> > > >> +	"	mov		%w1, %w2\n"			\
> > > >>  	"	.pushsection	 .fixup,\"ax\"\n"		\
> > > >>  	"	.align		2\n"				\
> > > >>  	"3:	mov		%w0, %w5\n"			\
> > > 
> > > On the second thought looks like we still update *data in case stxr
> > > fails (or I need more coffee).
> > 
> > I'm on the second cup and I see the same problem. Even if stxr fails, we
> > fall back through "mov %w1, %w2", so *data is always updated with the
> > loaded value. Maybe something like below on top of Will's patch:
> 
> Yeah, sorry, my original patch was an untested mess. I think we can avoid
> the "cc" clobber by adding a branch to the slow-path, as below.
> 
> Still needs testing, mind.

Right, it passes the simple test case I wrote (which fails under mainline).

Will