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

[Victor Kamensky]

On 14 April 2014 11:59, Oleg Nesterov <oleg at redhat.com> wrote:
> On 04/11, Linus Torvalds wrote:
>>
>> On Fri, Apr 11, 2014 at 10:24 AM, Oleg Nesterov <oleg at redhat.com> wrote:
>> > +static void arch_uprobe_copy_ixol(struct xol_area *area, unsigned long vaddr,
>> > +                                       struct arch_uprobe *auprobe)
>> > +{
>> > +#ifndef ARCH_UPROBE_XXX
>> > +       copy_to_page(area->page, vaddr, &auprobe->ixol, sizeof(&auprobe->ixol));
>> > +       /*
>> > +        * We probably need flush_icache_user_range() but it needs vma.
>> > +        * If this doesn't work define ARCH_UPROBE_XXX.
>> > +        */
>> > +       flush_dcache_page(area->page);
>> > +#else
>> > +       struct mm_struct *mm = current->mm;
>> > +       struct vm_area_struct *vma;
>> > +
>> > +       down_read(&mm->mmap_sem);
>> > +       vma = find_exact_vma(mm, area->vaddr, area->vaddr + PAGE_SIZE);
>> > +       if (vma) {
>> > +               void *kaddr = kmap_atomic(area->page);
>> > +               copy_to_user_page(vma, area->page,
>> > +                                       vaddr, kaddr + (vaddr & ~PAGE_MASK),
>> > +                                       &auprobe->ixol, sizeof(&auprobe->ixol));
>> > +               kunmap_atomic(kaddr);
>> > +       }
>> > +       up_read(&mm->mmap_sem);
>> > +#endif
>>
>> Yeah, no, this is wrong.
>
> Yesss, agreed.
>
>> So I really think we should just have a fixed
>> "flush_icache_page(page,vaddr)" function.
>> ...
>> Then the uprobe case can just do
>>
>>     copy_to_page()
>>     flush_dcache_page()
>>     flush_icache_page()
>
>
> And I obviously like this idea because (iiuc) it more or less matches
> flush_icache_page_xxx() I tried to suggest.

Would not page granularity to be too expensive? Note you need to do that on
each probe hit and you flushing whole data and instruction page every time.
IMHO it will work correctly when you flush just few dcache/icache lines that
correspond to xol slot that got modified. Note copy_to_user_page takes
len that describes size of area that has to be flushed. Given that we are
flushing xol area page at this case; and nothing except one xol slot is
any interest for current task, and if CPU can flush one dcache and icache
page as quickly as it can flush range, my remark may not matter.

I personally would prefer if we could have function like copy_to_user_page
but without requirement to pass vma to it.

Thanks,
Victor

> But we need a short term solution for arm. And unless I misunderstood
> Russell (this is quite possible), arm needs to disable preemption around
> copy + flush.
>
> Russel, so what do you think we can do for arm right now? Does the patch
> above (and subsequent discussion) answer the "why reinvent" question ?
>
> Oleg.
>