Re: [PATCH] target/ppc: Stop vCPU thread before calling parent_unrealize

[Amit Machhiwal <amachhiw@linux.ibm.com>]

On 2026/09/23 05:44 PM, Shivang Upadhyay wrote:
> During CPU hot-unplug (e.g. via dynamic reconfiguration unplug),
> ppc_cpu_unrealize() invoked pcc->parent_unrealize(dev) before calling
> cpu_remove_sync(CPU(cpu)).
> 
> pcc->parent_unrealize() calls cpu_common_unrealize(), which triggers
> accel_cpu_common_unrealize() -> tcg_exec_unrealizefn() -> tlb_destroy().
> This immediately frees the CPU's TLB tables and structures. Because the
> vCPU thread had not yet been stopped and joined via cpu_remove_sync(),
> the vCPU thread was still actively running its event loop and processing
> queued CPU work (such as tcg_commit_cpu / tlb_flush).
> 
> This resulted in a race where the running vCPU thread accessed and freed
> already-destroyed TLB tables concurrently with tlb_destroy(), leading to
> Segfault (due to heap corruption).
> 
> AddressSanitizer build reported a double-free:
> 
> =================================================================
> ==121930==ERROR: AddressSanitizer: attempting double-free on 0x7ef8f3438800 in thread T14:
>     #0 0x7fe8f74e5beb in free.part.0 (/lib64/libasan.so.8+0xe5beb)
>     #1 0x7fe8f6cb8f84 in g_free (/lib64/libglib-2.0.so.0+0x41f84)
>     #2 0x558bf6a391b1 in tlb_mmu_resize_locked accel/tcg/cputlb.c:249
>     #3 0x558bf6a396b5 in tlb_flush_one_mmuidx_locked accel/tcg/cputlb.c:296
>     #4 0x558bf6a39f91 in tlb_flush_by_mmuidx_async_work accel/tcg/cputlb.c:390
>     #5 0x558bf6a3a200 in tlb_flush_by_mmuidx accel/tcg/cputlb.c:417
>     #6 0x558bf6a3a22a in tlb_flush accel/tcg/cputlb.c:422
>     #7 0x558bf73f31ac in tcg_commit_cpu system/physmem.c:3068
>     #8 0x558bf6987c55 in process_queued_cpu_work cpu-common.c:378
>     #9 0x558bf73a9913 in qemu_process_cpu_events_common system/cpus.c:402
>     #10 0x558bf73a9a46 in qemu_process_cpu_events system/cpus.c:421
>     #11 0x558bf6a65974 in mttcg_cpu_thread_fn accel/tcg/tcg-accel-ops-mttcg.c:90
> 
> 0x7ef8f3438800 is located 0 bytes inside of 65536-byte region [0x7ef8f3438800,0x7ef8f3448800)
> freed by thread T9 here:
>     #0 0x7fe8f74e5beb in free.part.0 (/lib64/libasan.so.8+0xe5beb)
>     #1 0x7fe8f6cb8f84 in g_free (/lib64/libglib-2.0.so.0+0x41f84)
>     #2 0x558bf6a39a91 in tlb_destroy accel/tcg/cputlb.c:345
>     #3 0x558bf6a16354 in tcg_exec_unrealizefn accel/tcg/cpu-exec.c:1094
>     #4 0x558bf693d073 in accel_cpu_common_unrealize accel/accel-common.c:117
>     #5 0x558bf6980e37 in cpu_common_unrealize hw/core/cpu-common.c:279
>     #6 0x558bf6980dfa in cpu_common_unrealizefn hw/core/cpu-common.c:267
>     #7 0x558bf763ef65 in ppc_cpu_unrealize target/ppc/cpu_init.c:6965
>     #8 0x558bf7872199 in device_set_realized hw/core/qdev.c:618
>     #14 0x558bf756068f in spapr_unrealize_vcpu hw/ppc/spapr_cpu_core.c:209
> 
> Fix this by moving cpu_remove_sync() before pcc->parent_unrealize(dev)
> in ppc_cpu_unrealize(), ensuring the vCPU thread is stopped, has
> finished processing its events, and is joined before CPU resources
> and accelerator state are destroyed.
> 
> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> ---
>  target/ppc/cpu_init.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 6c626843c9..b711f9c0a8 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6962,10 +6962,10 @@ static void ppc_cpu_unrealize(DeviceState *dev)
>      PowerPCCPU *cpu = POWERPC_CPU(dev);
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>  
> -    pcc->parent_unrealize(dev);
> -
>      cpu_remove_sync(CPU(cpu));
>  
> +    pcc->parent_unrealize(dev);
> +

The fix is correct and the commit message is well written.  The ASan trace makes
the race unambiguous: tlb_destroy() frees fast->table and desc->fulltlb while
the MTTCG thread is still running tlb_mmu_resize_locked() on the same structures
via the queued tcg_commit_cpu work.

Reviewed-by: Amit Machhiwal <amachhiw@linux.ibm.com>

Minor: I think a short comment above cpu_remove_sync() would make the ordering
constraint self-documenting, e.g.:

    /*
     * Stop and join the vCPU thread before tearing down accelerator
     * state (TLB tables etc.) to prevent the running thread from
     * accessing freed resources.
     */
    cpu_remove_sync(CPU(cpu));

Thanks,
Amit