Re: [PATCH v4 03/20] virtio-scsi: avoid race between unplug and transport event

[Stefan Hajnoczi <stefanha@xxxxxxxxxx>]

On Tue, May 02, 2023 at 05:19:46PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 19:26 hat Stefan Hajnoczi geschrieben:
> > Only report a transport reset event to the guest after the SCSIDevice
> > has been unrealized by qdev_simple_device_unplug_cb().
> > 
> > qdev_simple_device_unplug_cb() sets the SCSIDevice's qdev.realized field
> > to false so that scsi_device_find/get() no longer see it.
> > 
> > scsi_target_emulate_report_luns() also needs to be updated to filter out
> > SCSIDevices that are unrealized.
> > 
> > These changes ensure that the guest driver does not see the SCSIDevice
> > that's being unplugged if it responds very quickly to the transport
> > reset event.
> > 
> > Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Reviewed-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > Reviewed-by: Daniil Tatianin <d-tatianin@xxxxxxxxxxxxxx>
> > Signed-off-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> 
> > @@ -1082,6 +1073,15 @@ static void virtio_scsi_hotunplug(HotplugHandler 
> > *hotplug_dev, DeviceState *dev,
> >          blk_set_aio_context(sd->conf.blk, qemu_get_aio_context(), NULL);
> >          virtio_scsi_release(s);
> >      }
> > +
> > +    if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
> > +        virtio_scsi_acquire(s);
> > +        virtio_scsi_push_event(s, sd,
> > +                               VIRTIO_SCSI_T_TRANSPORT_RESET,
> > +                               VIRTIO_SCSI_EVT_RESET_REMOVED);
> > +        scsi_bus_set_ua(&s->bus, SENSE_CODE(REPORTED_LUNS_CHANGED));
> > +        virtio_scsi_release(s);
> > +    }
> >  }
> 
> s, sd and s->bus are all unrealized at this point, whereas before this
> patch they were still realized. I couldn't find any practical problem
> with it, but it made me nervous enough that I thought I should comment
> on it at least.
> 
> Should we maybe have documentation on these functions that says that
> they accept unrealized objects as their parameters?

s is the VirtIOSCSI controller, not the SCSIDevice that is being
unplugged. The VirtIOSCSI controller is still realized.

s->bus is the VirtIOSCSI controller's bus, it is still realized.

You are right that the SCSIDevice (sd) has been unrealized at this
point:
- sd->conf.blk is safe because qdev properties stay alive the
  Object is deleted, but I'm not sure we should rely on that.
- virti_scsi_push_event(.., sd, ...) is questionable because the LUN
  that's fetched from sd no longer belongs to the unplugged SCSIDevice.

How about I change the code to fetch sd->conf.blk and the LUN before
unplugging?

Stefan

Attachment: signature.asc
Description: PGP signature