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

[David Daney <ddaney.cavm@gmail.com>]

On 03/21/2014 03:07 AM, Ralf Baechle wrote:
> 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?
>


The problem only happens when modifying target executable code through 
the ptrace() system call.

For all cases where a program is modifying its own executable memory, we 
require that it make the special mips cacheflush system call.

I don't object to modifying this file, but I wonder if the call to the 
flushing function should be pushed up into the ptrace() code.

David Daney


>    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,
>
>