Re: [Qemu-devel] [PATCH v2 for-2.3] virtio-blk: correctly dirty guest memory

[Michael S. Tsirkin]

On Thu, Apr 02, 2015 at 08:53:04PM +0200, Paolo Bonzini wrote:
> 
> 
> On 02/04/2015 20:46, Michael S. Tsirkin wrote:
> > Oh, true in fact.  It might be a good idea to add something like this to
> > the commit log:
> > 
> >     Additionally, virtio spec requires that device writes at least
> >     len bytes to descriptor - so that driver can rely on
> >     bytes 0..len-1 being initialized by device. Specifically, it says
> >     len can be used as an optimization "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".
> > 
> >     We violated this rule in two cases: on write - len should be 0,
> >     request size was mistakenly used
> 
> Should be 1 due to the status byte.
> 
> > - and on read error - we don't
> >     know whether the whole request size was written, so again len
> >     should be set to 0.
> 
> Oh no wait... my patch does not handle the read error case.
> 
> The len argument to virtqueue_push is being overloaded with two meanings:
> 
> 1) a value that is >= the actual count, used to set the dirty bitmap
> 
> 2) a value that should be <= the actual count, used as mentioned in your
> English text above.
> 
> This is a problem for read errors, because the status byte is at the end
> of the input buffers.  So (1) requires that you set len = size+1, while
> (2) requires that you set len = 0.
> 
> My patch only deals with (1), which is a correctness problem for
> migration, as Wen debugged.  It is a 2.3 regression.
> 
> I don't think (2) is fixable without changing the virtqueue API, and it
> is not a regression.
> 
> Paolo

I agree here. If you respin for any reason, documenting that you
fixed (1) might be a good idea.

-- 
MST