Re: [Qemu-devel] race between tb_gen_code() and qemu_ram_free()

[Paolo Bonzini]

On 20/04/2018 19:55, Peter Maydell wrote:
> There seems to be a race between tb_gen_code() and qemu_ram_free(),
> which results in an abort() in Edgar's test case that exercises the
> xilinx-spips mmio-exec functionality.
> 
> Here's what happens:
>  (1) memory_region_invalidate_mmio_ptr() is called, and it deletes
                                                          ^^

This "it" is more precisely memory_region_do_invalidate_mmio_ptr.

>  the temporary ram MemoryRegion. This doesn't immediately result in
>  deletion of the RAMBlock (which is done from the RCU thread later)
>  (2) we try to execute from this physaddr, so tb_gen_code() is called.
>  This calls get_page_addr_code(). Since the RAMBlock hasn't yet
>  really been deleted, get_page_addr_code() finds it, and returns a
>  ram_addr in the about-to-go-away memory...
>  (3) while the CPU thread is busy doing codegen, the RCU thread runs.
>  It does a flatview_destroy() which ends up doing an object_unref on the
>  memory region, whose destructor calls qemu_ram_free(), removing the
>  RAMBlock from the ram list
>  (4) when the CPU thread has done its codegen, it calls tb_link_page,
>  which calls tb_alloc_page, which calls tlb_protect_code(), with the
>  ram_addr that it got in step 2. We end up in tlb_reset_dirty_range_all(),
>  which calls qemu_get_ram_block() on that ram_addr.
>  (5) qemu_get_ram_block() aborts, because it can't find the RAMBlock
>  corresponding to the ram_addr (because we removed it in step 3).
> 
> How should we resolve this race ?

Note that qemu_ram_free() is _also_ RCU-freeing the RAMBlock.  If it is
not found, it means that codegen is not running within
rcu_read_lock()/rcu_read_unlock().  In fact it's not.

So one possibility would be to add it there.  Theoretically the simplest
possibility, but rcu_read_lock() is taken _outside_ tb_lock which may be
a problem.

The alternative is to add a ref/unref pair of the MemoryRegion whenever
get_page_addr_code() uses it.  That would delay the MemoryRegion
destructor after the end of code generation.  A rough sketch would be
that get_page_addr_code() would grow a MemoryRegion** argument and would
use memory_region_from_host() instead of qemu_ram_addr_from_host().
get_page_addr_code() would either call rcu_read_lock(), or the callers
would (see the doc comment for memory_region_from_host(); it always
feels good when you can refer to doc comments!)

Thanks,

Paolo

> (PS: rr is really useful for identifying what's actually happening
> in multi-thread races like this...)
> 
> thanks
> -- PMM
>