[Qemu-devel] [PATCH for-2.8] exec.c: Fix breakpoint invalidation race

[Qemu-devel] [PATCH for-2.8] exec.c: Fix breakpoint invalidation race

[Peter Maydell]

A bug (1647683) was reported showing a crash when removing
breakpoints.  The reproducer was bisected to 3359baad when tb_flush
was finally made thread safe.  While in MTTCG the locking in
breakpoint_invalidate would have prevented any problems, but
currently tb_lock() is a NOP for system emulation.

The race is between a tb_flush from the gdbstub and the
tb_invalidate_phys_addr() in breakpoint_invalidate().

Ideally we'd have actual locking here; for the moment the
simple fix is to do a full tb_flush() for a bp invalidate,
since that is thread-safe even if no lock is taken.

Reported-by: Julian Brown <julian@codesourcery.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is quite similar to Alex's patch
http://patchwork.ozlabs.org/patch/703188/
("exec.c: simplify the breakpoint invalidation logic").
The difference is that this patch doesn't drop the
breakpoint_invalidate() function entirely. I think this
is better both for a future "correct fix" and as a
minimal "just fix this for 2.8 release" change.
---
 exec.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/exec.c b/exec.c
index 3d867f1..08c558e 100644
--- a/exec.c
+++ b/exec.c
@@ -684,28 +684,15 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
 #endif
 }
 
-#if defined(CONFIG_USER_ONLY)
-static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
-{
-    mmap_lock();
-    tb_lock();
-    tb_invalidate_phys_page_range(pc, pc + 1, 0);
-    tb_unlock();
-    mmap_unlock();
-}
-#else
 static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
 {
-    MemTxAttrs attrs;
-    hwaddr phys = cpu_get_phys_page_attrs_debug(cpu, pc, &attrs);
-    int asidx = cpu_asidx_from_attrs(cpu, attrs);
-    if (phys != -1) {
-        /* Locks grabbed by tb_invalidate_phys_addr */
-        tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as,
-                                phys | (pc & ~TARGET_PAGE_MASK));
-    }
+    /* Flush the whole TB as this will not have race conditions
+     * even if we don't have proper locking yet.
+     * Ideally we would just invalidate the TBs for the
+     * specified PC.
+     */
+    tb_flush(cpu);
 }
-#endif
 
 #if defined(CONFIG_USER_ONLY)
 void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.8] exec.c: Fix breakpoint invalidation race

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> A bug (1647683) was reported showing a crash when removing
> breakpoints.  The reproducer was bisected to 3359baad when tb_flush
> was finally made thread safe.  While in MTTCG the locking in
> breakpoint_invalidate would have prevented any problems, but
> currently tb_lock() is a NOP for system emulation.
>
> The race is between a tb_flush from the gdbstub and the
> tb_invalidate_phys_addr() in breakpoint_invalidate().
>
> Ideally we'd have actual locking here; for the moment the
> simple fix is to do a full tb_flush() for a bp invalidate,
> since that is thread-safe even if no lock is taken.
>
> Reported-by: Julian Brown <julian@codesourcery.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is quite similar to Alex's patch
> http://patchwork.ozlabs.org/patch/703188/
> ("exec.c: simplify the breakpoint invalidation logic").
> The difference is that this patch doesn't drop the
> breakpoint_invalidate() function entirely. I think this
> is better both for a future "correct fix" and as a
> minimal "just fix this for 2.8 release" change.
> ---
>  exec.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 3d867f1..08c558e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -684,28 +684,15 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  #endif
>  }
>
> -#if defined(CONFIG_USER_ONLY)
> -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
> -{
> -    mmap_lock();
> -    tb_lock();
> -    tb_invalidate_phys_page_range(pc, pc + 1, 0);
> -    tb_unlock();
> -    mmap_unlock();
> -}
> -#else
>  static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
>  {
> -    MemTxAttrs attrs;
> -    hwaddr phys = cpu_get_phys_page_attrs_debug(cpu, pc, &attrs);
> -    int asidx = cpu_asidx_from_attrs(cpu, attrs);
> -    if (phys != -1) {
> -        /* Locks grabbed by tb_invalidate_phys_addr */
> -        tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as,
> -                                phys | (pc & ~TARGET_PAGE_MASK));
> -    }
> +    /* Flush the whole TB as this will not have race conditions
> +     * even if we don't have proper locking yet.
> +     * Ideally we would just invalidate the TBs for the
> +     * specified PC.
> +     */
> +    tb_flush(cpu);
>  }
> -#endif
>
>  #if defined(CONFIG_USER_ONLY)
>  void cpu_watchpoint_remove_all(CPUState *cpu, int mask)

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [PATCH for-2.8] exec.c: Fix breakpoint invalidation race

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1370 bytes --]

On Tue, Dec 06, 2016 at 06:07:09PM +0000, Peter Maydell wrote:
> A bug (1647683) was reported showing a crash when removing
> breakpoints.  The reproducer was bisected to 3359baad when tb_flush
> was finally made thread safe.  While in MTTCG the locking in
> breakpoint_invalidate would have prevented any problems, but
> currently tb_lock() is a NOP for system emulation.
> 
> The race is between a tb_flush from the gdbstub and the
> tb_invalidate_phys_addr() in breakpoint_invalidate().
> 
> Ideally we'd have actual locking here; for the moment the
> simple fix is to do a full tb_flush() for a bp invalidate,
> since that is thread-safe even if no lock is taken.
> 
> Reported-by: Julian Brown <julian@codesourcery.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is quite similar to Alex's patch
> http://patchwork.ozlabs.org/patch/703188/
> ("exec.c: simplify the breakpoint invalidation logic").
> The difference is that this patch doesn't drop the
> breakpoint_invalidate() function entirely. I think this
> is better both for a future "correct fix" and as a
> minimal "just fix this for 2.8 release" change.
> ---
>  exec.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]