[PATCH] NOMMU: use copy_*_user_page() in access_process_vm()
in [Kernel]
|
From: Mike Frysinger on 25 Nov 2009 01:00 From: Jie Zhang <jie.zhang(a)analog.com> The mmu code uses the copy_*_user_page() variants in access_process_vm() rather than copy_*_user() as the former includes an icache flush. This is important when doing things like setting software breakpoints with gdb. So switch the nommu code over to do the same. Signed-off-by: Jie Zhang <jie.zhang(a)analog.com> Signed-off-by: Mike Frysinger <vapier(a)gentoo.org> --- mm/nommu.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/mm/nommu.c b/mm/nommu.c index 9876fa0..51ae9be 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -1889,9 +1889,11 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in /* only read or write mappings where it is permitted */ if (write && vma->vm_flags & VM_MAYWRITE) - len -= copy_to_user((void *) addr, buf, len); + copy_to_user_page(vma, NULL, NULL, + (void *) addr, buf, len); else if (!write && vma->vm_flags & VM_MAYREAD) - len -= copy_from_user(buf, (void *) addr, len); + copy_from_user_page(vma, NULL, NULL, + buf, (void *) addr, len); else len = 0; } else { -- 1.6.5.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jamie Lokier on 25 Nov 2009 01:20 Mike Frysinger wrote: > From: Jie Zhang <jie.zhang(a)analog.com> > > The mmu code uses the copy_*_user_page() variants in access_process_vm() > rather than copy_*_user() as the former includes an icache flush. This is > important when doing things like setting software breakpoints with gdb. > So switch the nommu code over to do the same. Reasonable, but it's a bit subtle don't you think? How about a one-line comment saying why it's using copy_*_user_page()? (If it was called copy_*_user_flush_icache() I wouldn't say anything, but it isn't). > Signed-off-by: Jie Zhang <jie.zhang(a)analog.com> > Signed-off-by: Mike Frysinger <vapier(a)gentoo.org> > --- > mm/nommu.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/mm/nommu.c b/mm/nommu.c > index 9876fa0..51ae9be 100644 > --- a/mm/nommu.c > +++ b/mm/nommu.c > @@ -1889,9 +1889,11 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in > > /* only read or write mappings where it is permitted */ > if (write && vma->vm_flags & VM_MAYWRITE) > - len -= copy_to_user((void *) addr, buf, len); > + copy_to_user_page(vma, NULL, NULL, > + (void *) addr, buf, len); > else if (!write && vma->vm_flags & VM_MAYREAD) > - len -= copy_from_user(buf, (void *) addr, len); > + copy_from_user_page(vma, NULL, NULL, > + buf, (void *) addr, len); > else > len = 0; > } else { -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jie Zhang on 25 Nov 2009 01:30 On 11/25/2009 02:16 PM, Jamie Lokier wrote: > Mike Frysinger wrote: >> From: Jie Zhang<jie.zhang(a)analog.com> >> >> The mmu code uses the copy_*_user_page() variants in access_process_vm() >> rather than copy_*_user() as the former includes an icache flush. This is >> important when doing things like setting software breakpoints with gdb. >> So switch the nommu code over to do the same. > > Reasonable, but it's a bit subtle don't you think? > How about a one-line comment saying why it's using copy_*_user_page()? > > (If it was called copy_*_user_flush_icache() I wouldn't say anything, > but it isn't). > But I think it's well known in Linux kernel developers that copy_to_user_page and copy_from_user_page should do cache flushing. It's documented in Documentation/cachetlb.txt. I don't think it's necessary to replicate it here. Jie -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Paul Mundt on 25 Nov 2009 02:00 On Wed, Nov 25, 2009 at 02:27:22PM +0800, Jie Zhang wrote: > On 11/25/2009 02:16 PM, Jamie Lokier wrote: > >Mike Frysinger wrote: > >>From: Jie Zhang<jie.zhang(a)analog.com> > >> > >>The mmu code uses the copy_*_user_page() variants in access_process_vm() > >>rather than copy_*_user() as the former includes an icache flush. This is > >>important when doing things like setting software breakpoints with gdb. > >>So switch the nommu code over to do the same. > > > >Reasonable, but it's a bit subtle don't you think? > >How about a one-line comment saying why it's using copy_*_user_page()? > > > >(If it was called copy_*_user_flush_icache() I wouldn't say anything, > >but it isn't). > > > But I think it's well known in Linux kernel developers that > copy_to_user_page and copy_from_user_page should do cache flushing. It's > documented in Documentation/cachetlb.txt. I don't think it's necessary > to replicate it here. > Documenting it in the changelog is sufficient I think. Platforms that need the I-cache flush can deal with it there, and those that don't aren't going to notice any difference regardless. The change in semantics is subtle, but as it's bringing it in line with MMU behaviour it's not really worth noting the fact that NOMMU behaviour used to be different for no particular reason. Acked-by: Paul Mundt <lethal(a)linux-sh.org> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jamie Lokier on 25 Nov 2009 06:50
Jie Zhang wrote: > On 11/25/2009 02:16 PM, Jamie Lokier wrote: > >Mike Frysinger wrote: > >>From: Jie Zhang<jie.zhang(a)analog.com> > >> > >>The mmu code uses the copy_*_user_page() variants in access_process_vm() > >>rather than copy_*_user() as the former includes an icache flush. This is > >>important when doing things like setting software breakpoints with gdb. > >>So switch the nommu code over to do the same. > > > >Reasonable, but it's a bit subtle don't you think? > >How about a one-line comment saying why it's using copy_*_user_page()? > > > >(If it was called copy_*_user_flush_icache() I wouldn't say anything, > >but it isn't). > > > But I think it's well known in Linux kernel developers that > copy_to_user_page and copy_from_user_page should do cache flushing. It's > documented in Documentation/cachetlb.txt. I don't think it's necessary > to replicate it here. You're right, however I now think the commit message is misleading. Since this is the *only place in the entire kernel* where these functions are used (plus the mmu equivalent), I'm not sure I'd agree about well known, and the name could be better (copy_*_user_ptrace()), but I agree now, it doesn't need a comment. It was the talk of icache flush which bothered me, as I (wrongly) assumed copy_*_user_page() was used elsewhere, without knowledge of icache vs non-icache differences - which are often the responsibility of userspace to get right, so often the kernel does not care. In fact, it's not just icache. copy_*_user_page() has to do some *data* cache flushing too, on some architecures. For example, see arch/sparc/include/asm/cacheflush_64.h: #define copy_to_user_page(vma, page, vaddr, dst, src, len) \ do { \ flush_cache_page(vma, vaddr, page_to_pfn(page)); \ memcpy(dst, src, len); \ flush_ptrace_access(vma, page, vaddr, src, len, 0); \ } while (0) #define copy_from_user_page(vma, page, vaddr, dst, src, len) \ do { \ flush_cache_page(vma, vaddr, page_to_pfn(page)); \ memcpy(dst, src, len); \ flush_ptrace_access(vma, page, vaddr, dst, len, 1); \ } while (0) I'm not sure why I don't see the same dcache flushing on ARM, so I wonder if the ARM implementation of these buggy. Anyway, that means the commit message is a little misleading, saying it's for the icache flush. It is for whatever icache and dcache flushes are needed for a ptrace access. Which is why, given they are only used for ptrace (have just grepped), I'm inclined to think it'd be clearer to rename the functions to copy_*_user_ptrace(). And make your no-mmu change of course :-) Any thoughts on the rename? -- Jamie -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |