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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3502 bytes --]

On Wed, May 03, 2023 at 10:00:57AM +0200, Kevin Wolf wrote:
> 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@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > 
> > > > @@ -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.

Yes, I'll do that in the next revision.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]