[PATCH] arm: cmpxchg syscall should data abort if page not write or not young

[PATCH] arm: cmpxchg syscall should data abort if page not write or not young

[Po-Yu Chuang]

From: Po-Yu Chuang <ratbert@faraday-tech.com>

If the page to cmpxchg is user mode read only (not write)
or invalid (not young), we should simulate a data abort first.

Signed-off-by: Po-Yu Chuang <ratbert@faraday-tech.com>
---
 arch/arm/kernel/traps.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 446aee9..53c8852 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -563,7 +563,8 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
 		if (!pmd_present(*pmd))
 			goto bad_access;
 		pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
-		if (!pte_present(*pte) || !pte_dirty(*pte)) {
+		if (!pte_present(*pte) || !pte_write(*pte) ||
+		    !pte_dirty(*pte) || !pte_young(*pte)) {
 			pte_unmap_unlock(pte, ptl);
 			goto bad_access;
 		}
-- 
1.6.3.3

[PATCH] arm: cmpxchg syscall should data abort if page not write or not young

[Russell King - ARM Linux]

On Mon, Mar 14, 2011 at 06:28:36PM +0800, Po-Yu Chuang wrote:
> From: Po-Yu Chuang <ratbert@faraday-tech.com>
> 
> If the page to cmpxchg is user mode read only (not write)
> or invalid (not young), we should simulate a data abort first.

No.  If it is not young then it should be made young.  Age is an effect
of accesses.  It's not a permission.

[PATCH] arm: cmpxchg syscall should data abort if page not write or not young

[Po-Yu Chuang]

Dear Russell King,

On Mon, Mar 14, 2011 at 6:40 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Mar 14, 2011 at 06:28:36PM +0800, Po-Yu Chuang wrote:
>> From: Po-Yu Chuang <ratbert@faraday-tech.com>
>>
>> If the page to cmpxchg is user mode read only (not write)
>> or invalid (not young), we should simulate a data abort first.
>
> No. ?If it is not young then it should be made young. ?Age is an effect
> of accesses. ?It's not a permission.

OK, I will rethink about the not young part.
Thanks for your pointer.

I will resubmit a v2 which contains only the read only check.
It fixes a problem we met with futex.

best regards,
Po-Yu Chuang

[PATCH v2] arm: cmpxchg syscall should data abort if page not write

[Po-Yu Chuang]

From: Po-Yu Chuang <ratbert@faraday-tech.com>

If the page to cmpxchg is user mode read only (not write),
we should simulate a data abort first.

Signed-off-by: Po-Yu Chuang <ratbert@faraday-tech.com>
---
v2:
remove !pte_young() check

 arch/arm/kernel/traps.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 446aee9..eac7c05 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -563,7 +563,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
 		if (!pmd_present(*pmd))
 			goto bad_access;
 		pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
-		if (!pte_present(*pte) || !pte_dirty(*pte)) {
+		if (!pte_present(*pte) || !pte_write(*pte) || !pte_dirty(*pte)) {
 			pte_unmap_unlock(pte, ptl);
 			goto bad_access;
 		}
-- 
1.6.3.3

[PATCH v2] arm: cmpxchg syscall should data abort if page not write

[Po-Yu Chuang]

Dear Russell King,

On Tue, Mar 15, 2011 at 2:13 PM, Po-Yu Chuang <ratbert.chuang@gmail.com> wrote:
>
> From: Po-Yu Chuang <ratbert@faraday-tech.com>
>
> If the page to cmpxchg is user mode read only (not write),
> we should simulate a data abort first.
>
> Signed-off-by: Po-Yu Chuang <ratbert@faraday-tech.com>
> ---
> v2:
> remove !pte_young() check
>
> ?arch/arm/kernel/traps.c | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 446aee9..eac7c05 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -563,7 +563,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
> ? ? ? ? ? ? ? ?if (!pmd_present(*pmd))
> ? ? ? ? ? ? ? ? ? ? ? ?goto bad_access;
> ? ? ? ? ? ? ? ?pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
> - ? ? ? ? ? ? ? if (!pte_present(*pte) || !pte_dirty(*pte)) {
> + ? ? ? ? ? ? ? if (!pte_present(*pte) || !pte_write(*pte) || !pte_dirty(*pte)) {
> ? ? ? ? ? ? ? ? ? ? ? ?pte_unmap_unlock(pte, ptl);
> ? ? ? ? ? ? ? ? ? ? ? ?goto bad_access;
> ? ? ? ? ? ? ? ?}
> --
> 1.6.3.3
>

I think maybe I should describe more details of the problem.
Here is the story.

There is a lock with value 0. After fork(), the page containing the lock
becomes user mode read only for COW later. Process 0 writes 1 to
the lock with cmpxchg syscall. This write should cause COW.
The value of lock of Process 0 should become 1 and the value of lock
of Porcess 1 should still be 0 in the COWed page.

(CORRECT)

P0:lock=0
P0:fork
P0:cmpxchg -> COW
P0:lock=1	P1:lock=0

However, because cmpxchg syscall did not check user mode read only,
it wrote 1 to the lock value directly. After returning to user mode,
Process 0 wrote another variable, say foo, on the same page and
caused COW. The value of lock of Process 1 became 1 which is
incorrect.

(INCORRECT)

P0:lock=0
P0:fork
P0:cmpxchg
P0:lock=1
P0:foo=123 -> COW
P0:lock=1	P1:lock=1

best regards,
Po-Yu Chuang

[PATCH v2] arm: cmpxchg syscall should data abort if page not write

[Nicolas Pitre]

On Tue, 15 Mar 2011, Po-Yu Chuang wrote:

> From: Po-Yu Chuang <ratbert@faraday-tech.com>
> 
> If the page to cmpxchg is user mode read only (not write),
> we should simulate a data abort first.
> 
> Signed-off-by: Po-Yu Chuang <ratbert@faraday-tech.com>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

> ---
> v2:
> remove !pte_young() check
> 
>  arch/arm/kernel/traps.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 446aee9..eac7c05 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -563,7 +563,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
>  		if (!pmd_present(*pmd))
>  			goto bad_access;
>  		pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
> -		if (!pte_present(*pte) || !pte_dirty(*pte)) {
> +		if (!pte_present(*pte) || !pte_write(*pte) || !pte_dirty(*pte)) {
>  			pte_unmap_unlock(pte, ptl);
>  			goto bad_access;
>  		}
> -- 
> 1.6.3.3
> 

[PATCH v2] arm: cmpxchg syscall should data abort if page not write

[Ashwin Chaugule]

On Thu, Mar 17, 2011 at 1:01 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Tue, 15 Mar 2011, Po-Yu Chuang wrote:
>
>> From: Po-Yu Chuang <ratbert@faraday-tech.com>
>>
>> If the page to cmpxchg is user mode read only (not write),
>> we should simulate a data abort first.
>>
>> Signed-off-by: Po-Yu Chuang <ratbert@faraday-tech.com>
>
> Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>
>
>> ---
>> v2:
>> remove !pte_young() check
>>
>> ?arch/arm/kernel/traps.c | ? ?2 +-
>> ?1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>> index 446aee9..eac7c05 100644
>> --- a/arch/arm/kernel/traps.c
>> +++ b/arch/arm/kernel/traps.c
>> @@ -563,7 +563,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
>> ? ? ? ? ? ? ? if (!pmd_present(*pmd))
>> ? ? ? ? ? ? ? ? ? ? ? goto bad_access;
>> ? ? ? ? ? ? ? pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
>> - ? ? ? ? ? ? if (!pte_present(*pte) || !pte_dirty(*pte)) {
>> + ? ? ? ? ? ? if (!pte_present(*pte) || !pte_write(*pte) || !pte_dirty(*pte)) {
>> ? ? ? ? ? ? ? ? ? ? ? pte_unmap_unlock(pte, ptl);
>> ? ? ? ? ? ? ? ? ? ? ? goto bad_access;
>> ? ? ? ? ? ? ? }
>> --
>> 1.6.3.3
>>


Just beginning to understand the subtleties involved, so please
correct me if I'm wrong.
Wont this patch also fix the problem that was brought up with futexes
on ARM SMP ?

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-June/017374.html

Cheers,
Ashwin

[PATCH v2] arm: cmpxchg syscall should data abort if page not write

[Nicolas Pitre]

On Thu, 17 Mar 2011, Ashwin Chaugule wrote:

> On Thu, Mar 17, 2011 at 1:01 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > On Tue, 15 Mar 2011, Po-Yu Chuang wrote:
> >
> >> From: Po-Yu Chuang <ratbert@faraday-tech.com>
> >>
> >> If the page to cmpxchg is user mode read only (not write),
> >> we should simulate a data abort first.
> >>
> >> Signed-off-by: Po-Yu Chuang <ratbert@faraday-tech.com>
> >
> > Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> >
> >> ---
> >> v2:
> >> remove !pte_young() check
> >>
> >> ?arch/arm/kernel/traps.c | ? ?2 +-
> >> ?1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> >> index 446aee9..eac7c05 100644
> >> --- a/arch/arm/kernel/traps.c
> >> +++ b/arch/arm/kernel/traps.c
> >> @@ -563,7 +563,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
> >> ? ? ? ? ? ? ? if (!pmd_present(*pmd))
> >> ? ? ? ? ? ? ? ? ? ? ? goto bad_access;
> >> ? ? ? ? ? ? ? pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
> >> - ? ? ? ? ? ? if (!pte_present(*pte) || !pte_dirty(*pte)) {
> >> + ? ? ? ? ? ? if (!pte_present(*pte) || !pte_write(*pte) || !pte_dirty(*pte)) {
> >> ? ? ? ? ? ? ? ? ? ? ? pte_unmap_unlock(pte, ptl);
> >> ? ? ? ? ? ? ? ? ? ? ? goto bad_access;
> >> ? ? ? ? ? ? ? }
> >> --
> >> 1.6.3.3
> >>
> 
> 
> Just beginning to understand the subtleties involved, so please
> correct me if I'm wrong.
> Wont this patch also fix the problem that was brought up with futexes
> on ARM SMP ?

Nope.  The code being fixed here was suptly broken so it needs fixing.  
However this code is almost never used, if at all, as it is a fall-back 
solution for when all the better alternatives are not available for some 
reasons (and I'm still wondering what those reasons are for Po-Yu Chuang 
to actually use that code).  In practice this bug should have affected 
no one.

If you have SMP then this code is definitively not what you should be 
using.


Nicolas

[PATCH v2] arm: cmpxchg syscall should data abort if page not write

[Po-Yu Chuang]

Dear Nicolas Pitre,

On Fri, Mar 18, 2011 at 1:57 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Thu, 17 Mar 2011, Ashwin Chaugule wrote:
>>
>> Just beginning to understand the subtleties involved, so please
>> correct me if I'm wrong.
>> Wont this patch also fix the problem that was brought up with futexes
>> on ARM SMP ?
>
> Nope. ?The code being fixed here was suptly broken so it needs fixing.
> However this code is almost never used, if at all, as it is a fall-back
> solution for when all the better alternatives are not available for some
> reasons (and I'm still wondering what those reasons are for Po-Yu Chuang
> to actually use that code). ?In practice this bug should have affected
> no one.

We met this problem while porting an v5 SMP processor because kernel
selects NEEDS_SYSCALL_FOR_CMPXCHG by default if CPU_32v5.

After we added our own implementation of __kuser_cmpxchg, this code
is not needed anymore. But since this is actually a bug, it is still a good
idea to submit a patch. :-)

> If you have SMP then this code is definitively not what you should be
> using.

Best regards,
Po-Yu Chuang

[PATCH v2] arm: cmpxchg syscall should data abort if page not write

[Nicolas Pitre]

On Fri, 18 Mar 2011, Po-Yu Chuang wrote:

> On Fri, Mar 18, 2011 at 1:57 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > Nope. ?The code being fixed here was suptly broken so it needs fixing.
> > However this code is almost never used, if at all, as it is a fall-back
> > solution for when all the better alternatives are not available for some
> > reasons (and I'm still wondering what those reasons are for Po-Yu Chuang
> > to actually use that code). ?In practice this bug should have affected
> > no one.
> 
> We met this problem while porting an v5 SMP processor because kernel
> selects NEEDS_SYSCALL_FOR_CMPXCHG by default if CPU_32v5.
> 
> After we added our own implementation of __kuser_cmpxchg, this code
> is not needed anymore. But since this is actually a bug, it is still a good
> idea to submit a patch. :-)

Yes.  Please send to RMK's patch system with my ACK.


Nicolas

[PATCH v2] arm: cmpxchg syscall should data abort if page not write

[Po-Yu Chuang]

Dear Nicolas Pitre,

On Tue, Mar 22, 2011 at 4:41 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Fri, 18 Mar 2011, Po-Yu Chuang wrote:
>> On Fri, Mar 18, 2011 at 1:57 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>> > Nope. ?The code being fixed here was suptly broken so it needs fixing.
>> > However this code is almost never used, if at all, as it is a fall-back
>> > solution for when all the better alternatives are not available for some
>> > reasons (and I'm still wondering what those reasons are for Po-Yu Chuang
>> > to actually use that code). ?In practice this bug should have affected
>> > no one.
>>
>> We met this problem while porting an v5 SMP processor because kernel
>> selects NEEDS_SYSCALL_FOR_CMPXCHG by default if CPU_32v5.
>>
>> After we added our own implementation of __kuser_cmpxchg, this code
>> is not needed anymore. But since this is actually a bug, it is still a good
>> idea to submit a patch. :-)
>
> Yes. ?Please send to RMK's patch system with my ACK.

I thought that Russell King would apply this patch after you ACKed it.
Doesn't it work like this here? How should I do next?

Best regards,
Po-Yu Chuang