mbox series

[v4,0/9] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread

Message ID 20200831150124.206267-1-mlevitsk@redhat.com
Headers show
Series Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand

Message

Maxim Levitsky Aug. 31, 2020, 3:01 p.m. UTC 
Hi!

This is a patch series that is a result of my discussion with Paulo on
how to correctly fix the root cause of the BZ #1812399.

The root cause of this bug is the fact that IO thread is running mostly
unlocked versus main thread on which device hotplug is done.

qdev_device_add first creates the device object, then places it on the bus,
and only then realizes it.

However some drivers and currently only virtio-scsi enumerate its child bus
devices on each request that is received from the guest and that can happen on the IO
thread.

Thus we have a window when new device is on the bus but not realized and can be accessed
by the virtio-scsi driver in that state.

Fix that by doing two things:

1. Add partial RCU protection to the list of a bus's child devices.
This allows the scsi IO thread to safely enumerate the child devices
while it races with the hotplug placing the device on the bus.

2. Let scsi_device_find not return devices that are on the bus but not realized

Note that in the particular bug report the issue wasn't a race but rather due
to combination of things, the .realize code in the middle managed to trigger IO on the virtqueue
which caused the virtio-scsi driver to access the half realized device. However
since this can happen as well with real IO thread, this patch series was done,
which fixes this as well.

Changes from V3:

* Rebased to latest qemu

* Added a new patch to fix related race in scsi_target_emulate_report_luns

* Moved the non-realized device check to scsi core, since there is no
  way a device driver will want to see non realized devices on a scsi bus.
  (scsi-bus still needs this and can using an internal function)

* Splitted patch that added drain_rcu and used it, to patch that only adds it, and one
  that uses it (no other changes so I kept Reviewed-by)

*Some tweaks to commits

This series was tested by adding a virtio-scsi drive with iothread,
then running fio stress job in the guest in a loop, and then adding/removing
the scsi drive on the host in the loop.
This test was failing usually on 1st iteration withouth this patch series,
and now it seems to work smoothly.

Best regards,
	Maxim Levitsky

Maxim Levitsky (9):
  scsi/scsi_bus: switch search direction in scsi_device_find
  rcu: Implement drain_call_rcu
  device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add
  device-core: use RCU for list of childs of a bus
  device-core: use atomic_set on .realized property
  scsi/scsi-bus: scsi_device_find: don't return unrealized devices
  scsi/scsi_bus: Add scsi_device_get
  virtio-scsi: use scsi_device_get
  scsi/scsi_bus: fix races in REPORT LUNS

 hw/core/bus.c          |  28 +++++---
 hw/core/qdev.c         |  56 +++++++++++----
 hw/scsi/scsi-bus.c     | 151 ++++++++++++++++++++++++++---------------
 hw/scsi/virtio-scsi.c  |  27 +++++---
 include/hw/qdev-core.h |  11 +++
 include/hw/scsi/scsi.h |   1 +
 include/qemu/rcu.h     |   1 +
 qdev-monitor.c         |  22 ++++++
 util/rcu.c             |  55 +++++++++++++++
 9 files changed, 265 insertions(+), 87 deletions(-)

-- 
2.26.2

Comments

Stefan Hajnoczi Sept. 8, 2020, 3 p.m. UTC | #1 
On Mon, Aug 31, 2020 at 06:01:21PM +0300, Maxim Levitsky wrote:
> The device core first places a device on the bus and then realizes it.
> Make scsi_device_find avoid returing such devices to avoid
> races in drivers that use an iothread (currently virtio-scsi)
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1812399
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/scsi/scsi-bus.c | 88 ++++++++++++++++++++++++++++------------------
>  1 file changed, 53 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 92d412b65c..7ceae2c92b 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -51,6 +51,56 @@ static const TypeInfo scsi_bus_info = {
>  };
>  static int next_scsi_bus;
>  
> +static SCSIDevice *_scsi_device_find(SCSIBus *bus, int channel, int id, int lun,
> +                                     bool include_unrealized)

Declaring an identifier with a leading underscore with file scope is
undefined behavior according to the C99 standard (7.1.3 Reserved
identifiers). QEMU code usually avoids doing this by calling the
function do_scsi_device_find() or similar.

I'm not aware of any practical problem though, so don't worry about
changing it unless you respin the series:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Maxim Levitsky Sept. 9, 2020, 8:15 a.m. UTC | #2 
On Tue, 2020-09-08 at 16:00 +0100, Stefan Hajnoczi wrote:
> On Mon, Aug 31, 2020 at 06:01:21PM +0300, Maxim Levitsky wrote:
> > The device core first places a device on the bus and then realizes it.
> > Make scsi_device_find avoid returing such devices to avoid
> > races in drivers that use an iothread (currently virtio-scsi)
> > 
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1812399
> > 
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  hw/scsi/scsi-bus.c | 88 ++++++++++++++++++++++++++++------------------
> >  1 file changed, 53 insertions(+), 35 deletions(-)
> > 
> > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> > index 92d412b65c..7ceae2c92b 100644
> > --- a/hw/scsi/scsi-bus.c
> > +++ b/hw/scsi/scsi-bus.c
> > @@ -51,6 +51,56 @@ static const TypeInfo scsi_bus_info = {
> >  };
> >  static int next_scsi_bus;
> >  
> > +static SCSIDevice *_scsi_device_find(SCSIBus *bus, int channel, int id, int lun,
> > +                                     bool include_unrealized)
> 
> Declaring an identifier with a leading underscore with file scope is
> undefined behavior according to the C99 standard (7.1.3 Reserved
> identifiers). QEMU code usually avoids doing this by calling the
> function do_scsi_device_find() or similar.

I'll fix that, thanks!

Best regards,
	Maxim Levitsky
> 
> I'm not aware of any practical problem though, so don't worry about
> changing it unless you respin the series:
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Stefan Hajnoczi Sept. 11, 2020, 3:12 p.m. UTC | #3 
On Wed, Sep 09, 2020 at 11:20:24AM +0300, Maxim Levitsky wrote:
> On Tue, 2020-09-08 at 16:27 +0100, Stefan Hajnoczi wrote:
> > On Mon, Aug 31, 2020 at 06:01:24PM +0300, Maxim Levitsky wrote:
> > > @@ -460,46 +466,36 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
> > >      }
> > >      channel = r->req.dev->channel;
> > >      id = r->req.dev->id;
> > > -    found_lun0 = false;
> > > -    n = 0;
> > >  
> > > -    rcu_read_lock();
> > >  
> > > -    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> > > -        DeviceState *qdev = kid->child;
> > > -        SCSIDevice *dev = SCSI_DEVICE(qdev);
> > > +    /* add size (will be updated later to correct value */
> > > +    g_byte_array_append(buf, tmp, 8);
> > > +    len += 8;
> > 
> > Can g_byte_array_size() be used instead of keeping a len local variable?
> Glib don't seem to have this function, I checked the docs.
> Its seems that they want to convert it to GBytes which is basically immutible verion
> of GByteArray and it does have g_bytes_get_size.
> I decided that a local variable while ugly is still better that this.
> 
> 
> I haven't wrote much code that uses Glib, so I might have missed something though.
> I had read this reference:
> https://developer.gnome.org/glib/stable/glib-Byte-Arrays.html

Oops, you're right. GByteArray != GBytes. The local variable makes sense.

Stefan