On 21 April 2014 09:16, David Long dave.long@linaro.org wrote:
On 04/16/14 18:25, Russell King - ARM Linux wrote:
Our I-caches don't snoop/see the D-cache at all - so writes need to be pushed out to what we call the "point of unification" where the I and D streams meet. For anything we care about, that's normally the L2 cache - L1 cache is harvard, L2 cache is unified.
Hence, we don't care which D-alias (if any) the data is written, so long as it's pushed out of the L1 data cache so that it's visible to the L1 instruction cache.
If we're writing via a different mapping to that which is being executed, I think the safest thing to do is to flush it out of the L1 D-cache at the address it was written, and then flush any stale line from the L1 I-cache using the user address. This is quite a unique requirement, and we don't have anything which covers it. The closest you could get is to that using existing calls is:
- write the new instruction
- flush_dcache_page()
- flush_cache_user_range() using the user address
and I think that should be safe on all the above cache types.
It doesn't feel to me like we yet have a clear consensus on the appropriate near or long-term fix for this problem. I'm worried time is short to get a fix in for v3.15. I'm not sure how elegant that fix needs to be. I've gotten good test runs using a modified/simplified version of Victor's arch callback and a slight variation of Russell's sequence of operations from above:
void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, const void *src, int len) { void *kaddr = kmap_atomic(page);
#ifdef CONFIG_SMP preempt_disable(); #endif memcpy(kaddr + (vaddr & ~PAGE_MASK), src, len); clean_dcache_area(kaddr, len); flush_cache_user_range(vaddr, vaddr + len);
I wonder would flush_cache_user_range addresses other cores icache invaludation issue? We discussed that user-land task while doing uprobes single step could be migrated to another core. So if ARM cpu that has cache_ops_need_broadcast() to true and migration happens, other core icache may still has old stale instruction by this address. Note ARM ptrace breakpoint write code in flush_ptrace_access deals with such situation.
Given that cache_ops_need_broadcast() true is not typical and cache invalidation in this case could be slow, but we would like to be functionally correct even in such situations, rather than experience very very rare mysterious crash of user-land process under the trace.
#ifdef CONFIG_SMP preempt_enable(); #endif kunmap_atomic(kaddr); }
I have to say using clean_dcache_area() to write back the two words that changed (and rest of the cache line of course) seems more appropriate than flushing a whole page. Are there implications in doing that which makes this a bad idea though?
At any rate, for v3.15 do we want to persue the more complex solutions with "congruent" mappings and use of copy_to_user(), or just something like the above (plus the rest of Victors v3 patch)? I'm sure Oleg is even less happy than me about yet another arch_ callback but we can hold out the hope that a more elegant solution can follow in the next release. One that might introduce risk we can't accept in v3.15 right now (e.g.: mapping the xol area writeable for all architectures).
I have also tested (somewhat) both Victor's unmodified v3 and v4 patches on exynos 5250 and found them to work.
It seems to me that we cannot find common solution for this issue and arch specific callback should be introduced.
If arch callback is introduced I will be more comfortable to keep current behavior as default and as far as ARM specific implementation is concerned it would be good to have code/logic sharing with code that deals with ptrace breakpoint write.
IMHO such solution would be something like V3 [1] version of the patch, Note [1] also needs to address Linus's comment about removing '#ifdef CONFIG_SMP' in code sequence similar to above.
Thanks, Victor
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/247793.html
Thanks, -dl