[Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

[Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

[Peter Maydell]

If multiple guest threads in user-mode emulation write to a
page which QEMU has marked read-only because of cached TCG
translations, the threads can race in page_unprotect:

 * threads A & B both try to do a write to a page with code in it at
   the same time (ie which we've made non-writeable, so SEGV)
 * they race into the signal handler with this faulting address
 * thread A happens to get to page_unprotect() first and takes the
   mmap lock, so thread B sits waiting for it to be done
 * A then finds the page, marks it PAGE_WRITE and mprotect()s it writable
 * A can then continue OK (returns from signal handler to retry the
   memory access)
 * ...but when B gets the mmap lock it finds that the page is already
   PAGE_WRITE, and so it exits page_unprotect() via the "not due to
   protected translation" code path, and wrongly delivers the signal
   to the guest rather than just retrying the access

In particular, this meant that trying to run 'javac' in user-mode
emulation would fail with a spurious guest SIGSEGV.

Handle this by making page_unprotect() assume that a call for a page
which is already PAGE_WRITE is due to a race of this sort and return
a "fault handled" indication.

Since this would cause an infinite loop if we ever called
page_unprotect() for some other kind of fault than "write failed due
to bad access permissions", tighten the condition in
handle_cpu_signal() to check the signal number and si_code, and add a
comment so that if somebody does ever find themselves debugging an
infinite loop of faults they have some clue about why.

(The trick for identifying the correct setting for
current_tb_invalidated for thread B (needed to handle the precise-SMC
case) is due to Richard Henderson.  Paolo Bonzini suggested just
relying on si_code rather than trying anything more complicated.)

I'm slightly nervous about assuming that we get SEGV with SEGV_ACCERR
on a write if and only if the page permissions are set to non-writeable
(for instance https://www.mail-archive.com/tech@openbsd.org/msg40358.html
is a surprisingly recent patch to fix this for OpenBSD/x86, and I
would be unsurprised if QEMU usermode itself got the si_code wrong,
and the Boehm gc code at https://github.com/ivmai/bdwgc/blob/master/os_dep.c#L3182
is quite convoluted), but on the other hand we really ought in theory
to be able to rely on si_code, and trying to handle this some other way
gets pretty convoluted...

thanks
-- PMM

Peter Maydell (2):
  linux-user: Propagate siginfo_t through to handle_cpu_signal()
  page_unprotect(): handle calls to pages that are PAGE_WRITE

 accel/tcg/translate-all.c | 50 ++++++++++++++++++++++++++++-----------------
 accel/tcg/user-exec.c     | 52 +++++++++++++++++++++++------------------------
 2 files changed, 57 insertions(+), 45 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] linux-user: Propagate siginfo_t through to handle_cpu_signal()

[Peter Maydell]

Currently all the architecture/OS specific cpu_signal_handler()
functions call handle_cpu_signal() without passing it the
siginfo_t. We're going to want that so we can look at the si_code
to determine whether this is a SEGV_ACCERR access violation or
some other kind of fault, so change the functions to pass through
the pointer to the siginfo_t rather than just the si_addr value.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/user-exec.c | 39 ++++++++++++++-------------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index f42285e..e8f26ff 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -57,12 +57,13 @@ static void cpu_exit_tb_from_sighandler(CPUState *cpu, sigset_t *old_set)
    the effective address of the memory exception. 'is_write' is 1 if a
    write caused the exception and otherwise 0'. 'old_set' is the
    signal set which should be restored */
-static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
+static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
                                     int is_write, sigset_t *old_set)
 {
     CPUState *cpu = current_cpu;
     CPUClass *cc;
     int ret;
+    unsigned long address = (unsigned long)info->si_addr;
 
     /* We must handle PC addresses from two different sources:
      * a call return address and a signal frame address.
@@ -215,9 +216,8 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 #endif
     pc = EIP_sig(uc);
     trapno = TRAP_sig(uc);
-    return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             trapno == 0xe ?
-                             (ERROR_sig(uc) >> 1) & 1 : 0,
+    return handle_cpu_signal(pc, info,
+                             trapno == 0xe ? (ERROR_sig(uc) >> 1) & 1 : 0,
                              &MASK_sig(uc));
 }
 
@@ -261,9 +261,8 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 #endif
 
     pc = PC_sig(uc);
-    return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             TRAP_sig(uc) == 0xe ?
-                             (ERROR_sig(uc) >> 1) & 1 : 0,
+    return handle_cpu_signal(pc, info,
+                             TRAP_sig(uc) == 0xe ? (ERROR_sig(uc) >> 1) & 1 : 0,
                              &MASK_sig(uc));
 }
 
@@ -341,8 +340,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
         is_write = 1;
     }
 #endif
-    return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, &uc->uc_sigmask);
+    return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 
 #elif defined(__alpha__)
@@ -372,8 +370,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
         is_write = 1;
     }
 
-    return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, &uc->uc_sigmask);
+    return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 #elif defined(__sparc__)
 
@@ -432,8 +429,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
             break;
         }
     }
-    return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, sigmask);
+    return handle_cpu_signal(pc, info, is_write, sigmask);
 }
 
 #elif defined(__arm__)
@@ -466,9 +462,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
      * later processor; on v5 we will always report this as a read).
      */
     is_write = extract32(uc->uc_mcontext.error_code, 11, 1);
-    return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write,
-                             &uc->uc_sigmask);
+    return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 
 #elif defined(__aarch64__)
@@ -495,8 +489,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, void *puc)
                 /* Ignore bits 23 & 24, controlling indexing.  */
                 || (insn & 0x3a400000) == 0x28000000); /* C3.3.7,14-16 */
 
-    return handle_cpu_signal(pc, (uintptr_t)info->si_addr,
-                             is_write, &uc->uc_sigmask);
+    return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 
 #elif defined(__ia64)
@@ -529,9 +522,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, void *puc)
     default:
         break;
     }
-    return handle_cpu_signal(ip, (unsigned long)info->si_addr,
-                             is_write,
-                             (sigset_t *)&uc->uc_sigmask);
+    return handle_cpu_signal(ip, info, is_write, (sigset_t *)&uc->uc_sigmask);
 }
 
 #elif defined(__s390__)
@@ -583,8 +574,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
         }
         break;
     }
-    return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, &uc->uc_sigmask);
+    return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 
 #elif defined(__mips__)
@@ -599,8 +589,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 
     /* XXX: compute is_write */
     is_write = 0;
-    return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, &uc->uc_sigmask);
+    return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 
 #else
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] page_unprotect(): handle calls to pages that are PAGE_WRITE

[Peter Maydell]

If multiple guest threads in user-mode emulation write to a
page which QEMU has marked read-only because of cached TCG
translations, the threads can race in page_unprotect:

 * threads A & B both try to do a write to a page with code in it at
   the same time (ie which we've made non-writeable, so SEGV)
 * they race into the signal handler with this faulting address
 * thread A happens to get to page_unprotect() first and takes the
   mmap lock, so thread B sits waiting for it to be done
 * A then finds the page, marks it PAGE_WRITE and mprotect()s it writable
 * A can then continue OK (returns from signal handler to retry the
   memory access)
 * ...but when B gets the mmap lock it finds that the page is already
   PAGE_WRITE, and so it exits page_unprotect() via the "not due to
   protected translation" code path, and wrongly delivers the signal
   to the guest rather than just retrying the access

In particular, this meant that trying to run 'javac' in user-mode
emulation would fail with a spurious guest SIGSEGV.

Handle this by making page_unprotect() assume that a call for a page
which is already PAGE_WRITE is due to a race of this sort and return
a "fault handled" indication.

Since this would cause an infinite loop if we ever called
page_unprotect() for some other kind of fault than "write failed due
to bad access permissions", tighten the condition in
handle_cpu_signal() to check the signal number and si_code, and add a
comment so that if somebody does ever find themselves debugging an
infinite loop of faults they have some clue about why.

(The trick for identifying the correct setting for
current_tb_invalidated for thread B (needed to handle the precise-SMC
case) is due to Richard Henderson.  Paolo Bonzini suggested just
relying on si_code rather than trying anything more complicated.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/translate-all.c | 50 +++++++++++++++++++++++++++++------------------
 accel/tcg/user-exec.c     | 13 +++++++++++-
 2 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index e7f0329..96e68d5 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2182,29 +2182,41 @@ int page_unprotect(target_ulong address, uintptr_t pc)
 
     /* if the page was really writable, then we change its
        protection back to writable */
-    if ((p->flags & PAGE_WRITE_ORG) && !(p->flags & PAGE_WRITE)) {
-        host_start = address & qemu_host_page_mask;
-        host_end = host_start + qemu_host_page_size;
-
-        prot = 0;
+    if (p->flags & PAGE_WRITE_ORG) {
         current_tb_invalidated = false;
-        for (addr = host_start ; addr < host_end ; addr += TARGET_PAGE_SIZE) {
-            p = page_find(addr >> TARGET_PAGE_BITS);
-            p->flags |= PAGE_WRITE;
-            prot |= p->flags;
-
-            /* and since the content will be modified, we must invalidate
-               the corresponding translated code. */
-            current_tb_invalidated |= tb_invalidate_phys_page(addr, pc);
-#ifdef CONFIG_USER_ONLY
-            if (DEBUG_TB_CHECK_GATE) {
-                tb_invalidate_check(addr);
+        if (p->flags & PAGE_WRITE) {
+            /* If the page is actually marked WRITE then assume this is because
+             * this thread raced with another one which got here first and
+             * set the page to PAGE_WRITE and did the TB invalidate for us.
+             */
+#ifdef TARGET_HAS_PRECISE_SMC
+            TranslationBlock *current_tb = tb_find_pc(pc);
+            if (current_tb) {
+                current_tb_invalidated = tb_cflags(current_tb) & CF_INVALID;
             }
 #endif
+        } else {
+            host_start = address & qemu_host_page_mask;
+            host_end = host_start + qemu_host_page_size;
+
+            prot = 0;
+            for (addr = host_start; addr < host_end; addr += TARGET_PAGE_SIZE) {
+                p = page_find(addr >> TARGET_PAGE_BITS);
+                p->flags |= PAGE_WRITE;
+                prot |= p->flags;
+
+                /* and since the content will be modified, we must invalidate
+                   the corresponding translated code. */
+                current_tb_invalidated |= tb_invalidate_phys_page(addr, pc);
+#ifdef CONFIG_USER_ONLY
+                if (DEBUG_TB_CHECK_GATE) {
+                    tb_invalidate_check(addr);
+                }
+#endif
+            }
+            mprotect((void *)g2h(host_start), qemu_host_page_size,
+                     prot & PAGE_BITS);
         }
-        mprotect((void *)g2h(host_start), qemu_host_page_size,
-                 prot & PAGE_BITS);
-
         mmap_unlock();
         /* If current TB was invalidated return to main loop */
         return current_tb_invalidated ? 2 : 1;
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index e8f26ff..c973752 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -104,7 +104,18 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
            pc, address, is_write, *(unsigned long *)old_set);
 #endif
     /* XXX: locking issue */
-    if (is_write && h2g_valid(address)) {
+    /* Note that it is important that we don't call page_unprotect() unless
+     * this is really a "write to nonwriteable page" fault, because
+     * page_unprotect() assumes that if it is called for an access to
+     * a page that's writeable this means we had two threads racing and
+     * another thread got there first and already made the page writeable;
+     * so we will retry the access. If we were to call page_unprotect()
+     * for some other kind of fault that should really be passed to the
+     * guest, we'd end up in an infinite loop of retrying the faulting
+     * access.
+     */
+    if (is_write && info->si_signo == SIGSEGV && info->si_code == SEGV_ACCERR &&
+        h2g_valid(address)) {
         switch (page_unprotect(h2g(address), pc)) {
         case 0:
             /* Fault not caused by a page marked unwritable to protect
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] linux-user: Propagate siginfo_t through to handle_cpu_signal()

[Richard Henderson]

On 11/28/2017 02:35 PM, Peter Maydell wrote:
> Currently all the architecture/OS specific cpu_signal_handler()
> functions call handle_cpu_signal() without passing it the
> siginfo_t. We're going to want that so we can look at the si_code
> to determine whether this is a SEGV_ACCERR access violation or
> some other kind of fault, so change the functions to pass through
> the pointer to the siginfo_t rather than just the si_addr value.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  accel/tcg/user-exec.c | 39 ++++++++++++++-------------------------
>  1 file changed, 14 insertions(+), 25 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

[Peter Maydell]

On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> If multiple guest threads in user-mode emulation write to a
> page which QEMU has marked read-only because of cached TCG
> translations, the threads can race in page_unprotect:

> Peter Maydell (2):
>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>   page_unprotect(): handle calls to pages that are PAGE_WRITE

Ping! Linux-user maintainers, any chance this could get into
a pull-request sometime soon? (I have another cleanup I'm
thinking of that will touch the same code so I'd rather this
went into master before I look at that.)

(I have a bunch of other pending linux-user patchsets which
I shan't bother to ping individually unless you want me to.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

[Laurent Vivier]

Le 15/01/2018 à 13:48, Peter Maydell a écrit :
> On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>> If multiple guest threads in user-mode emulation write to a
>> page which QEMU has marked read-only because of cached TCG
>> translations, the threads can race in page_unprotect:
> 
>> Peter Maydell (2):
>>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>>   page_unprotect(): handle calls to pages that are PAGE_WRITE
> 
> Ping! Linux-user maintainers, any chance this could get into
> a pull-request sometime soon? (I have another cleanup I'm
> thinking of that will touch the same code so I'd rather this
> went into master before I look at that.)
> 
> (I have a bunch of other pending linux-user patchsets which
> I shan't bother to ping individually unless you want me to.)

Riku,

if you have no time to prepare a pull request, I can.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

[Riku Voipio]

Hi Laurent,

On Mon, Jan 15, 2018 at 01:52:32PM +0100, Laurent Vivier wrote:
> Le 15/01/2018 à 13:48, Peter Maydell a écrit :
> > On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> If multiple guest threads in user-mode emulation write to a
> >> page which QEMU has marked read-only because of cached TCG
> >> translations, the threads can race in page_unprotect:
> > 
> >> Peter Maydell (2):
> >>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
> >>   page_unprotect(): handle calls to pages that are PAGE_WRITE
> > 
> > Ping! Linux-user maintainers, any chance this could get into
> > a pull-request sometime soon? (I have another cleanup I'm
> > thinking of that will touch the same code so I'd rather this
> > went into master before I look at that.)
> > 
> > (I have a bunch of other pending linux-user patchsets which
> > I shan't bother to ping individually unless you want me to.)
> 
> Riku,
> 
> if you have no time to prepare a pull request, I can.

Thanks, that would be great.

Riku

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

[Laurent Vivier]

Le 19/01/2018 à 15:51, Riku Voipio a écrit :
> Hi Laurent,
> 
> On Mon, Jan 15, 2018 at 01:52:32PM +0100, Laurent Vivier wrote:
>> Le 15/01/2018 à 13:48, Peter Maydell a écrit :
>>> On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> If multiple guest threads in user-mode emulation write to a
>>>> page which QEMU has marked read-only because of cached TCG
>>>> translations, the threads can race in page_unprotect:
>>>
>>>> Peter Maydell (2):
>>>>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>>>>   page_unprotect(): handle calls to pages that are PAGE_WRITE
>>>
>>> Ping! Linux-user maintainers, any chance this could get into
>>> a pull-request sometime soon? (I have another cleanup I'm
>>> thinking of that will touch the same code so I'd rather this
>>> went into master before I look at that.)
>>>
>>> (I have a bunch of other pending linux-user patchsets which
>>> I shan't bother to ping individually unless you want me to.)
>>
>> Riku,
>>
>> if you have no time to prepare a pull request, I can.
> 
> Thanks, that would be great.

OK, I'm going to prepare it.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

[Laurent Vivier]

Le 15/01/2018 à 13:48, Peter Maydell a écrit :
> On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>> If multiple guest threads in user-mode emulation write to a
>> page which QEMU has marked read-only because of cached TCG
>> translations, the threads can race in page_unprotect:
> 
>> Peter Maydell (2):
>>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>>   page_unprotect(): handle calls to pages that are PAGE_WRITE
> 
> Ping! Linux-user maintainers, any chance this could get into
> a pull-request sometime soon? (I have another cleanup I'm
> thinking of that will touch the same code so I'd rather this
> went into master before I look at that.)
> 
> (I have a bunch of other pending linux-user patchsets which
> I shan't bother to ping individually unless you want me to.)
> 
> thanks
> -- PMM
> 

I've applied this series to my linux-user branch.

I've tried to find all pending linux-user patches.

If you want to check none of yours is is missing, all patches I have
collected are in:

https://github.com/vivier/qemu/commits/linux-user-for-2.12

I'm going to run some tests before sending the pull request.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

[Laurent Vivier]

Le 19/01/2018 à 15:51, Riku Voipio a écrit :
> Hi Laurent,
> 
> On Mon, Jan 15, 2018 at 01:52:32PM +0100, Laurent Vivier wrote:
>> Le 15/01/2018 à 13:48, Peter Maydell a écrit :
>>> On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> If multiple guest threads in user-mode emulation write to a
>>>> page which QEMU has marked read-only because of cached TCG
>>>> translations, the threads can race in page_unprotect:
>>>
>>>> Peter Maydell (2):
>>>>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>>>>   page_unprotect(): handle calls to pages that are PAGE_WRITE
>>>
>>> Ping! Linux-user maintainers, any chance this could get into
>>> a pull-request sometime soon? (I have another cleanup I'm
>>> thinking of that will touch the same code so I'd rather this
>>> went into master before I look at that.)
>>>
>>> (I have a bunch of other pending linux-user patchsets which
>>> I shan't bother to ping individually unless you want me to.)
>>
>> Riku,
>>
>> if you have no time to prepare a pull request, I can.
> 
> Thanks, that would be great.

Riku, if you agree, I can continue to collect patches for the next
pull-request. It's easier to collect patches on the fly than scanning
the ML when we want to create a pull request.

Thanks,
Laurent