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

[Michael S. Tsirkin]

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.

-- 
MST