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