Re: [PATCH] MIPS: copy_to_user_page: Avoid ptrace(2) I-cache incoherency

[Ralf Baechle <ralf@linux-mips.org>]

On Thu, Nov 07, 2013 at 06:35:54PM +0000, Maciej W. Rozycki wrote:

> We currently support no MIPS processor that has its I-cache coherent with 
> the D-cache, no such processor may even exist.  We apparently have two 
> configurations that have fully coherent D-caches and therefore set 
> cpu_has_ic_fills_f_dc, and these are the Alchemy and NetLogic processor 
> families.  I have checked relevant CPU documentation I was able to track 
> down and in both cases the respective documents[1][2] clearly state that 
> the I-cache provides no hardware coherency and whenever instructions in 
> memory are modified then the I-cache has to be synchronized by software 
> even though the D-caches are fully coherent.

No, cpu_has_ic_fills_f_dc doesn't mean the D-cache is coherent but rather
that the I-cache is refilled from the D-cache if there was a hit.  This
means there is no need to write back the D-cache to S-cache or even memory
which is saving some time.

> Therefore we cannot ever avoid the call to flush_cache_page in 
> copy_to_user_page and here is a change that reflects this observation.  
> The implementation of flush_cache_page may then choose freely whether it 
> needs to perform a full cache synchronization with D-cache writeback and 
> invalidation requests or whether a lone I-cache invalidation will suffice.  
> The c-r4k.c implementation already respects the setting of 
> cpu_has_ic_fills_f_dc and avoids touching the D-cache unless necessary.
> 
> The lack of I-cache synchronization is typically seen in debugging 
> sessions e.g. with GDB where software breakpoints are used.  When such a 
> breakpoint is hit and subsequently replaced using a ptrace(2) call with 
> the original instruction, the BREAK instruction previously executed 
> sometimes remains in the I-cache and causes the breakpoint just removed to 
> hit again regardless, resulting in a spurious SIGTRAP signal delivery that 
> debuggers typically complain about (e.g. "Program received signal SIGTRAP, 
> Trace/breakpoint trap" in the case of GDB).  Of course the I-cache line 
> containing the BREAK instruction may have since been randomly replaced, in 
> which case no problem occurs.
> 
> [1] "AMD Alchemy Au1200 Processor Data Book", AMD Alchemy, January, 2005, 
>     Publication ID: 32798A
> 
> [2] "XLP Processor Family Programming Reference Manual", NetLogic 
>     Microsystems, Revision Level 1.10, February, 2011, Document Number 
>     10724V110PM-CR (regrettably not publicly available)
> 
> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
> ---
> Ralf,
> 
>  Please apply.  I've seen these SIGTRAPs in some NetLogic GDB testing and 
> the removal of this cpu_has_ic_fills_f_dc condition from copy_to_user_page 
> is really necessary; also the Au1200 document is very explicit about the 
> requirement of I-cache invalidation in software (see Section 2.3.7.3 
> "Instruction Cache Coherency").

You found a bug and yes, the fix you sent improves things a bit.  But
there is also the cache on a cache coherent system where a page might
be marked for a delayed cache flush with SetPageDcacheDirty(), then
flushed by flush_cache_page() before eventually the delayed cacheflush
flushes it once more for a good meassure.

What do you think about below patch to also deal with the duplicate flushing?

  Ralf

 arch/mips/mm/init.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/mm/init.c b/arch/mips/mm/init.c
index 6b59617..80072ef 100644
--- a/arch/mips/mm/init.c
+++ b/arch/mips/mm/init.c
@@ -227,13 +227,13 @@ void copy_to_user_page(struct vm_area_struct *vma,
 		void *vto = kmap_coherent(page, vaddr) + (vaddr & ~PAGE_MASK);
 		memcpy(vto, src, len);
 		kunmap_coherent();
+		if (vma->vm_flags & VM_EXEC)
+			flush_cache_page(vma, vaddr, page_to_pfn(page));
 	} else {
 		memcpy(dst, src, len);
 		if (cpu_has_dc_aliases)
 			SetPageDcacheDirty(page);
 	}
-	if ((vma->vm_flags & VM_EXEC) && !cpu_has_ic_fills_f_dc)
-		flush_cache_page(vma, vaddr, page_to_pfn(page));
 }
 
 void copy_from_user_page(struct vm_area_struct *vma,