[PATCH v2] ARM: uprobes need icache flush after xol write

[Victor Kamensky]

On 9 April 2014 11:45, Oleg Nesterov <oleg at 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
>