From: Po-Yu Chuang <[email protected]>
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 <[email protected]>
---
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
On Mon, Mar 14, 2011 at 06:28:36PM +0800, Po-Yu Chuang wrote:
> From: Po-Yu Chuang <[email protected]>
>
> 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.
Dear Russell King,
On Mon, Mar 14, 2011 at 6:40 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Mon, Mar 14, 2011 at 06:28:36PM +0800, Po-Yu Chuang wrote:
>> From: Po-Yu Chuang <[email protected]>
>>
>> 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
From: Po-Yu Chuang <[email protected]>
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 <[email protected]>
---
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
Dear Russell King,
On Tue, Mar 15, 2011 at 2:13 PM, Po-Yu Chuang <[email protected]> wrote:
>
> From: Po-Yu Chuang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Tue, 15 Mar 2011, Po-Yu Chuang wrote:
> From: Po-Yu Chuang <[email protected]>
>
> 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 <[email protected]>
Acked-by: Nicolas Pitre <[email protected]>
> ---
> 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
>
On Thu, Mar 17, 2011 at 1:01 PM, Nicolas Pitre <[email protected]> wrote:
> On Tue, 15 Mar 2011, Po-Yu Chuang wrote:
>
>> From: Po-Yu Chuang <[email protected]>
>>
>> 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 <[email protected]>
>
> Acked-by: Nicolas Pitre <[email protected]>
>
>> ---
>> 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
On Thu, 17 Mar 2011, Ashwin Chaugule wrote:
> On Thu, Mar 17, 2011 at 1:01 PM, Nicolas Pitre <[email protected]> wrote:
> > On Tue, 15 Mar 2011, Po-Yu Chuang wrote:
> >
> >> From: Po-Yu Chuang <[email protected]>
> >>
> >> 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 <[email protected]>
> >
> > Acked-by: Nicolas Pitre <[email protected]>
> >
> >> ---
> >> 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
Dear Nicolas Pitre,
On Fri, Mar 18, 2011 at 1:57 AM, Nicolas Pitre <[email protected]> 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
On Fri, 18 Mar 2011, Po-Yu Chuang wrote:
> On Fri, Mar 18, 2011 at 1:57 AM, Nicolas Pitre <[email protected]> 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
Dear Nicolas Pitre,
On Tue, Mar 22, 2011 at 4:41 AM, Nicolas Pitre <[email protected]> wrote:
> On Fri, 18 Mar 2011, Po-Yu Chuang wrote:
>> On Fri, Mar 18, 2011 at 1:57 AM, Nicolas Pitre <[email protected]> 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