On Tue, Apr 08, 2014 at 08:35:01AM -0700, Victor Kamensky wrote:
Looking at flush_ptrace_access more closely. Now I am not sure that ptrace write code could easily reused.
- flush_ptrace_access seems to handle both data and text segments
write. In case of xol write we always know that it is code write
Of course it has to, but writing code is the harder of the two problems. With writes to data segments, the only thing that has to be dealt with is the data cache. With code, not only do you need to deal with the data cache, but you also need to deal with the instruction cache too.
- as I pointed before flush_ptrace_access handles smp case whereas
xol write does not need to do that
Are you sure about that?
If I'm reading the code correctly, uprobes inserts a trapping instruction into the userspace program. When that instruction is hit, it checks whether the thread is the desired one, and may request a slot in this magic page, which is when the write happens.
The uprobes special page is shared across all threads which share the mm_struct, so in the case of a multi-threaded program running on a SMP machine, this page is visible to multiple CPUs.
Is it possible for uprobes to be active on more than one thread at a time? If so, because that page is shared, you could end up writing to a partial cache line from two threads. From what I can see, ixol[] is two words, and there's normally 8 works per cache line on ARM, or occasionally 16.
So, the question now is: is it possible to have uprobes active on more than one thread, and for two threads to hit the uprobes processing, both needing a slot in the page, hitting the same cache line?
Now, what happens if thread 1 on CPU1 gets there first with its write. Then thread 2 on CPU2 gets there, causing the cache line to migrate to CPU2. Then CPU1 does it's (non-broadcasted) flush, meanwhile CPU2 then gets preempted and goes off and does something else.
Please tell me that can't happen. :)
- flush_ptrace_access needs vma, which uprobe code does not have on
layer where xol is installed. That is probably most critical issue that could stop suggested code sharing. And note that vma in flush_ptrace_access is needed only for cases cases 1) and 2) above, about which xol write does not really care. If we take only required pieces from flush_ptrace_access would not xol cache flush look like this:
void arch_uprobe_xol_sync_dcache_icache(struct page *page, unsigned long addr, unsigned long size) { if (icache_is_vipt_aliasing()) flush_icache_alias(page_to_pfn(page), addr, size); else __cpuc_coherent_user_range(addr, size); }
And this is only half the story.
What you're saying above is:
- if the *instruction* cache is an aliasing VIPT cache, then we can map the user page at the same colour and flush the data and instruction caches according to that colour. Problem: if you have a VIPT aliasing data cache, and you wrote to the page via a mapping which had a different colour (which is highly likely given you're using kmap_atomic()) then you are not flushing the new instructions out of the data cache. - if the *instruction* cache is not an aliasing VIPT cache, then we flush using the userspace mapping directly. Problem: what if the data cache is aliasing... same problem as the above case.
This is why we have the check for cache_is_vipt_aliasing() in there - that's not _just_ to deal with ptrace data modification, it's there for text modification as well.
Note on the side: flush_ptrace_access works with user-land memory but it still uses __cpuc_coherent_kern_range. I think it should use __cpuc_coherent_user_range.
No, because __cpuc_coherent_kern_range() there is used on the *kernel* mapping of the page, not on the *user* mapping (which may fault). The kernel mapping will never fault.
It looks like implementation wise it is the same but user variant seems more appropriate in given situation. Or am I missing something here?
They do seem to be the same for both cases, so we could probably remove the distinction. It's unlikely we'll ever see an implementation needing a difference between the two now.