[PATCH 0/2] block: Fix locking in media change monitor commands

[PATCH 0/2] block: Fix locking in media change monitor commands

[Kevin Wolf]

Kevin Wolf (2):
  block: Fix locking in media change monitor commands
  iotests: Test media change with iothreads

 block/qapi-sysemu.c    | 5 +++++
 tests/qemu-iotests/118 | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.41.0

[PATCH 1/2] block: Fix locking in media change monitor commands

[Kevin Wolf]

blk_insert_bs() requires that the caller holds the AioContext lock for
the node to be inserted. Since commit c066e808e11, neglecting to do so
causes a crash when the child has to be moved to a different AioContext
to attach it to the BlockBackend.

This fixes qmp_blockdev_insert_anon_medium(), which is called for the
QMP commands 'blockdev-insert-medium' and 'blockdev-change-medium', to
correctly take the lock.

Cc: qemu-stable@nongnu.org
Fixes: https://issues.redhat.com/browse/RHEL-3922
Fixes: c066e808e11a5c181b625537b6c78e0de27a4801
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi-sysemu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/qapi-sysemu.c b/block/qapi-sysemu.c
index 3f614cbc04..1618cd225a 100644
--- a/block/qapi-sysemu.c
+++ b/block/qapi-sysemu.c
@@ -237,6 +237,7 @@ static void qmp_blockdev_insert_anon_medium(BlockBackend *blk,
                                             BlockDriverState *bs, Error **errp)
 {
     Error *local_err = NULL;
+    AioContext *ctx;
     bool has_device;
     int ret;
 
@@ -258,7 +259,11 @@ static void qmp_blockdev_insert_anon_medium(BlockBackend *blk,
         return;
     }
 
+    ctx = bdrv_get_aio_context(bs);
+    aio_context_acquire(ctx);
     ret = blk_insert_bs(blk, bs, errp);
+    aio_context_release(ctx);
+
     if (ret < 0) {
         return;
     }
-- 
2.41.0

[PATCH 2/2] iotests: Test media change with iothreads

[Kevin Wolf]

iotests case 118 already tests all relevant operations for media change
with multiple devices, however never with iothreads. This changes the
test so that the virtio-scsi tests run with an iothread.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/118 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index cae52ffa5e..bc7533bb54 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -295,7 +295,8 @@ class TestInitiallyFilled(GeneralChangeTestsBaseClass):
                                    'file.driver=file',
                                    'file.filename=%s' % old_img ])
         if self.interface == 'scsi':
-            self.vm.add_device('virtio-scsi-pci')
+            self.vm.add_object('iothread,id=iothread0')
+            self.vm.add_device('virtio-scsi-pci,iothread=iothread0')
         self.vm.add_device('%s,drive=drive0,id=%s' %
                            (interface_to_device_name(self.interface),
                             self.device_name))
@@ -332,7 +333,8 @@ class TestInitiallyEmpty(GeneralChangeTestsBaseClass):
         if self.use_drive:
             self.vm.add_drive(None, 'media=%s' % self.media, 'none')
         if self.interface == 'scsi':
-            self.vm.add_device('virtio-scsi-pci')
+            self.vm.add_object('iothread,id=iothread0')
+            self.vm.add_device('virtio-scsi-pci,iothread=iothread0')
         self.vm.add_device('%s,%sid=%s' %
                            (interface_to_device_name(self.interface),
                             'drive=drive0,' if self.use_drive else '',
-- 
2.41.0

Re: [PATCH 1/2] block: Fix locking in media change monitor commands

[Hanna Czenczek]

On 13.10.23 17:33, Kevin Wolf wrote:
> blk_insert_bs() requires that the caller holds the AioContext lock for
> the node to be inserted. Since commit c066e808e11, neglecting to do so
> causes a crash when the child has to be moved to a different AioContext
> to attach it to the BlockBackend.
>
> This fixes qmp_blockdev_insert_anon_medium(), which is called for the
> QMP commands 'blockdev-insert-medium' and 'blockdev-change-medium', to
> correctly take the lock.
>
> Cc: qemu-stable@nongnu.org
> Fixes: https://issues.redhat.com/browse/RHEL-3922
> Fixes: c066e808e11a5c181b625537b6c78e0de27a4801
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qapi-sysemu.c | 5 +++++
>   1 file changed, 5 insertions(+)

Do we need to take the lock for the dev_ops tray callbacks, too?  I 
suppose not, and it also wouldn’t really matter in light of the lock 
being supposed to go away anyway, but still thought I should ask.

In any case, this change here is necessary, so:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 2/2] iotests: Test media change with iothreads

[Hanna Czenczek]

On 13.10.23 17:33, Kevin Wolf wrote:
> iotests case 118 already tests all relevant operations for media change
> with multiple devices, however never with iothreads. This changes the
> test so that the virtio-scsi tests run with an iothread.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/118 | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 1/2] block: Fix locking in media change monitor commands

[Kevin Wolf]

Am 31.10.2023 um 12:54 hat Hanna Czenczek geschrieben:
> On 13.10.23 17:33, Kevin Wolf wrote:
> > blk_insert_bs() requires that the caller holds the AioContext lock for
> > the node to be inserted. Since commit c066e808e11, neglecting to do so
> > causes a crash when the child has to be moved to a different AioContext
> > to attach it to the BlockBackend.
> > 
> > This fixes qmp_blockdev_insert_anon_medium(), which is called for the
> > QMP commands 'blockdev-insert-medium' and 'blockdev-change-medium', to
> > correctly take the lock.
> > 
> > Cc: qemu-stable@nongnu.org
> > Fixes: https://issues.redhat.com/browse/RHEL-3922
> > Fixes: c066e808e11a5c181b625537b6c78e0de27a4801
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/qapi-sysemu.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> 
> Do we need to take the lock for the dev_ops tray callbacks, too?  I suppose
> not, and it also wouldn’t really matter in light of the lock being supposed
> to go away anyway, but still thought I should ask.

Seems nobody ever bothered to define what the callbacks expects, and I
don't know either. Not taking the lock can obviously be a problem, but
taking it can also be a problem if the callback then locks a second time
and calls a synchronous function that polls.

What I do see is that callers disagree about this, so no matter what the
correct answer is, I'm almost sure there is a bug hiding somewhere.

Kevin