[Qemu-devel] [PATCH for-2.0? 0/2] fix bugs involving linux-user signal handling

[Qemu-devel] [PATCH for-2.0? 0/2] fix bugs involving linux-user signal handling

[Peter Maydell]

This patch series fixes bugs reported by Andrei Warkentin involving
signal handling in linux-user mode. The first is Andrei's first patch
(though I have tweaked the commit message a little). The second
patch is aimed at fixing the locking bug that Andrei noted, in
a somewhat simpler way than his patches use.

The test cases Andrei provided both pass with these patches.

(The tb_lock is a completely ill-thought-out idea anyway; maybe
one day we'll actually sort out how multiple linux-user threads
should interact with the TCG data structures. But for now this
fixes an obvious bug.)

Andrei Warkentin (1):
  page_check_range: don't bail out early after unprotecting page

Peter Maydell (1):
  cpu-exec: Unlock tb_lock if we longjmp out of code generation

 cpu-exec.c      | 7 +++++++
 translate-all.c | 1 -
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
1.9.0

[Qemu-devel] [PATCH for-2.0? 1/2] page_check_range: don't bail out early after unprotecting page

[Peter Maydell]

From: Andrei Warkentin <andrey.warkentin@gmail.com>

When checking a page range, if we found that a page was
made read-only by QEMU because it contained translated code,
we were incorrectly returning immediately after unprotecting
that page, rather than continuing to check the entire range,
so we might fail to unprotect pages later in the range, or
might incorrectly return a "success" result even if later
pages were not writable.

In particular, this could cause segfaults in a case where
signals are delivered back to back on a target architecture
which uses trampoline code in the stack frame (as AArch64
currently does). The second signal causes a segfault because
the frame cannot be written to (it was protected because
we translated and executed the restorer trampoline, and the
unprotect logic did not unprotect the whole range).

Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com
[PMM: expanded commit message a bit]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 translate-all.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/translate-all.c b/translate-all.c
index f243c10..5759974 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1777,7 +1777,6 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
                     return -1;
                 }
             }
-            return 0;
         }
     }
     return 0;
-- 
1.9.0

Re: [Qemu-devel] [PATCH for-2.0? 1/2] page_check_range: don't bail out early after unprotecting page

[Richard Henderson]

On 04/03/2014 09:45 AM, Peter Maydell wrote:
> From: Andrei Warkentin <andrey.warkentin@gmail.com>
> 
> When checking a page range, if we found that a page was
> made read-only by QEMU because it contained translated code,
> we were incorrectly returning immediately after unprotecting
> that page, rather than continuing to check the entire range,
> so we might fail to unprotect pages later in the range, or
> might incorrectly return a "success" result even if later
> pages were not writable.
> 
> In particular, this could cause segfaults in a case where
> signals are delivered back to back on a target architecture
> which uses trampoline code in the stack frame (as AArch64
> currently does). The second signal causes a segfault because
> the frame cannot be written to (it was protected because
> we translated and executed the restorer trampoline, and the
> unprotect logic did not unprotect the whole range).
> 
> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com
> [PMM: expanded commit message a bit]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  translate-all.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[Qemu-devel] [PATCH for-2.0? 2/2] cpu-exec: Unlock tb_lock if we longjmp out of code generation

[Peter Maydell]

If the guest attempts to execute from unreadable memory, this will
cause us to longjmp back to the main loop from inside the
target frontend decoder. For linux-user mode, this means we will
still hold the tb_ctx.tb_lock, and will deadlock when we try to
start executing code again. Unlock the lock in the return-from-longjmp
code path to avoid this.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpu-exec.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/cpu-exec.c b/cpu-exec.c
index 0914d3c..02168d9 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -227,6 +227,8 @@ int cpu_exec(CPUArchState *env)
     TranslationBlock *tb;
     uint8_t *tc_ptr;
     uintptr_t next_tb;
+    /* This must be volatile so it is not trashed by longjmp() */
+    volatile bool have_tb_lock = false;
 
     if (cpu->halted) {
         if (!cpu_has_work(cpu)) {
@@ -600,6 +602,7 @@ int cpu_exec(CPUArchState *env)
                     cpu_loop_exit(cpu);
                 }
                 spin_lock(&tcg_ctx.tb_ctx.tb_lock);
+                have_tb_lock = true;
                 tb = tb_find_fast(env);
                 /* Note: we do it here to avoid a gcc bug on Mac OS X when
                    doing it in tb_find_slow */
@@ -621,6 +624,7 @@ int cpu_exec(CPUArchState *env)
                     tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
                                 next_tb & TB_EXIT_MASK, tb);
                 }
+                have_tb_lock = false;
                 spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
 
                 /* cpu_interrupt might be called while translating the
@@ -692,6 +696,9 @@ int cpu_exec(CPUArchState *env)
 #ifdef TARGET_I386
             x86_cpu = X86_CPU(cpu);
 #endif
+            if (have_tb_lock) {
+                spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
+            }
         }
     } /* for(;;) */
 
-- 
1.9.0

Re: [Qemu-devel] [PATCH for-2.0? 2/2] cpu-exec: Unlock tb_lock if we longjmp out of code generation

[Richard Henderson]

On 04/03/2014 09:45 AM, Peter Maydell wrote:
> +            if (have_tb_lock) {
> +                spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
> +            }

It ought not matter, since we ought to exit the loop on the next round, but i
have a strong preference for resetting have_tb_lock here.

Otherwise, this is nicely self-contained.


r~

Re: [Qemu-devel] [PATCH for-2.0? 2/2] cpu-exec: Unlock tb_lock if we longjmp out of code generation

[Peter Maydell]

On 3 April 2014 17:51, Richard Henderson <rth@twiddle.net> wrote:
> On 04/03/2014 09:45 AM, Peter Maydell wrote:
>> +            if (have_tb_lock) {
>> +                spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
>> +            }
>
> It ought not matter, since we ought to exit the loop on the
> next round, but i have a strong preference for resetting
> have_tb_lock here.

Absolutely -- dumb oversight on my part.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.0? 2/2] cpu-exec: Unlock tb_lock if we longjmp out of code generation

[Andrei E. Warkentin]

Hiya,

Cool. Definitely more compact and less intrusive, and definitely
should catch more issues than the original page->flags check. The only
possible cost is maintenance and debugging (implicit state and all
that)... so... How about adding a comment around the "if
(have_tb_lock)" to explain how we can get there?

Acked-by: Andrei Warkentin <andrey.warkentin@gmail.com>

2014-04-03 12:53 GMT-04:00 Peter Maydell <peter.maydell@linaro.org>:
> On 3 April 2014 17:51, Richard Henderson <rth@twiddle.net> wrote:
>> On 04/03/2014 09:45 AM, Peter Maydell wrote:
>>> +            if (have_tb_lock) {
>>> +                spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
>>> +            }
>>
>> It ought not matter, since we ought to exit the loop on the
>> next round, but i have a strong preference for resetting
>> have_tb_lock here.
>
> Absolutely -- dumb oversight on my part.
>
> thanks
> -- PMM



-- 
A

Re: [Qemu-devel] [PATCH for-2.0? 2/2] cpu-exec: Unlock tb_lock if we longjmp out of code generation

[Peter Maydell]

On 3 April 2014 20:38, Andrei E. Warkentin <andrey.warkentin@gmail.com> wrote:
> Hiya,
>
> Cool. Definitely more compact and less intrusive, and definitely
> should catch more issues than the original page->flags check. The only
> possible cost is maintenance and debugging (implicit state and all
> that)... so... How about adding a comment around the "if
> (have_tb_lock)" to explain how we can get there?

I dunno, it seems fairly obvious to me that if you get to this point
with have_tb_lock true then it must be because you longjumped
out of the codegen. (This happens for softmmu as well as linux-user,
it's just softmmu doesn't actually do any tb locking).

thanks
-- PMM