| Message ID | 20140409184507.GA1058@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 9 April 2014 11:45, Oleg Nesterov <oleg@redhat.com> wrote: > Victor, sorry, I can't even read this thread now, will try to reply > tomorrow... But at first glance, Sure, np. Will wait for you to free up. > On 04/08, Victor Kamensky wrote: >> >> Because on ARM cache flush function need kernel address > > ... > >> --- a/kernel/events/uprobes.c >> +++ b/kernel/events/uprobes.c >> @@ -1287,6 +1287,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe) >> { >> struct xol_area *area; >> unsigned long xol_vaddr; >> + void *xol_page_kaddr; >> >> area = get_xol_area(); >> if (!area) >> @@ -1296,14 +1297,22 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe) >> if (unlikely(!xol_vaddr)) >> return 0; >> >> - /* Initialize the slot */ >> - copy_to_page(area->page, xol_vaddr, >> - &uprobe->arch.ixol, sizeof(uprobe->arch.ixol)); >> /* >> - * We probably need flush_icache_user_range() but it needs vma. >> - * This should work on supported architectures too. >> + * We don't use copy_to_page here because we need kernel page >> + * addr to invalidate caches correctly >> */ >> - flush_dcache_page(area->page); >> + xol_page_kaddr = kmap_atomic(area->page); >> + >> + /* Initialize the slot */ >> + memcpy(xol_page_kaddr + (xol_vaddr & ~PAGE_MASK), >> + &uprobe->arch.ixol, >> + sizeof(uprobe->arch.ixol)); >> + >> + arch_uprobe_flush_xol_access(area->page, xol_vaddr, >> + xol_page_kaddr + (xol_vaddr & ~PAGE_MASK), >> + sizeof(uprobe->arch.ixol)); >> + >> + kunmap_atomic(xol_page_kaddr); >> >> return xol_vaddr; >> } >> @@ -1346,6 +1355,18 @@ static void xol_free_insn_slot(struct task_struct *tsk) >> } >> } >> >> +void __weak arch_uprobe_flush_xol_access(struct page *page, unsigned long vaddr, >> + void *kaddr, unsigned long len) >> +{ >> + /* >> + * We probably need flush_icache_user_range() but it needs vma. >> + * This should work on most of architectures by default. If >> + * architecture needs to do something different it can define >> + * its own version of the function. >> + */ >> + flush_dcache_page(page); >> +} > > In this case I'd suggest to move this kmap/memcpy horror into arch_ helper. > IOW, something like below. > > If arm needs the kernel address we are writing to, let it do kmap/etc in > its implementation. Fair enough. Can do that, as well as address Dave's comment about checkpatch. But before I do that let's reach some conclusion wrt latest Russell's remarks about copy_{to,from}_user_page usage in uprobes. It is bigger question covering more than ARM issue and possibly having bigger implication on uprobes design. I have some comments/thoughts about it, but since I am not uprobes maintainer, will wait for you to address it first. Thanks, Victor > Oleg. > > > --- x/kernel/events/uprobes.c > +++ x/kernel/events/uprobes.c > @@ -1291,18 +1291,16 @@ static unsigned long xol_get_insn_slot(s > if (unlikely(!xol_vaddr)) > return 0; > > - /* Initialize the slot */ > - copy_to_page(area->page, xol_vaddr, > - &uprobe->arch.ixol, sizeof(uprobe->arch.ixol)); > - /* > - * We probably need flush_icache_user_range() but it needs vma. > - * This should work on supported architectures too. > - */ > - flush_dcache_page(area->page); > - > + arch_uprobe_xxx(area->page, xol_vaddr, ...); > return xol_vaddr; > } > > +void __weak arch_uprobe_xxx(page, xol_vaddr, ...) > +{ > + copy_to_page(page, xol_vaddr, ...) > + flush_dcache_page(page); > +} > + > /* > * xol_free_insn_slot - If slot was earlier allocated by > * @xol_get_insn_slot(), make the slot available for >
On Wed, Apr 09, 2014 at 12:13:56PM -0700, Victor Kamensky wrote: > Fair enough. Can do that, as well as address Dave's comment about > checkpatch. > > But before I do that let's reach some conclusion wrt latest Russell's > remarks about copy_{to,from}_user_page usage in uprobes. It is bigger > question covering more than ARM issue and possibly having bigger > implication on uprobes design. I have some comments/thoughts about > it, but since I am not uprobes maintainer, will wait for you to address > it first. I encourage folk to read this thread: http://lkml.iu.edu//hypermail/linux/kernel/1404.1/00725.html and involve David in the loop - I suspect you need to sell whatever solution you agree on to David (as one of the other arch maintainers) as well. Thanks.
OK, here is an alternative solution for the kernel-dcache / user-icache flush issue in uprobes which I think follows Dave Miller's suggested approach. As a reminder: the goal is to make sure the user-space icache does not have stale data after the kernel rewrite of an instruction in the user's uprobe "execute out of line" (xol) page. It seems only ARM currently finds the flush_dcache_page() call insufficient, but then apparently only two architectures (other than ARM) support uprobes. I've modified events/uprobes.c to simply call the copy_to_user_page() function instead of doing a memcpy() followed by a flush_dcache_page() call. This results in a net reduction of one line of code in that file. Then I modified copy_to_user_page() and/or flushing function(s) it calls to treat a NULL vma pointer to mean: "assume the user icache address range is now invalid". In the majority of cases this is pretty basic and should be safe as nothing could have been doing this previously. In some cases this now results in flushing more icache than is necessary. For the mips, sh, sparc, and alpha architectures something more complicated is necessary and I have not currently done that. I am not certain this approach can be made to work cleanly for those architectures, although there is probably always the last resort of flushing all icache. On the other hand, it appears only x86, powerpc, and (god-willing) ARM currently support uprobes. I have only tested this on ARM (arndale) at this point. The preliminary patch follows in my next email. (BTW, is depending on the C compiler short-circuiting conditonals acceptable style?) -dl
--- x/kernel/events/uprobes.c +++ x/kernel/events/uprobes.c @@ -1291,18 +1291,16 @@ static unsigned long xol_get_insn_slot(s if (unlikely(!xol_vaddr)) return 0; - /* Initialize the slot */ - copy_to_page(area->page, xol_vaddr, - &uprobe->arch.ixol, sizeof(uprobe->arch.ixol)); - /* - * We probably need flush_icache_user_range() but it needs vma. - * This should work on supported architectures too. - */ - flush_dcache_page(area->page); - + arch_uprobe_xxx(area->page, xol_vaddr, ...); return xol_vaddr; } +void __weak arch_uprobe_xxx(page, xol_vaddr, ...) +{ + copy_to_page(page, xol_vaddr, ...) + flush_dcache_page(page); +} + /* * xol_free_insn_slot - If slot was earlier allocated by * @xol_get_insn_slot(), make the slot available for