On 04/15, David Long wrote:
On 04/15/14 11:46, Oleg Nesterov wrote:
But. Please do not add copy_to_user_page() into copy_to_page() (as your patch did). This is certainly not what uprobe_write_opcode() wants, we do not want or need "flush" in this case. The same for __create_xol_area().
It looked me like a call to a new __copy_to_user_page(current->mm, ...) in xol_get_insn_slot() would be in line with David Miller's suggestion and would cure the problem on ARM (and hopefuly be more philosophically correct for all architectures):
David, let me repeat. I am not going to argue even if I obviously like the Linus's suggestion more.
I only argued with the suggestions to follow the __access_remote_vm() logic.
@@ -1297,13 +1298,11 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe) 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);
kaddr = kmap_atomic(area->page);
__copy_to_user_page(current->mm, area->page, xol_vaddr,
kaddr + (xol_vaddr & ~PAGE_MASK),
&uprobe->arch.ixol, sizeof(uprobe->arch.ixol), true);
kunmap_atomic(kaddr);
This all is fine, but you need to introduce __copy_to_user_page() first.
And you need to do this for every arch. And you need to verify that it does not need mmap_sem. It seems that at least alpha needs this lock. And as Russel pointed out (again, again, I could easily misunderstood him) you need preempt_disable() around memcpy + flush, so down_read(mmap_sem) should probably go into __copy_to_user_page(). And I am not sure this helper needs "struct mm_struct *mm" argument, but this is minor.
Finally, let me repeat, you should verify that this __copy_to_user_page(page, uaddr, kaddr) will not something bad if uaddr is not mmapped, or its mapping do not match area->page.
Good luck ;)
Oleg.