Re: [PATCH 1/1] arm64: fix flush_cache_range

[Leizhen (ThunderTown)]

On 2016/5/24 21:02, Catalin Marinas wrote:
> On Tue, May 24, 2016 at 08:19:05PM +0800, Leizhen (ThunderTown) wrote:
>> On 2016/5/24 19:37, Mark Rutland wrote:
>>> On Tue, May 24, 2016 at 07:16:37PM +0800, Zhen Lei wrote:
>>>> When we ran mprotect04(a test case in LTP) infinitely, it would always
>>>> failed after a few seconds. The case can be described briefly that: copy
>>>> a empty function from code area into a new memory area(created by mmap),
>>>> then call mprotect to change the protection to PROT_EXEC. The syscall
>>>> sys_mprotect will finally invoke flush_cache_range, but this function
>>>> currently only invalid icache, the operation of flush dcache is missed.
>>>
>>> In the LTP code I see powerpc-specific D-cache / I-cache synchronisation
>>> (i.e. d-cache cleaning followed by I-cache invalidation), so there
>>> appears to be some expectation of userspace maintenance. Hoever, there
>>> is no such ARM-specific I-cache maintenance.
>>
>> But I see some other platforms have D-cache maintenance, like: arch/nios2/mm/cacheflush.c
>> And according to the name of flush_cache_range, it should do this, I judged. Otherwise,
>> mprotect04 will be failed on more platforms, it's easy to discover. Only PPC have specific
>> cache synchronization, maybe it meets some hardware limitation. It's impossible a programmer
>> fixed a common bug on only one platform but leave others unchanged.
> 
> flush_cache_range() is primarily used on VIVT caches before changing the
> mapping and should not really be implemented on arm64. I don't recall
> why we still have the I-cache invalidation, possibly for the ASID-tagged
> VIVT I-cache case, though we should have a specific check for this.
> 
> There are some other cases where flush_cache_range() is called and no
> D-cache maintenance is necessary on arm64, so I don't want to penalise
> them by implementing flush_cache_range().
> 
>>> It looks like the test may be missing I-cache maintenance regardless of
>>> the semantics of mprotect in this case.
>>>
>>> I have not yet devled into flush_cache_range and how it is called.
>>
>> SYSCALL_DEFINE3(mprotect ---> mprotect_fixup ---> change_protection ---> change_protection_range --> flush_cache_range
> 
> The change_protection() shouldn't need to flush the caches in
> flush_cache_range(). The change_pte_range() function eventually ends up
> calling set_pte_at() which calls __sync_icache_dcache() if the mapping
> is executable.

OK, I see.
But I'm afraid it entered the "if (pte_present(oldpte))" branch in function change_pte_range.
Because the test case called mmap to create pte first, then called pte_modify.
I will check it later.

> 
> Can you be more specific about the kernel version you are using, its
> configuration?
> 
I used the latest mainline kernel version, and built with arch/arm64/configs/defconfig, ran on our D02 board.
I have attached the testcase, you can simply run: sh test.sh

Attachment: mprotect04
Description: Binary data

#!/bin/bash

i=0
while [ 1 ]
do
	./mprotect04
	if [ $? -ne 0 ]; then
		echo "i=$i, failed"
		exit 1
	fi

	i=$(($i+1))

done