[RFC PATCH] uprobes: copy to user-space xol page with proper cache flushing

[oleg@redhat.com (Oleg Nesterov)]

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.