Re: [PATCH 0/4] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread

[Stefan Hajnoczi]

On Thu, Apr 16, 2020 at 11:36:20PM +0300, Maxim Levitsky wrote:
> 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. Make the virtio-scsi driver check .realized property of the scsi device
> and avoid touching the device if it isn't
> 
> I don't think that this is very pretty way to solve this, we discussed this
> with Paulo and it kind of looks like the lesser evil. I am open to your 
> thoughts about this.
> 
> Note that this patch series doesn't pass some unit tests and in particular 
> qtest 'drive_del-test'
> I did some light debug of this test and I see that the reason for this is 
> that now child device deletion
> can be delayed due to RCU. This is also something I would like to discuss in 
> this RFC.
> 
> Note also that I might have some code style errors and bugs in this since I 
> haven't
> tested the code in depth yet, because I am not yet sure that this is the 
> right way
> to fix that bug
> 
> Also 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.

Nice approach! Thanks for solving this at the hw/core/bus.c level so
that other devices can benefit too.

Regarding drive_del, I guess the issue here is that this HMP command's
semantics need to include not synchronize_rcu() but some kind of
drain_call_rcu() operation as well that ensures deletion has completed?

This also suggests that all code that removes bus children needs to be
audited since the actual object unref is now deferred to a later point
in time. There could be other cases besides drive_del that don't yet
handle this.

drain_call_rcu() can be implemented by invoking call_rcu(temp,
drain_call_rcu_cb, rcu) where drain_call_rcu_cb() sets a QemuEvent that
the caller is waiting on. This way the caller can be sure that all
previously queued call_rcu() callbacks have completed. call_rcu_thread()
needs to be tweaked to avoid g_usleep() and instead use a timed wait so
that drain_call_rcu() can immediately wake up the thread.

Stefan

signature.asc
Description: PGP signature