Re: [PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug

[Kevin Wolf <kwolf@redhat.com>]

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

Am 02.05.2023 um 22:02 hat Stefan Hajnoczi geschrieben:
> On Tue, May 02, 2023 at 03:19:52PM +0200, Kevin Wolf wrote:
> > Am 01.05.2023 um 17:09 hat Stefan Hajnoczi geschrieben:
> > > On Fri, Apr 28, 2023 at 04:22:55PM +0200, Kevin Wolf wrote:
> > > > Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> > > > > This patch is part of an effort to remove the aio_disable_external()
> > > > > API because it does not fit in a multi-queue block layer world where
> > > > > many AioContexts may be submitting requests to the same disk.
> > > > > 
> > > > > The SCSI emulation code is already in good shape to stop using
> > > > > aio_disable_external(). It was only used by commit 9c5aad84da1c
> > > > > ("virtio-scsi: fixed virtio_scsi_ctx_check failed when detaching scsi
> > > > > disk") to ensure that virtio_scsi_hotunplug() works while the guest
> > > > > driver is submitting I/O.
> > > > > 
> > > > > Ensure virtio_scsi_hotunplug() is safe as follows:
> > > > > 
> > > > > 1. qdev_simple_device_unplug_cb() -> qdev_unrealize() ->
> > > > >    device_set_realized() calls qatomic_set(&dev->realized, false) so
> > > > >    that future scsi_device_get() calls return NULL because they exclude
> > > > >    SCSIDevices with realized=false.
> > > > > 
> > > > >    That means virtio-scsi will reject new I/O requests to this
> > > > >    SCSIDevice with VIRTIO_SCSI_S_BAD_TARGET even while
> > > > >    virtio_scsi_hotunplug() is still executing. We are protected against
> > > > >    new requests!
> > > > > 
> > > > > 2. Add a call to scsi_device_purge_requests() from scsi_unrealize() so
> > > > >    that in-flight requests are cancelled synchronously. This ensures
> > > > >    that no in-flight requests remain once qdev_simple_device_unplug_cb()
> > > > >    returns.
> > > > > 
> > > > > Thanks to these two conditions we don't need aio_disable_external()
> > > > > anymore.
> > > > > 
> > > > > Cc: Zhengui Li <lizhengui@huawei.com>
> > > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > > Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > 
> > > > qemu-iotests 040 starts failing for me after this patch, with what looks
> > > > like a use-after-free error of some kind.
> > > > 
> > > > (gdb) bt
> > > > #0  0x000055b6e3e1f31c in job_type (job=0xe3e3e3e3e3e3e3e3) at ../job.c:238
> > > > #1  0x000055b6e3e1cee5 in is_block_job (job=0xe3e3e3e3e3e3e3e3) at ../blockjob.c:41
> > > > #2  0x000055b6e3e1ce7d in block_job_next_locked (bjob=0x55b6e72b7570) at ../blockjob.c:54
> > > > #3  0x000055b6e3df6370 in blockdev_mark_auto_del (blk=0x55b6e74af0a0) at ../blockdev.c:157
> > > > #4  0x000055b6e393e23b in scsi_qdev_unrealize (qdev=0x55b6e7c04d40) at ../hw/scsi/scsi-bus.c:303
> > > > #5  0x000055b6e3db0d0e in device_set_realized (obj=0x55b6e7c04d40, value=false, errp=0x55b6e497c918 <error_abort>) at ../hw/core/qdev.c:599
> > > > #6  0x000055b6e3dba36e in property_set_bool (obj=0x55b6e7c04d40, v=0x55b6e7d7f290, name=0x55b6e41bd6d8 "realized", opaque=0x55b6e7246d20, errp=0x55b6e497c918 <error_abort>)
> > > >     at ../qom/object.c:2285
> > > > #7  0x000055b6e3db7e65 in object_property_set (obj=0x55b6e7c04d40, name=0x55b6e41bd6d8 "realized", v=0x55b6e7d7f290, errp=0x55b6e497c918 <error_abort>) at ../qom/object.c:1420
> > > > #8  0x000055b6e3dbd84a in object_property_set_qobject (obj=0x55b6e7c04d40, name=0x55b6e41bd6d8 "realized", value=0x55b6e74c1890, errp=0x55b6e497c918 <error_abort>)
> > > >     at ../qom/qom-qobject.c:28
> > > > #9  0x000055b6e3db8570 in object_property_set_bool (obj=0x55b6e7c04d40, name=0x55b6e41bd6d8 "realized", value=false, errp=0x55b6e497c918 <error_abort>) at ../qom/object.c:1489
> > > > #10 0x000055b6e3daf2b5 in qdev_unrealize (dev=0x55b6e7c04d40) at ../hw/core/qdev.c:306
> > > > #11 0x000055b6e3db509d in qdev_simple_device_unplug_cb (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at ../hw/core/qdev-hotplug.c:72
> > > > #12 0x000055b6e3c520f9 in virtio_scsi_hotunplug (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at ../hw/scsi/virtio-scsi.c:1065
> > > > #13 0x000055b6e3db4dec in hotplug_handler_unplug (plug_handler=0x55b6e81c3630, plugged_dev=0x55b6e7c04d40, errp=0x7ffec5519200) at ../hw/core/hotplug.c:56
> > > > #14 0x000055b6e3a28f84 in qdev_unplug (dev=0x55b6e7c04d40, errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:935
> > > > #15 0x000055b6e3a290fa in qmp_device_del (id=0x55b6e74c1760 "scsi0", errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:955
> > > > #16 0x000055b6e3fb0a5f in qmp_marshal_device_del (args=0x7f61cc005eb0, ret=0x7f61d5a8ae38, errp=0x7f61d5a8ae40) at qapi/qapi-commands-qdev.c:114
> > > > #17 0x000055b6e3fd52e1 in do_qmp_dispatch_bh (opaque=0x7f61d5a8ae08) at ../qapi/qmp-dispatch.c:128
> > > > #18 0x000055b6e4007b9e in aio_bh_call (bh=0x55b6e7dea730) at ../util/async.c:155
> > > > #19 0x000055b6e4007d2e in aio_bh_poll (ctx=0x55b6e72447c0) at ../util/async.c:184
> > > > #20 0x000055b6e3fe3b45 in aio_dispatch (ctx=0x55b6e72447c0) at ../util/aio-posix.c:421
> > > > #21 0x000055b6e4009544 in aio_ctx_dispatch (source=0x55b6e72447c0, callback=0x0, user_data=0x0) at ../util/async.c:326
> > > > #22 0x00007f61ddc14c7f in g_main_dispatch (context=0x55b6e7244b20) at ../glib/gmain.c:3454
> > > > #23 g_main_context_dispatch (context=0x55b6e7244b20) at ../glib/gmain.c:4172
> > > > #24 0x000055b6e400a7e8 in glib_pollfds_poll () at ../util/main-loop.c:290
> > > > #25 0x000055b6e400a0c2 in os_host_main_loop_wait (timeout=0) at ../util/main-loop.c:313
> > > > #26 0x000055b6e4009fa2 in main_loop_wait (nonblocking=0) at ../util/main-loop.c:592
> > > > #27 0x000055b6e3a3047b in qemu_main_loop () at ../softmmu/runstate.c:731
> > > > #28 0x000055b6e3dab27d in qemu_default_main () at ../softmmu/main.c:37
> > > > #29 0x000055b6e3dab2b8 in main (argc=24, argv=0x7ffec55196a8) at ../softmmu/main.c:48
> > > > (gdb) p jobs
> > > > $4 = {lh_first = 0x0}
> > > 
> > > I wasn't able to reproduce this with gcc 13.1.1 or clang 16.0.1:
> > > 
> > >   $ tests/qemu-iotests/check -qcow2 040
> > > 
> > > Any suggestions on how to reproduce the issue?
> > 
> > It happens consistently for me with the same command line, both with gcc
> > and clang.
> > 
> > gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)
> > clang version 15.0.7 (Fedora 15.0.7-2.fc37)
> > 
> > Maybe there is a semantic merge conflict? I have applied the series on
> > top of master (05d50ba2d4) and my block branch (88f81f7bc8).
> 
> I can't find 88f81f7bc8 but rebased on repo.or.cz/qemu/kevin.git block
> (4514dac7f2e9) and the test passes here.
> 
> I rebased on qemu.git/master (05d50ba2d4) and it also passes.
> 
> Please let me know if the following tree (a0ff680a72f6) works on your
> machine:
> https://gitlab.com/stefanha/qemu/-/tree/remove-aio_disable_external

Fails in the same way.

So I tried to debug this myself now. The problem is that iterating the
jobs in blockdev_mark_auto_del() is incorrect: job_cancel_locked()
frees the job and then block_job_next_locked() is a use after free.

It also drops job_mutex temporarily and polls, so even switching to a
*_FOREACH_SAFE style loop won't fix this. I guess we have to restart
the whole search from the start after a job_cancel_locked() because the
list might look very different after the call.

Now, of course, how this is related to your patch and why it doesn't
trigger before it, is still less than clear. What I found out is that
adding the scsi_device_purge_requests() is enough to crash it. Maybe
it's related to the blk_drain() inside of it. That the job finishes
earlier during the unplug now or something like that.

Anyway, changing blockdev_mark_auto_del() fixes it. I'll send a patch.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]