Re: [Qemu-devel] [PATCH 0/2] virtio len fixes for qemu.

[Rusty Russell]

"Michael S. Tsirkin" <address@hidden> writes:
> On Wed, Mar 11, 2015 at 02:47:47PM +0800, Fam Zheng wrote:
>> On Wed, 03/11 07:19, Michael S. Tsirkin wrote:
>> > On Wed, Mar 11, 2015 at 04:29:30PM +1030, Rusty Russell wrote:
>> > > The virtio 'used' ring describes descriptors which have been used.  It
>> > > also says how many bytes have been written to the ring.  For some cases,
>> > > this value is ignored by Linux guests, thus errors have not been noticed.
>> > > I was working on increasing the checking in Linux when I noticed this
>> > > behaviour.
>> > > 
>> > > The first patch changes the 'len' formal parameter name to 'len_written' 
>> > > to
>> > > make the API clearer, and adds an assert(). The second fixes block 
>> > > writes.
>> > > 
>> > > Cheers,
>> > > Rusty.
>> > > PS.  It's based on MST's virtio-1.0 tree, but should be easily ported.
>> > 
>> > Thanks, this applies to current master without issues.
>> > However, I think  it's best to apply patch 2, then patch 1,
>> > to avoid triggering errors when bisecting.
>> 
>> I'm seeing a make check failure. If this is a false alarm, the test should be
>> fixed too.
>
> Yea, I'm also now thinking we need a spec clarification on this one, and
> some testing with non linux drivers before jumping to changing hosts and
> guests.

The spec is very clear.  The implementation is crap; let's fix it before
1.0.

Quote:

        Each entry in the ring is a pair: \field{id} indicates the head
        entry of the descriptor chain describing the buffer (this
        matches an entry placed in the available ring by the guest
        earlier), and \field{len} the total of bytes written into the
        buffer. The latter is extremely useful for drivers using
        untrusted buffers: if you do not know exactly how much has been
        written by the device, you usually have to zero the buffer to
        ensure no data leakage occurs.

I have a patch for the Linux side, too, which warns once per device
and fixes it up.  I will make the warning conditional on v1.0.

Cheers,
Rusty.