Re: [PATCH] virtio-scsi: fix race in virtio_scsi_dataplane_start()

[Paolo Bonzini]

On 8/3/22 18:28, Stefan Hajnoczi wrote:
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 8bb6e6acfc..a575c3f0cd 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -66,6 +66,21 @@ static int virtio_scsi_set_host_notifier(VirtIOSCSI *s, 
> VirtQueue *vq, int n)
>       return 0;
>   }
>   
> +/* Context: BH in IOThread */
> +static void virtio_scsi_dataplane_start_bh(void *opaque)
> +{
> +    VirtIOSCSI *s = opaque;
> +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> +    int i;
> +
> +    virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx);
> +    virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx);
> +
> +    for (i = 0; i < vs->conf.num_queues; i++) {
> +        virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx);
> +    }
> +}
> +
>   /* Context: BH in IOThread */
>   static void virtio_scsi_dataplane_stop_bh(void *opaque)
>   {
> @@ -136,16 +151,18 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
>   
>       memory_region_transaction_commit();
>   
> -    aio_context_acquire(s->ctx);
> -    virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx);
> -    virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx);
> -
> -    for (i = 0; i < vs->conf.num_queues; i++) {
> -        virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx);
> -    }
> -
>       s->dataplane_starting = false;
>       s->dataplane_started = true;
> +
> +    /*
> +     * Attach notifiers from within the IOThread. It's possible to attach
> +     * notifiers from our thread directly but this approach has the advantages
> +     * that virtio_scsi_dataplane_start_bh() is symmetric with
> +     * virtio_scsi_dataplane_stop_bh() and the s->dataplane_started assignment
> +     * above doesn't require explicit synchronization.
> +     */
> +    aio_context_acquire(s->ctx);
> +    aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_start_bh, s);
>       aio_context_release(s->ctx);

Taking the AioContext lock for code that is running in another thread 
seems wrong.  But really there is no need to take the lock: I think it 
wanted to protect against the handler running before 
s->dataplane_starting/started were set, but it's not needed now because 
the iothread is busy running the bottom half.

Also, please do the same in virtio-blk as well.

Thanks,

Paolo