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

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

[Stian Halseth]

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.

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
-- 
2.43.0

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

[Alex Bennée]

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

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

[Stian Halseth]

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

Hi Alex,

On Fri, 2026-10-23 at 18:05 +0100, Alex Bennée wrote:
> 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.

Not much, as far as I can measure. I timed the sparc64 reproducer with
-tb-size 1, where the buffer flushes constantly, so the worst case for
the longjmp, with three variants on current master:

- this patch (keep the shortcut, drop last_tb)
- flush in place, then leave through cpu_loop_exit()
- drop the shortcut, back to queue_tb_flush() + cpu_loop_exit()

All three fix the crash and pass test-xpage-chain and tb-link, and the
run times are within noise of each other.

Dropping the shortcut looks like the cleaner fix though. no last_tb
bookkeeping, and the cleanup you mention runs again. Richard, would you
prefer that as v2, or is there a case where the in-place retry matters,
e.g. cpu_exec_step_atomic?

/Stian

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Matt Turner]

On Fri, Oct 23, 2026 at 7:32 AM Stian Halseth <stian@itx.no> wrote:
> 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.
>
> 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")

Thanks for tracking this down.

I don't think the second Fixes tag is right. The stale chain comes
from ebf7a5d294 alone: any last_tb that exited through an unpatched
goto_tb slot gets chained after the flush, same page or not. Your
x86_64 observation fits that. 25e5c6a320 makes it much easier to
hit, which is worth saying in the commit message, but it didn't
introduce the bug. I'd drop that tag.

ebf7a5d294 has been in since v10.2.0, so this should go to stable:

Cc: qemu-stable@nongnu.org

> ---
> 25e5c6a320db is not in a release yet (v11.1.x does not have it), so the
> crash is new in the 11.2 cycle.

The easy sparc64 reproducer is new, but tb_add_jump() writing into
a freed and possibly reused TB is older than that.

[...]

> +                /* tb_gen_code may have flushed, freeing last_tb. */
> +                if (flush_count != qatomic_read(&tb_ctx.tb_flush_count)) {
> +                    last_tb = NULL;
> +                }

The fix itself looks right to me. The parallel case already leaves
the loop through queue_tb_flush() and cpu_loop_exit(), so this is
the only place that needs the check.

With the Fixes tag fixed:

Reviewed-by: Matt Turner <mattst88@gmail.com>

If you have a small reproducer that runs with -tb-size 1, a test
under tests/tcg would be nice to have, but I wouldn't hold the
patch for it.

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

[Stian Halseth]

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

Hi Matt,

On Fri, 2026-10-23 at 20:08 -0400, Matt Turner wrote:
> 
> Thanks for tracking this down.
> 
> I don't think the second Fixes tag is right. The stale chain comes
> from ebf7a5d294 alone: any last_tb that exited through an unpatched
> goto_tb slot gets chained after the flush, same page or not. Your
> x86_64 observation fits that. 25e5c6a320 makes it much easier to
> hit, which is worth saying in the commit message, but it didn't
> introduce the bug. I'd drop that tag.
> 
> ebf7a5d294 has been in since v10.2.0, so this should go to stable:

Agreed, thanks. v2 drops the tag, names 25e5c6a320 in the message
instead and adds Cc: qemu-stable:
https://lore.kernel.org/qemu-devel/20261024070347.1202239-1-stian@itx.no/

/Stian



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Richard Henderson]

On 10/23/26 12:53, Stian Halseth wrote:
> Hi Alex,
> 
> On Fri, 2026-10-23 at 18:05 +0100, Alex Bennée wrote:
>> 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.
> 
> Not much, as far as I can measure. I timed the sparc64 reproducer with
> -tb-size 1, where the buffer flushes constantly, so the worst case for
> the longjmp, with three variants on current master:
> 
> - this patch (keep the shortcut, drop last_tb)
> - flush in place, then leave through cpu_loop_exit()
> - drop the shortcut, back to queue_tb_flush() + cpu_loop_exit()
> 
> All three fix the crash and pass test-xpage-chain and tb-link, and the
> run times are within noise of each other.
> 
> Dropping the shortcut looks like the cleaner fix though. no last_tb
> bookkeeping, and the cleanup you mention runs again. Richard, would you
> prefer that as v2, or is there a case where the in-place retry matters,
> e.g. cpu_exec_step_atomic?
I don't think in-place retry matters anywhere.  I suspect it was a case 
of premature optimization because "obviously" no other thread can 
interfere here.


r~