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

[Victor Kamensky]

On 15 April 2014 08:46, Oleg Nesterov <oleg at redhat.com> wrote:
> On 04/14, Victor Kamensky wrote:
>>
>> On 14 April 2014 11:59, Oleg Nesterov <oleg at redhat.com> wrote:
>> > On 04/11, Linus Torvalds wrote:
>> >>
>> >> 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.
>
> We can add "vaddr, len" to the argument list.
>
>> I personally would prefer if we could have function like copy_to_user_page
>> but without requirement to pass vma to it.
>
> I won't argue, but you need to convince maintainers.
>
>
> And to remind, we need something simple/nonintrusive for arm right now.
> Again, I won't argue if we turn copy_to_page() + flush_dcache_page() into
> __weak arch_uprobe_copy_ixol(), and add the necessary hacks into arm's
> implementatiion. This is up to you and Russel.

For short term arm specific solution I will follow up on [1]. Yes, I
will incorporate
your request to make arch_uprobe_copy_ixol() instead of
arch_uprobe_flush_xol_access, will address Dave Long's
comments about checkpatch and will remove special handling for broadcast
situation (FLAG_UA_BROADCAST) since in further discussion it was
established that task can migrate while doing uprobes xol single stepping.

I don't think my patch does those things that you describe below. Anyway,
I will repost new version of short term arm specific fix proposal today PST
and we will see what Russell, you and all will say about it.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/245952.html
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/245743.html

Thanks,
Victor

>
>
> 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().
>
> Note also that currently copy_to_user_page() is always called under mmap_sem.
> I do not know if arm actually needs ->mmap_sem, but if you propose it as a
> generic solution we should probably take this lock.
>
> Also. Even if we have copy_to_user_page_no_vma() or change copy_to_user_page()
> to accept vma => NULL, I am not sure this will work fine on arm when the probed
> application unmaps xol_area and mmaps something else at the same vaddr. I mean,
> in this case we do not care if the application crashes, but please verify that
> something really bad can't happen.
>
> Let me repeat just in case, I know nothing about arm/. I can't even understand
> how, say, flush_pfn_alias() works, and how it should work if 2 threads call it
> at the same time with the same vaddr (or CACHE_COLOUR(vaddr)).
>
> Oleg.
>