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