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

[Kevin Wolf <kwolf@xxxxxxxxxx>]

Am 02.05.2023 um 20:56 hat Stefan Hajnoczi geschrieben:
> 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're right, I misread this part.

> 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.

This feels relatively safe (and it's preexisting anyway), reading a
property doesn't do anything unpredictable and we know the pointer is
still valid.

> - virti_scsi_push_event(.., sd, ...) is questionable because the LUN
>   that's fetched from sd no longer belongs to the unplugged SCSIDevice.

This call is what made me nervous.

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

You mean passing sd->id and sd->lun to virtio_scsi_push_event() instead
of sd itself? That would certainly look cleaner and make sure that we
don't later add code to it that does something with sd that would
require it to be realized.

Kevin

Attachment: signature.asc
Description: PGP signature