Re: [PATCH] used ring: define the meaning and requirements of the len field.

[Rusty Russell <rusty@au1.ibm.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
>> We said what it was for, and noted why.  We didn't place any requirements
>> on it, nor clearly spell out the implications of its use.
>> 
>> This clarification comes particularly from noticing that QEMU didn't
>> set len correctly, and philosophising over the correct value when
>> an error has occurred.
>> 
>> (Wording precision feedback from Michael and Cornelia - Thanks!)
>> 
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Regarding 1.0 versus 1.1: do you think this is a non-material change?
> If we do material changes, we need a public review period,
> and I feel reviewer's time is better spent reviewing 1.1.

It's borderline.  These requirements are implied by the text but not
spelled out.  Clearly it is better to do so, but I don't think it's
worth a full review period.

>> diff --git a/content.tex b/content.tex
>> index 6ba079d..2ff8c65 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -600,10 +600,19 @@ them: it is only written to by the device, and read by the driver.
>>  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.
>> +of bytes written into the buffer. 
>> +
>> +\begin{note}
>> +\field{len} is useful
>
> I would add "in particular" here. It's also useful for many other drivers.

OK, I said "particularly useful".

>> +for drivers using untrusted buffers: if a driver does not know exactly
>> +how much has been written by the device, the driver would have to zero
>> +the buffer in advance to ensure no data leakage occurs.
>> +
>> +For example, a network driver may hand a received buffer directly to
>> +an unprivileged userspace application.  If the network device has not
>> +overwritten the bytes which were in that buffer, this could leak the
>> +contents of freed memory from other processes to the application.
>> +\end{note}
>>  
>>  \begin{note}
>>  The legacy \hyperref[intro:Virtio PCI Draft]{[Virtio PCI Draft]}
>> @@ -612,6 +621,28 @@ the constant as VRING_USED_F_NO_NOTIFY, but the layout and value were
>>  identical.
>>  \end{note}
>>  
>> +\devicenormative{\subsubsection}{Virtqueue Notification Suppression}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
>
> Notification Suppression? 

Hmm, cut & paste bug.

>> +The device MUST set \field{len} prior to updating the used \field{idx}.
>> +
>> +The device MUST write at least \field{len} bytes to descriptor,
>> +beginning at the first device-writable buffer,
>> +prior to updating the used \field{idx}.
>> +
>> +The device MAY write more than \field{len} bytes to descriptor.
>> +
>> +\begin{note}
>> +There are potential error cases where a device might not know what
>> +parts of the buffers have been written.  This is why \field{len} is
>> +permitted to be an underestimate: that's preferable to the driver believing
>> +that uninitialized memory has been overwritten when it has not.
>> +\end{note}
>> +
>> +\drivernormative{\subsubsection}{Virtqueue Notification Suppression}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
>
>
> Same here.
>
>> +
>> +The driver MUST NOT make assumptions about data in device-writable buffers
>> +beyond the first \field{len} bytes, and SHOULD ignore this data.
>> +
>>  \subsection{Virtqueue Notification Suppression}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
>>  
>>  The device can suppress notifications in a manner analogous to the way

Inter-diff:

diff --git a/content.tex b/content.tex
index 2ff8c65..8e8765d 100644
--- a/content.tex
+++ b/content.tex
@@ -603,7 +603,7 @@ placed in the available ring by the guest earlier), and \field{len} the total
 of bytes written into the buffer. 
 
 \begin{note}
-\field{len} is useful
+\field{len} is particularly useful
 for drivers using untrusted buffers: if a driver does not know exactly
 how much has been written by the device, the driver would have to zero
 the buffer in advance to ensure no data leakage occurs.
@@ -621,7 +621,7 @@ the constant as VRING_USED_F_NO_NOTIFY, but the layout and value were
 identical.
 \end{note}
 
-\devicenormative{\subsubsection}{Virtqueue Notification Suppression}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
+\devicenormative{\subsubsection}{The Virtqueue Used Ring}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
 
 The device MUST set \field{len} prior to updating the used \field{idx}.
 
@@ -638,7 +638,7 @@ permitted to be an underestimate: that's preferable to the driver believing
 that uninitialized memory has been overwritten when it has not.
 \end{note}
 
-\drivernormative{\subsubsection}{Virtqueue Notification Suppression}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
+\drivernormative{\subsubsection}{The Virtqueue Used Ring}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
 
 The driver MUST NOT make assumptions about data in device-writable buffers
 beyond the first \field{len} bytes, and SHOULD ignore this data.