[Qemu-devel] [PATCH] linux-user: Fix stale tbs after mmap

[Qemu-devel] [PATCH] linux-user: Fix stale tbs after mmap

[Alexander Graf]

If we execute linux-user code that does the following:

  * A = mmap()
  * execute code in A
  * munmap(A)
  * B = mmap(), but mmap returns the same address as A
  * execute code in B

we end up executing a stale cached tb that contains translated code
from A, while we want new code from B.

This patch adds a TB flush for mmap'ed regions, before we return them,
avoiding the whole issue. It also adds a flush for munmap, so that we
don't execute stale TBs instead of getting a segfault.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - rebase on current git
  - fix munmap too
---
 exec-all.h        |    2 ++
 exec.c            |   17 +++++++++++++++++
 linux-user/mmap.c |    6 +++++-
 3 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/exec-all.h b/exec-all.h
index c1b7e1f..9bda7f7 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -96,6 +96,8 @@ void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
 int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
 void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
                                    int is_cpu_write_access);
+void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end,
+                              int is_cpu_write_access);
 #if !defined(CONFIG_USER_ONLY)
 /* cputlb.c */
 void tlb_flush_page(CPUArchState *env, target_ulong addr);
diff --git a/exec.c b/exec.c
index 0607c9b..a0494c7 100644
--- a/exec.c
+++ b/exec.c
@@ -1075,6 +1075,23 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
     return tb;
 }
 
+/*
+ * invalidate all TBs which intersect with the target physical pages
+ * starting in range [start;end[. NOTE: start and end may refer to
+ * different physical pages. 'is_cpu_write_access' should be true if called
+ * from a real cpu write access: the virtual CPU will exit the current
+ * TB if code is modified inside this TB.
+ */
+void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end,
+                              int is_cpu_write_access)
+{
+    while (start < end) {
+        tb_invalidate_phys_page_range(start, end, is_cpu_write_access);
+        start &= TARGET_PAGE_MASK;
+        start += TARGET_PAGE_SIZE;
+    }
+}
+
 /* invalidate all TBs which intersect with the target physical page
    starting in range [start;end[. NOTE: start and end must refer to
    the same physical page. 'is_cpu_write_access' should be true if called
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 7125d1c..d9468fe 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -573,6 +573,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     page_dump(stdout);
     printf("\n");
 #endif
+    tb_invalidate_phys_range(start, start + len, 0);
     mmap_unlock();
     return start;
 fail:
@@ -675,8 +676,10 @@ int target_munmap(abi_ulong start, abi_ulong len)
         }
     }
 
-    if (ret == 0)
+    if (ret == 0) {
         page_set_flags(start, start + len, 0);
+        tb_invalidate_phys_range(start, start + len, 0);
+    }
     mmap_unlock();
     return ret;
 }
@@ -754,6 +757,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
         page_set_flags(old_addr, old_addr + old_size, 0);
         page_set_flags(new_addr, new_addr + new_size, prot | PAGE_VALID);
     }
+    tb_invalidate_phys_range(new_addr, new_addr + new_size, 0);
     mmap_unlock();
     return new_addr;
 }
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH] linux-user: Fix stale tbs after mmap

[Peter Maydell]

On 11 May 2012 09:40, Alexander Graf <agraf@suse.de> wrote:
> If we execute linux-user code that does the following:
>
>  * A = mmap()
>  * execute code in A
>  * munmap(A)
>  * B = mmap(), but mmap returns the same address as A
>  * execute code in B
>
> we end up executing a stale cached tb that contains translated code
> from A, while we want new code from B.
>
> This patch adds a TB flush for mmap'ed regions, before we return them,
> avoiding the whole issue. It also adds a flush for munmap, so that we
> don't execute stale TBs instead of getting a segfault.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH 1.1] linux-user: Fix stale tbs after mmap

[Peter Maydell]

Ping? This is 1.1 material in my opinion...

(patchwork url: http://patchwork.ozlabs.org/patch/158556/)

-- PMM

On 11 May 2012 17:25, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 May 2012 09:40, Alexander Graf <agraf@suse.de> wrote:
>> If we execute linux-user code that does the following:
>>
>>  * A = mmap()
>>  * execute code in A
>>  * munmap(A)
>>  * B = mmap(), but mmap returns the same address as A
>>  * execute code in B
>>
>> we end up executing a stale cached tb that contains translated code
>> from A, while we want new code from B.
>>
>> This patch adds a TB flush for mmap'ed regions, before we return them,
>> avoiding the whole issue. It also adds a flush for munmap, so that we
>> don't execute stale TBs instead of getting a segfault.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> -- PMM

Re: [Qemu-devel] [PATCH 1.1] linux-user: Fix stale tbs after mmap

[Anthony Liguori]

Riku,

Can you review/ack this patch?

Regards,

Anthony Liguori


On 05/15/2012 03:35 PM, Peter Maydell wrote:
> Ping? This is 1.1 material in my opinion...
>
> (patchwork url: http://patchwork.ozlabs.org/patch/158556/)
>
> -- PMM
>
> On 11 May 2012 17:25, Peter Maydell<peter.maydell@linaro.org>  wrote:
>> On 11 May 2012 09:40, Alexander Graf<agraf@suse.de>  wrote:
>>> If we execute linux-user code that does the following:
>>>
>>>   * A = mmap()
>>>   * execute code in A
>>>   * munmap(A)
>>>   * B = mmap(), but mmap returns the same address as A
>>>   * execute code in B
>>>
>>> we end up executing a stale cached tb that contains translated code
>>> from A, while we want new code from B.
>>>
>>> This patch adds a TB flush for mmap'ed regions, before we return them,
>>> avoiding the whole issue. It also adds a flush for munmap, so that we
>>> don't execute stale TBs instead of getting a segfault.
>>>
>>> Reported-by: Peter Maydell<peter.maydell@linaro.org>
>>> Signed-off-by: Alexander Graf<agraf@suse.de>
>>
>> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
>>
>> -- PMM
>

Re: [Qemu-devel] [PATCH 1.1] linux-user: Fix stale tbs after mmap

[Riku Voipio]

On Tue, May 15, 2012 at 04:32:51PM -0500, Anthony Liguori wrote:
> Riku,
> 
> Can you review/ack this patch?

Acked-by: Riku Voipio <riku.voipio@linaro.org>

Are there any other Linux-user patches to consider for 1.1 ?

> 
> Regards,
> 
> Anthony Liguori
> 
> 
> On 05/15/2012 03:35 PM, Peter Maydell wrote:
> >Ping? This is 1.1 material in my opinion...
> >
> >(patchwork url: http://patchwork.ozlabs.org/patch/158556/)
> >
> >-- PMM
> >
> >On 11 May 2012 17:25, Peter Maydell<peter.maydell@linaro.org>  wrote:
> >>On 11 May 2012 09:40, Alexander Graf<agraf@suse.de>  wrote:
> >>>If we execute linux-user code that does the following:
> >>>
> >>>  * A = mmap()
> >>>  * execute code in A
> >>>  * munmap(A)
> >>>  * B = mmap(), but mmap returns the same address as A
> >>>  * execute code in B
> >>>
> >>>we end up executing a stale cached tb that contains translated code
> >>>from A, while we want new code from B.
> >>>
> >>>This patch adds a TB flush for mmap'ed regions, before we return them,
> >>>avoiding the whole issue. It also adds a flush for munmap, so that we
> >>>don't execute stale TBs instead of getting a segfault.
> >>>
> >>>Reported-by: Peter Maydell<peter.maydell@linaro.org>
> >>>Signed-off-by: Alexander Graf<agraf@suse.de>
> >>
> >>Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> >>
> >>-- PMM
> >

Re: [Qemu-devel] [PATCH 1.1] linux-user: Fix stale tbs after mmap

[Andreas Färber]

Am 16.05.2012 11:26, schrieb Riku Voipio:
> On Tue, May 15, 2012 at 04:32:51PM -0500, Anthony Liguori wrote:
>> Riku,
>>
>> Can you review/ack this patch?
> 
> Acked-by: Riku Voipio <riku.voipio@linaro.org>
> 
> Are there any other Linux-user patches to consider for 1.1 ?

I had noticed that Alex' binfmt wrappers did not get applied, but
although they fix argv[0] issues they're more of a feature than a bugfix.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg