On 15 April 2014 08:46, Oleg Nesterov oleg@redhat.com wrote:
On 04/14, Victor Kamensky wrote:
On 14 April 2014 11:59, Oleg Nesterov oleg@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.