Re: [PATCH] accel/tcg: Do not chain from a TB freed by tb_gen_code's flush

["Alex Bennée" <alex.bennee@linaro.org>]

Stian Halseth <stian@itx.no> writes:

> In a serial context, tb_gen_code() flushes a full code buffer and
> retries in place. cpu_exec_loop() then chains its last_tb, now freed,
> into the new TB. When that TB is invalidated, tb_jmp_unlink() follows
> the stale entry and qemu crashes inside its SIGSEGV handler.

I wonder how much:

        /* flush must be done */
        if (cpu_in_serial_context(cpu)) {
            trace_tb_gen_code_buffer_overflow("tcg_tb_alloc");
            tb_flush__exclusive_or_serial();
            goto buffer_overflow;
        }

actually saves? Because we are skipping both cleanup in cpu_loop_exit
(the mem helpers) and cpu_exec_longjmp_cleanup which we get to when we
jump out.

>
> Drop last_tb when tb_gen_code() has flushed.
>
> User-only builds hit this easily since cross-page goto_tb chaining,
> e.g. qemu-sparc64 -tb-size 1 on a program writing next to its PLT.
>
> Fixes: ebf7a5d294a3 ("accel/tcg: Improve buffer overflow in tb_gen_code")
> Fixes: 25e5c6a320db ("accel/tcg: Allow cross-page goto_tb chaining in user-only builds")
> Signed-off-by: Stian Halseth <stian@itx.no>
> ---
> 25e5c6a320db is not in a release yet (v11.1.x does not have it), so the
> crash is new in the 11.2 cycle.
>
> Reproducers available on request: on x86_64 the stale chain happens
> too but has stayed harmless; on sparc64 it crashes every time.
>
>  accel/tcg/cpu-exec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 5b4bca5084..600e8c5fa8 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -968,11 +968,17 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc)
>              if (tb == NULL) {
>                  CPUJumpCache *jc;
>                  uint32_t h;
> +                unsigned flush_count = qatomic_read(&tb_ctx.tb_flush_count);
>  
>                  mmap_lock();
>                  tb = tb_gen_code(cpu, s);
>                  mmap_unlock();
>  
> +                /* tb_gen_code may have flushed, freeing last_tb. */
> +                if (flush_count != qatomic_read(&tb_ctx.tb_flush_count)) {
> +                    last_tb = NULL;
> +                }
> +
>                  /*
>                   * We add the TB in the virtual pc hash table
>                   * for the fast lookup

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro