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

Groups > linux.kernel > #1247553 > unrolled thread

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

Started byRussell King - ARM Linux <linux@arm.linux.org.uk>
First post2015-10-15 10:30 +0200
Last post2015-10-16 12:40 +0200
Articles 7 — 4 participants

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: [PATCH] ARM: SWP emulation: Restore original *data when failed Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-10-15 10:30 +0200
    Re: [PATCH] ARM: SWP emulation: Restore original *data when failed Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-10-15 11:00 +0200
      Re: [PATCH] ARM: SWP emulation: Restore original *data when failed Vladimir Murzin <vladimir.murzin@arm.com> - 2015-10-15 11:20 +0200
        Re: [PATCH] ARM: SWP emulation: Restore original *data when failed Will Deacon <will.deacon@arm.com> - 2015-10-15 15:10 +0200
          Re: [PATCH] ARM: SWP emulation: Restore original *data when failed Vladimir Murzin <vladimir.murzin@arm.com> - 2015-10-15 15:30 +0200
            Re: [PATCH] ARM: SWP emulation: Restore original *data when failed Vladimir Murzin <vladimir.murzin@arm.com> - 2015-10-16 10:00 +0200
              Re: [PATCH] ARM: SWP emulation: Restore original *data when failed Catalin Marinas <catalin.marinas@arm.com> - 2015-10-16 12:40 +0200

#1247553 — Re: [PATCH] ARM: SWP emulation: Restore original *data when failed

FromRussell King - ARM Linux <linux@arm.linux.org.uk>
Date2015-10-15 10:30 +0200
SubjectRe: [PATCH] ARM: SWP emulation: Restore original *data when failed
Message-ID<qjM5c-QH-9@gated-at.bofh.it>
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1247587

FromRussell King - ARM Linux <linux@arm.linux.org.uk>
Date2015-10-15 11:00 +0200
Message-ID<qjMyd-1oN-11@gated-at.bofh.it>
In reply to#1247553
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1247600

FromVladimir Murzin <vladimir.murzin@arm.com>
Date2015-10-15 11:20 +0200
Message-ID<qjMRz-21D-5@gated-at.bofh.it>
In reply to#1247587
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1247781

FromWill Deacon <will.deacon@arm.com>
Date2015-10-15 15:10 +0200
Message-ID<qjQsa-7jf-13@gated-at.bofh.it>
In reply to#1247600
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1247792

FromVladimir Murzin <vladimir.murzin@arm.com>
Date2015-10-15 15:30 +0200
Message-ID<qjQLv-7Gi-9@gated-at.bofh.it>
In reply to#1247781
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"			\
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1248431

FromVladimir Murzin <vladimir.murzin@arm.com>
Date2015-10-16 10:00 +0200
Message-ID<qk85H-8iu-17@gated-at.bofh.it>
In reply to#1247792
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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1248571

FromCatalin Marinas <catalin.marinas@arm.com>
Date2015-10-16 12:40 +0200
Message-ID<qkaAy-3B7-23@gated-at.bofh.it>
In reply to#1248431
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web