[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.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/mmap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 2620f88..390e940 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     page_dump(stdout);
     printf("\n");
 #endif
+    tb_invalidate_phys_page_range(start, start + len, 0);
     mmap_unlock();
     return start;
 fail:
@@ -768,6 +769,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_page_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 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>     page_dump(stdout);
>     printf("\n");
>  #endif
> +    tb_invalidate_phys_page_range(start, start + len, 0);
>     mmap_unlock();
>     return start;

The comment at the top of tb_invalidate_phys_page_range() says
"start and end must refer to the same physical page" -- is it
out of date or does that not apply to user-mode?

Do you need to also invalidate the range on munmap() and
mprotect-to-not-executable in order to correctly fault on
the case of:
  map something
  execute it
  unmap it
  try to execute it again

? (haven't tested that case but it seems like it might be an issue)

-- PMM

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

[Alexander Graf]

On 07.05.2012, at 12:37, Peter Maydell wrote:

> On 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
>> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>     page_dump(stdout);
>>     printf("\n");
>>  #endif
>> +    tb_invalidate_phys_page_range(start, start + len, 0);
>>     mmap_unlock();
>>     return start;
> 
> The comment at the top of tb_invalidate_phys_page_range() says
> "start and end must refer to the same physical page" -- is it
> out of date or does that not apply to user-mode?

:(

No, you're right. It only flushes the first page.

> Do you need to also invalidate the range on munmap() and
> mprotect-to-not-executable in order to correctly fault on
> the case of:
>  map something
>  execute it
>  unmap it
>  try to execute it again
> 
> ? (haven't tested that case but it seems like it might be an issue)

I'm not sure. But it's an unrelated issue either way, right? :)
Could you please try to quickly write a test case for this one while I fix the patch?


Alex

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

[Alexander Graf]

On 07.05.2012, at 12:37, Peter Maydell wrote:

> On 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
>> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>     page_dump(stdout);
>>     printf("\n");
>>  #endif
>> +    tb_invalidate_phys_page_range(start, start + len, 0);
>>     mmap_unlock();
>>     return start;
> 
> The comment at the top of tb_invalidate_phys_page_range() says
> "start and end must refer to the same physical page" -- is it
> out of date or does that not apply to user-mode?

How about this one?


diff --git a/exec-all.h b/exec-all.h
index c211242..7fcd76f 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -96,6 +96,8 @@ void QEMU_NORETURN cpu_loop_exit(CPUState *env1);
 int page_unprotect(target_ulong address, unsigned long 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);
 void tlb_flush_page(CPUState *env, target_ulong addr);
 void tlb_flush(CPUState *env, int flush_global);
 #if !defined(CONFIG_USER_ONLY)
diff --git a/exec.c b/exec.c
index e71a82d..532b00c 100644
--- a/exec.c
+++ b/exec.c
@@ -1016,6 +1016,23 @@ TranslationBlock *tb_gen_code(CPUState *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 2620f88..3611deb 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -587,6 +587,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:
@@ -768,6 +769,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;
 }

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

[Alexander Graf]

On 07.05.2012, at 12:37, Peter Maydell wrote:

> On 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
>> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>     page_dump(stdout);
>>     printf("\n");
>>  #endif
>> +    tb_invalidate_phys_page_range(start, start + len, 0);
>>     mmap_unlock();
>>     return start;
> 
> The comment at the top of tb_invalidate_phys_page_range() says
> "start and end must refer to the same physical page" -- is it
> out of date or does that not apply to user-mode?
> 
> Do you need to also invalidate the range on munmap() and
> mprotect-to-not-executable in order to correctly fault on
> the case of:
>  map something
>  execute it
>  unmap it
>  try to execute it again
> 
> ? (haven't tested that case but it seems like it might be an issue)

Yeah, the issue does exist:

#include <stdio.h>
#include <sys/mman.h>
#include <string.h>
#include <unistd.h>
#include <stdlib.h>

static int foo(void)
{
    return 5;
}

int main(int argc, char **argv)
{
    void *p;
    int x;
    int (*f)(void);

    p = mmap(NULL,0x1000,PROT_EXEC|PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS,0,0);
    if (!p) {
        printf("Error: mmap returned failure\n");
        exit(1);
    }
    memcpy(p, (void*)foo, 0x10);
    f = p;
    x = f();
    printf("returned %d\n", x);
    munmap(p, 0x1000);
    x = f();
    printf("returned %d\n", x);
}

-----

baur:/> ./test
returned 5
returned 5

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

[Alexander Graf]

On 07.05.2012, at 13:32, Alexander Graf wrote:

> 
> On 07.05.2012, at 12:37, Peter Maydell wrote:
> 
>> On 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
>>> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>>    page_dump(stdout);
>>>    printf("\n");
>>> #endif
>>> +    tb_invalidate_phys_page_range(start, start + len, 0);
>>>    mmap_unlock();
>>>    return start;
>> 
>> The comment at the top of tb_invalidate_phys_page_range() says
>> "start and end must refer to the same physical page" -- is it
>> out of date or does that not apply to user-mode?
>> 
>> Do you need to also invalidate the range on munmap() and
>> mprotect-to-not-executable in order to correctly fault on
>> the case of:
>> map something
>> execute it
>> unmap it
>> try to execute it again
>> 
>> ? (haven't tested that case but it seems like it might be an issue)
> 
> Yeah, the issue does exist:

And the below patch on top of my revised patch fixes it. The question is whether we still need to flush on mmap() then?


Alex

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 3611deb..bb4e752 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -690,8 +690,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;
 }

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

[Peter Maydell]

On 7 May 2012 12:38, Alexander Graf <agraf@suse.de> wrote:
> And the below patch on top of my revised patch fixes it.
> The question is whether we still need to flush on mmap() then?

IIRC the kernel will let you MAP_FIXED mmap a file to an
address that's already mmap'd for something else without
having to munmap the previous mapping. So yes, we still
need to flush on mmap I think.

Also I think you need mprotect() for the case of:
  mmap(...,PROT_EXEC)
  execute
  mprotect(not PROT_EXEC)
  try to execute again

-- PMM

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

[Peter Maydell]

On 7 May 2012 12:38, Alexander Graf <agraf@suse.de> wrote:
>
> On 07.05.2012, at 13:32, Alexander Graf wrote:
>
>>
>> On 07.05.2012, at 12:37, Peter Maydell wrote:
>>
>>> On 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
>>>> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>>>    page_dump(stdout);
>>>>    printf("\n");
>>>> #endif
>>>> +    tb_invalidate_phys_page_range(start, start + len, 0);
>>>>    mmap_unlock();
>>>>    return start;
>>>
>>> The comment at the top of tb_invalidate_phys_page_range() says
>>> "start and end must refer to the same physical page" -- is it
>>> out of date or does that not apply to user-mode?
>>>
>>> Do you need to also invalidate the range on munmap() and
>>> mprotect-to-not-executable in order to correctly fault on
>>> the case of:
>>> map something
>>> execute it
>>> unmap it
>>> try to execute it again
>>>
>>> ? (haven't tested that case but it seems like it might be an issue)
>>
>> Yeah, the issue does exist:
>
> And the below patch on top of my revised patch fixes it.

I think these two patches look correct (and as you pointed out
on irc I was wrong about mprotect, which effectively already
handles flushing the tb if needed). If you can roll them together
into a single patch with a commit message and signed-off-by
you can add my Reviewed-by: tag to it.

thanks
-- PMM

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

[Alexander Graf]

On 11.05.2012, at 17:46, Peter Maydell wrote:

> On 7 May 2012 12:38, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 07.05.2012, at 13:32, Alexander Graf wrote:
>> 
>>> 
>>> On 07.05.2012, at 12:37, Peter Maydell wrote:
>>> 
>>>> On 7 May 2012 10:30, Alexander Graf <agraf@suse.de> wrote:
>>>>> @@ -587,6 +587,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>>>>    page_dump(stdout);
>>>>>    printf("\n");
>>>>> #endif
>>>>> +    tb_invalidate_phys_page_range(start, start + len, 0);
>>>>>    mmap_unlock();
>>>>>    return start;
>>>> 
>>>> The comment at the top of tb_invalidate_phys_page_range() says
>>>> "start and end must refer to the same physical page" -- is it
>>>> out of date or does that not apply to user-mode?
>>>> 
>>>> Do you need to also invalidate the range on munmap() and
>>>> mprotect-to-not-executable in order to correctly fault on
>>>> the case of:
>>>> map something
>>>> execute it
>>>> unmap it
>>>> try to execute it again
>>>> 
>>>> ? (haven't tested that case but it seems like it might be an issue)
>>> 
>>> Yeah, the issue does exist:
>> 
>> And the below patch on top of my revised patch fixes it.
> 
> I think these two patches look correct (and as you pointed out
> on irc I was wrong about mprotect, which effectively already
> handles flushing the tb if needed). If you can roll them together
> into a single patch with a commit message and signed-off-by
> you can add my Reviewed-by: tag to it.

Well, if we invalidate on unmap, do we need to invalidate on mmap() still? Or is only invalidating on unmap enough? Maybe when you use fixed addresses... hrm

Ah whatever, let's just flush everywhere now and then optimize later.


Alex

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

[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