Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification

[Cornelia Huck]

On Wed, 1 Mar 2017 19:19:40 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Wed, Mar 01, 2017 at 06:14:52PM +0100, Cornelia Huck wrote:
> > On Wed, 1 Mar 2017 19:04:56 +0200
> > "Michael S. Tsirkin" <address@hidden> wrote:
> > 
> > > On Wed, Mar 01, 2017 at 06:00:44PM +0100, Cornelia Huck wrote:
> > > > On Wed, 1 Mar 2017 18:50:34 +0200
> > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > 
> > > > > Yes all these callbacks complicate code to the point it's barely
> > > > > readable. I really don't see why are we poking at device beforehand at
> > > > > all. IMHO this is closer to the root of the problem. Don't do it.  One
> > > > > way to look at it is to say that we start aio too early. Do it at
> > > > > driver_ok for virtio 1 and on kick for virtio 0 and problems will go
> > > > > away.
> > > > 
> > > > The problem exists for the case where the guest sets up only the first
> > > > queue, but the host has more than one queue. The guest setting up other
> > > > queues later is probably patholotical, but not really forbidden by the
> > > > spec (I think).
> > > 
> > > I think it's forbidden.  spec explains how queues are set up:
> > > 
> > > 1. Reset the device.
> > > 2. Set the ACKNOWLEDGE status bit: the guest OS has notice the device.
> > > 3. Set the DRIVER status bit: the guest OS knows how to drive the device.
> > > 4. Read device feature bits, and write the subset of feature bits 
> > > understood by the OS and driver to the
> > > device. During this step the driver MAY read (but MUST NOT write) the 
> > > device-specific configuration
> > > fields to check that it can support the device before accepting it.
> > > 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature 
> > > bits after this step.
> > > 6. Re-read device status to ensure the FEATURES_OK bit is still set: 
> > > otherwise, the device does not
> > > support our subset of features and the device is unusable.
> > > 7. Perform device-specific setup, including discovery of virtqueues for 
> > > the device, optional per-bus setup,
> > > reading and possibly writing the device’s virtio configuration space, and 
> > > population of virtqueues.
> > > 8. Set the DRIVER_OK status bit. At this point the device is “live”.
> > 
> > It seems I managed to read over this statement in step 7 several
> > times... So "make sure that we only start aio after DRIVER_OK, and
> > forbid any setup for !desc permanently" will take care of virtio-1
> > devices.
> > 
> > > 
> > > And it goes on to mention a bug in legacy drivers:
> > > Legacy driver implementations often used the device before setting the 
> > > DRIVER_OK bit, and sometimes
> > > even before writing the feature bits to the device.
> > 
> > I wonder whether we need to accommodate legacy drivers doing both that
> > _and_ setting up virtqueues between using the device for the first time
> > and setting DRIVER_OK? Just using the logic of "if there were no rings
> > setup when we first try to start aio, ignore that queue until reset"
> > would cover all but those very odd drivers, and I highly doubt anybody
> > cares about such broken setups.
> 
> 
> kick on a vq before this vq is setup would be a broken thing to do.
> 
> They did this though
> 
>       for each ring
>               add buffer
>               kick
>       driver ok
> 
> so it's a per-ring thing.

Hm... If we just check for !desc, we should be fine after the first aio
poll, I think...