[PATCH] ARM: Implement copy_to_user_page() for noMMU

[PATCH] ARM: Implement copy_to_user_page() for noMMU

[Catalin Marinas]

Commit 7959722 introduced calls to copy_(to|from)_user_page() from
access_process_vm() in mm/nommu.c. The copy_to_user_page() was not
implemented on noMMU ARM.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/mm/nommu.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/nommu.c b/arch/arm/mm/nommu.c
index f8791ee..33b3273 100644
--- a/arch/arm/mm/nommu.c
+++ b/arch/arm/mm/nommu.c
@@ -65,6 +65,15 @@ void flush_dcache_page(struct page *page)
 }
 EXPORT_SYMBOL(flush_dcache_page);
 
+void copy_to_user_page(struct vm_area_struct *vma, struct page *page,
+		       unsigned long uaddr, void *dst, const void *src,
+		       unsigned long len)
+{
+	memcpy(dst, src, len);
+	if (vma->vm_flags & VM_EXEC)
+		__cpuc_coherent_user_range(uaddr, uaddr + len);
+}
+
 void __iomem *__arm_ioremap_pfn(unsigned long pfn, unsigned long offset,
 				size_t size, unsigned int mtype)
 {

[PATCH] ARM: Implement copy_to_user_page() for noMMU

[Jamie Lokier]

Catalin Marinas wrote:
> +void copy_to_user_page(struct vm_area_struct *vma, struct page *page,
> +		       unsigned long uaddr, void *dst, const void *src,
> +		       unsigned long len)
> +{
> +	memcpy(dst, src, len);
> +	if (vma->vm_flags & VM_EXEC)
> +		__cpuc_coherent_user_range(uaddr, uaddr + len);
> +}

Does that do the right thing with uaddr + len == 0?

Since this is debugging, it would be unfortunate if something was
mapped !VM_EXEC but executed anyway (because it works, or because of a
bug), and setting a breakpoint failed to be effective because of
entries in the i-cache.  It's forbidden semantically, and code which
wrote _itself_ to code without flushing i-cache on nommu gets what it
deserves.  But it may occur that it's executing, even if just due to
an application bug, and I'm thinking tracing under the debugger is one
time it's good to be more reliable.

Other variations such as writing when a mapping is !VM_EXEC and later
mapping or mprotecting the same shmem VM_EXEC, but it's even more
forbidden semantically to write to a read-only mapping (and just as
unchecked on nommu), and conversion of writable to VM_EXEC ought to
flush i-cache at mprotect time.

-- Jamie

[PATCH] ARM: Implement copy_to_user_page() for noMMU

[Russell King - ARM Linux]

On Tue, Mar 30, 2010 at 02:31:32AM +0100, Jamie Lokier wrote:
> Other variations such as writing when a mapping is !VM_EXEC and later
> mapping or mprotecting the same shmem VM_EXEC, but it's even more
> forbidden semantically to write to a read-only mapping (and just as
> unchecked on nommu), and conversion of writable to VM_EXEC ought to
> flush i-cache at mprotect time.

If NX isn't implemented, then arm_elf_read_implies_exec() must return 1
for the CPU - so that any region that is marked readable will have
VM_EXEC set.

[PATCH] ARM: Implement copy_to_user_page() for noMMU

[Catalin Marinas]

On Tue, 2010-03-30 at 02:31 +0100, Jamie Lokier wrote:
> Catalin Marinas wrote:
> > +void copy_to_user_page(struct vm_area_struct *vma, struct page *page,
> > +                    unsigned long uaddr, void *dst, const void *src,
> > +                    unsigned long len)
> > +{
> > +     memcpy(dst, src, len);
> > +     if (vma->vm_flags & VM_EXEC)
> > +             __cpuc_coherent_user_range(uaddr, uaddr + len);
> > +}
> 
> Does that do the right thing with uaddr + len == 0?

There is a corner case where uaddr + len == 0 and uaddr is at the top of
the 4GB range. Are there such platforms? A workaround is to change the
CPU-specific loop, probably to using BLT rather than BLO.

> Other variations such as writing when a mapping is !VM_EXEC and later
> mapping or mprotecting the same shmem VM_EXEC, but it's even more
> forbidden semantically to write to a read-only mapping (and just as
> unchecked on nommu), and conversion of writable to VM_EXEC ought to
> flush i-cache at mprotect time.

ARM Linux doesn't do any cache maintenance for mprotect on VIPT or noMMU
hardware (not sure about VIVT). We discussed this in the past and it
wasn't clear whether it is required or not.

-- 
Catalin

[PATCH] ARM: Implement copy_to_user_page() for noMMU

[Catalin Marinas]

On Tue, 2010-03-30 at 08:29 +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 30, 2010 at 02:31:32AM +0100, Jamie Lokier wrote:
> > Other variations such as writing when a mapping is !VM_EXEC and later
> > mapping or mprotecting the same shmem VM_EXEC, but it's even more
> > forbidden semantically to write to a read-only mapping (and just as
> > unchecked on nommu), and conversion of writable to VM_EXEC ought to
> > flush i-cache at mprotect time.
> 
> If NX isn't implemented, then arm_elf_read_implies_exec() must return 1
> for the CPU - so that any region that is marked readable will have
> VM_EXEC set.

If we run uClinux on ARMv6 hardware, we get the above function returning
0. Should we make this function conditional on CONFIG_MMU?

-- 
Catalin

[PATCH] ARM: Implement copy_to_user_page() for noMMU

[Jamie Lokier]

Catalin Marinas wrote:
> > Other variations such as writing when a mapping is !VM_EXEC and later
> > mapping or mprotecting the same shmem VM_EXEC, but it's even more
> > forbidden semantically to write to a read-only mapping (and just as
> > unchecked on nommu), and conversion of writable to VM_EXEC ought to
> > flush i-cache at mprotect time.
> 
> ARM Linux doesn't do any cache maintenance for mprotect on VIPT or noMMU
> hardware (not sure about VIVT). We discussed this in the past and it
> wasn't clear whether it is required or not.

I still think it ought to happen on mprotect, but maybe that's a
linux-arch discussion.  I saw IRIX actually has two PROT_EXEC flavours
for mprotect so you can choose.

Back to this, just to double check, what about mapping?  E.g. where a
shmem is mapped writable (but not executable), has executable code
written to it by ptrace, and is later mapped in another process and
executed.  Will the act of making the second mapping flush i-cache for
that range?  (No-MMU doesn't need to flush caches on task switch)

-- Jamie

[PATCH] ARM: Implement copy_to_user_page() for noMMU

[Jamie Lokier]

Catalin Marinas wrote:
> On Tue, 2010-03-30 at 08:29 +0100, Russell King - ARM Linux wrote:
> > On Tue, Mar 30, 2010 at 02:31:32AM +0100, Jamie Lokier wrote:
> > > Other variations such as writing when a mapping is !VM_EXEC and later
> > > mapping or mprotecting the same shmem VM_EXEC, but it's even more
> > > forbidden semantically to write to a read-only mapping (and just as
> > > unchecked on nommu), and conversion of writable to VM_EXEC ought to
> > > flush i-cache at mprotect time.
> > 
> > If NX isn't implemented, then arm_elf_read_implies_exec() must return 1
> > for the CPU - so that any region that is marked readable will have
> > VM_EXEC set.
> 
> If we run uClinux on ARMv6 hardware, we get the above function returning
> 0. Should we make this function conditional on CONFIG_MMU?

Good catch.  I suspect no-MMU implies read-implies-exec, because it
does... unless there is some exciting MPU setting against it.

If no-MMU implies read-implies-exec, there is no need to check VM_EXEC
in this no-MMU code, is there? ;-)

Actually I'm inclined to unconditionally flush i-cache on ptrace write
just because (a) ptrace write is not performance critical, and (b)
invalid target program behaviour (forgetting to set PROT_EXEC) should
not cause GDB setting a breakpoint to fail in such a subtle way.

-- Jamie

[PATCH] ARM: Implement copy_to_user_page() for noMMU

[Catalin Marinas]

On Tue, 2010-03-30 at 12:25 +0100, Jamie Lokier wrote:
> Catalin Marinas wrote:
> > > Other variations such as writing when a mapping is !VM_EXEC and later
> > > mapping or mprotecting the same shmem VM_EXEC, but it's even more
> > > forbidden semantically to write to a read-only mapping (and just as
> > > unchecked on nommu), and conversion of writable to VM_EXEC ought to
> > > flush i-cache at mprotect time.
> >
> > ARM Linux doesn't do any cache maintenance for mprotect on VIPT or noMMU
> > hardware (not sure about VIVT). We discussed this in the past and it
> > wasn't clear whether it is required or not.
> 
> I still think it ought to happen on mprotect, but maybe that's a
> linux-arch discussion.  I saw IRIX actually has two PROT_EXEC flavours
> for mprotect so you can choose.

You can search some discussions a few months ago with COW pages and
cache coherency. This is where mprotect was discussed as well. I think
without additional kernel API, we just end up with too much flushing on
VIPT hardware (by implementing flush_cache_page).

> Back to this, just to double check, what about mapping?  E.g. where a
> shmem is mapped writable (but not executable), has executable code
> written to it by ptrace, and is later mapped in another process and
> executed.  Will the act of making the second mapping flush i-cache for
> that range?  (No-MMU doesn't need to flush caches on task switch)

ptrace writes to user pages via copy_to_user_page() which does the
flushing if it's a text page (VM_EXEC). In the case you describe above
with shared mappings, you would not get any flushing. Is this a real
situation?

-- 
Catalin