| Message ID | 20231013153302.39234-2-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Fix locking in media change monitor commands | expand |
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>
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
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; }
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(+)