On Wed, Dec 14, 2022 at 08:03:38AM -0800, Chuang Xu wrote:
> On 2022/12/13 下午9:35, Chuang Xu wrote:
>
> Before using any flatview, sanity check we're not during a memory
> region transaction or the map can be invalid.
>
> Signed-off-by: Chuang Xu <xuchuangxc...@bytedance.com>
> <xuchuangxc...@bytedance.com>
> ---
> include/exec/memory.h | 9 +++++++++
> softmmu/memory.c | 1 -
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 91f8a2395a..b43cd46084 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1069,8 +1069,17 @@ struct FlatView {
> MemoryRegion *root;
> };
>
> +static unsigned memory_region_transaction_depth;
> +
> static inline FlatView *address_space_to_flatview(AddressSpace *as)
> {
> + /*
> + * Before using any flatview, sanity check we're not during a memory
> + * region transaction or the map can be invalid. Note that this can
> + * also be called during commit phase of memory transaction, but that
> + * should also only happen when the depth decreases to 0 first.
> + */
> + assert(memory_region_transaction_depth == 0);
> return qatomic_rcu_read(&as->current_map);
> }
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bc0be3f62c..f177c40cd8 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -37,7 +37,6 @@
>
> //#define DEBUG_UNASSIGNED
>
> -static unsigned memory_region_transaction_depth;
> static bool memory_region_update_pending;
> static bool ioeventfd_update_pending;
> unsigned int global_dirty_tracking;
>
> Here are some new situations to be synchronized.
>
> I found that there is a probability to trigger assert in the QEMU startup
> phase.
>
> Here is the coredump backtrace:
>
> #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> #1 0x00007f7825e33535 in __GI_abort () at abort.c:79
> #2 0x00007f7825e3340f in __assert_fail_base (fmt=0x7f7825f94ef0
> "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5653c643add8
> "memory_region_transaction_depth == 0",
> file=0x5653c63dad78
> "/data00/migration/qemu-open/include/exec/memory.h", line=1082,
> function=<optimized out>) at assert.c:92
> #3 0x00007f7825e411a2 in __GI___assert_fail
> (assertion=assertion@entry=0x5653c643add8
> "memory_region_transaction_depth == 0",
> file=file@entry=0x5653c63dad78
> "/data00/migration/qemu-open/include/exec/memory.h",
> line=line@entry=1082,
> function=function@entry=0x5653c643bd00 <__PRETTY_FUNCTION__.18101>
> "address_space_to_flatview") at assert.c:101
> #4 0x00005653c60f0383 in address_space_to_flatview (as=0x5653c6af2340
> <address_space_memory>) at
> /data00/migration/qemu-open/include/exec/memory.h:1082
> #5 address_space_to_flatview (as=0x5653c6af2340
> <address_space_memory>) at
> /data00/migration/qemu-open/include/exec/memory.h:1074
> #6 address_space_get_flatview (as=0x5653c6af2340
> <address_space_memory>) at ../softmmu/memory.c:809
> #7 0x00005653c60fef04 in address_space_cache_init
> (cache=cache@entry=0x7f781fff8420, as=<optimized out>,
> addr=63310635776, len=48, is_write=is_write@entry=false)
> at ../softmmu/physmem.c:3352
> #8 0x00005653c60c08c5 in virtqueue_split_pop (vq=0x7f781c576270,
> sz=264) at ../hw/virtio/virtio.c:1959
> #9 0x00005653c60c0b7d in virtqueue_pop (vq=vq@entry=0x7f781c576270,
> sz=<optimized out>) at ../hw/virtio/virtio.c:2177
> #10 0x00005653c609f14f in virtio_scsi_pop_req
> (s=s@entry=0x5653c9034300, vq=vq@entry=0x7f781c576270) at
> ../hw/scsi/virtio-scsi.c:219
> #11 0x00005653c60a04a3 in virtio_scsi_handle_cmd_vq
> (vq=0x7f781c576270, s=0x5653c9034300) at ../hw/scsi/virtio-scsi.c:735
> #12 virtio_scsi_handle_cmd (vdev=0x5653c9034300, vq=0x7f781c576270) at
> ../hw/scsi/virtio-scsi.c:776
> #13 0x00005653c60ba72f in virtio_queue_notify_vq (vq=0x7f781c576270)
> at ../hw/virtio/virtio.c:2847
> #14 0x00005653c62d9706 in aio_dispatch_handler
> (ctx=ctx@entry=0x5653c84909e0, node=0x7f68e4007840) at
> ../util/aio-posix.c:369
> #15 0x00005653c62da254 in aio_dispatch_ready_handlers
> (ready_list=0x7f781fffe6a8, ctx=0x5653c84909e0) at
> ../util/aio-posix.c:399
> #16 aio_poll (ctx=0x5653c84909e0, blocking=blocking@entry=true) at
> ../util/aio-posix.c:713
> #17 0x00005653c61b2296 in iothread_run
> (opaque=opaque@entry=0x5653c822c390) at ../iothread.c:67
> #18 0x00005653c62dcd8a in qemu_thread_start (args=<optimized out>) at
> ../util/qemu-thread-posix.c:505
> #19 0x00007f7825fd8fa3 in start_thread (arg=<optimized out>) at
> pthread_create.c:486
> #20 0x00007f7825f0a06f in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
This does look like a bug to me.
Paolo/Michael?
>
> I find that some functions doesn't take bql before calling
> address_space_to_flatview(), as shown in the backtrace. I
> also find other similar situations in the code. I find that
> prepare_mmio_access() will take bql to provide some protection,
> but I don't think it's sufficient.I think that if we want to
> ensure that the reading and writing of mr and the modification
> of mr don't occur at the same time, maybe we need to take bql
> in address_space_to_flatview() like this:
>
>
> static FlatView *address_space_to_flatview(AddressSpace *as)
> {
> bool release_lock = false;
> FlatView *ret;
>
> if (!qemu_mutex_iothread_locked()) {
> qemu_mutex_lock_iothread();
> release_lock = true;
> }
>
> /*
> * Before using any flatview, sanity check we're not during a memory
> * region transaction or the map can be invalid. Note that this can
> * also be called during commit phase of memory transaction, but that
> * should also only happen when the depth decreases to 0 first.
> */
> assert(memory_region_transaction_depth == 0);
> ret = qatomic_rcu_read(&as->current_map);
>
> if (release_lock) {
> qemu_mutex_unlock_iothread();
> }
>
> return ret;
> }
--
Peter Xu
