On 22 September 2016 22:38:33 BST, Leonid Yegoshin <Leonid.Yegoshin@xxxxxxxxxx> wrote:
>On 09/22/2016 02:15 PM, James Hogan wrote:
>> On Wed, Sep 21, 2016 at 11:15:55AM -0700, Leonid Yegoshin wrote:
>>> On 09/21/2016 06:26 AM, Ralf Baechle wrote:
>>>> On Thu, Sep 01, 2016 at 05:30:13PM +0100, James Hogan wrote:
>>>>
>>>>> Update arch_uprobe_copy_ixol() to use the kmap_atomic() based
>kernel
>>>>> address to flush the icache with flush_icache_range(), rather than
>the
>>>>> user mapping. We have the kernel mapping available anyway and this
>>>>> avoids having to switch to using the new
>__flush_icache_user_range() for
>>>>> the sake of Enhanced Virtual Addressing (EVA) where
>flush_icache_range()
>>>>> will become ineffective on user addresses.
>>>>>
>>>>> Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
>>>>> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
>>>>> Cc: Leonid Yegoshin <leonid.yegoshin@xxxxxxxxxx>
>>>>> Cc: linux-mips@xxxxxxxxxxxxxx
>>>>> ---
>>>>> arch/mips/kernel/uprobes.c | 13 ++++---------
>>>>> 1 file changed, 4 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/arch/mips/kernel/uprobes.c
>b/arch/mips/kernel/uprobes.c
>>>>> index 8452d933a645..9a507ab1ea38 100644
>>>>> --- a/arch/mips/kernel/uprobes.c
>>>>> +++ b/arch/mips/kernel/uprobes.c
>>>>> @@ -301,19 +301,14 @@ int set_orig_insn(struct arch_uprobe
>*auprobe, struct mm_struct *mm,
>>>>> void __weak arch_uprobe_copy_ixol(struct page *page, unsigned
>long vaddr,
>>>>> void *src, unsigned long len)
>>>>> {
>>>>> - void *kaddr;
>>>>> + void *kaddr, kstart;
>>>>>
>>>>> /* Initialize the slot */
>>>>> kaddr = kmap_atomic(page);
>>>>> - memcpy(kaddr + (vaddr & ~PAGE_MASK), src, len);
>>>>> + kstart = kaddr + (vaddr & ~PAGE_MASK);
>>>>> + memcpy(kstart, src, len);
>>>>> + flush_icache_range(kstart, kstart + len);
>>>>> kunmap_atomic(kaddr);
>>>>> -
>>>>> - /*
>>>>> - * The MIPS version of flush_icache_range will operate safely on
>>>>> - * user space addresses and more importantly, it doesn't require
>a
>>>>> - * VMA argument.
>>>>> - */
>>>>> - flush_icache_range(vaddr, vaddr + len);
>>>> I can't convince myself this is right wrt. to cache aliases ...
>>>>
>>>> Ralf
>>>>
>>> It is incorrect if there is I-cache aliasing (very rare) and there
>is no
>>> HIGHMEM cache aliasing fix (not fixed). But top tree Linux doesn't
>work
>>> with I-cache aliasing either.
>> Well its pretty trivial to just use the newly introduced
>> __flush_icache_user_range() on the user address instead of using
>> flush_icache_range().
>
>It may not work - you should flush kernel Dcache but user Icache and
>__flush_icache_user_range() doesn't do it.
well it'll do a protected dcache flush (i.e. using CACHEE with EVA). Would kmap/kunmap or variants (fixed to work with aliasing dcache) be able to take care of colouring / further flushing?
In any case, simply changing to the user_ one is a no-op compared to leaving as is where patch 9 would probably break it on EVA by making it operate only on kernel addresses.
Cheers
James
> Some CPU may accept an
>aliasing CACHE instruction and flush both colors and it can work in
>this
>case.
>
>Besides that, I had no time to research - does uprobe keep data on the
>same page as code? If yes, then we may want to make a special flush
>sequence for cache aliasing systems here. (User-Dcache, Write-to-page,
>Kernel-Dcache, User-Icache).
>
>- Leonid
--
James Hogan
