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

[Michael S. Tsirkin]

On Thu, Apr 02, 2015 at 07:57:22PM +0200, Paolo Bonzini wrote:
> 
> 
> On 02/04/2015 19:54, Michael S. Tsirkin wrote:
> > On Thu, Apr 02, 2015 at 07:50:44PM +0200, Paolo Bonzini wrote:
> >> After qemu_iovec_destroy, the QEMUIOVector's size is zeroed and
> >> the zero size ultimately is used to compute virtqueue_push's len
> >> argument.  Therefore, reads from virtio-blk devices did not
> >> migrate their results correctly.  (Writes were okay).
> >>
> >> Save the size in virtio_blk_handle_request, and use it when the request
> >> is completed.
> >>
> >> Based on a patch by Wen Congyang.
> >>
> >> Signed-off-by: Wen Congyang <address@hidden>
> >> Signed-off-by: Paolo Bonzini <address@hidden>
> > 
> > If you are touching this code anyway, maybe it makes
> > sense to merge Rusty's virtio len patches?
> > I didn't want them in 2.3 since they aren't ciritical,
> > but we are changing these lines anyway, maybe make
> > them correct?
> 
> My patch is a strict superset of Rusty's patch 2/2.  In fact the first
> version was very similar to his, but neither my v1 nor his patch covers
> SCSI or flush or get-serial requests.

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 - and on read error - we don't
        know whether the whole request size was written, so again len
        should be set to 0.

> 
> Rusty's patch 1/2 only adds an assertion, I think.  It's not critical
> for 2.3.
> 
> Paolo

I agree.

> >> ---
> >>  hw/block/dataplane/virtio-blk.c |  3 +--
> >>  hw/block/virtio-blk.c           | 13 ++++++++++++-
> >>  include/hw/virtio/virtio-blk.h  |  1 +
> >>  3 files changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/block/dataplane/virtio-blk.c 
> >> b/hw/block/dataplane/virtio-blk.c
> >> index cd41478..3db139b 100644
> >> --- a/hw/block/dataplane/virtio-blk.c
> >> +++ b/hw/block/dataplane/virtio-blk.c
> >> @@ -77,8 +77,7 @@ static void complete_request_vring(VirtIOBlockReq *req, 
> >> unsigned char status)
> >>      VirtIOBlockDataPlane *s = req->dev->dataplane;
> >>      stb_p(&req->in->status, status);
> >>  
> >> -    vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem,
> >> -               req->qiov.size + sizeof(*req->in));
> >> +    vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem, 
> >> req->in_len);
> >>  
> >>      /* Suppress notification to guest by BH and its scheduled
> >>       * flag because requests are completed as a batch after io
> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 000c38d..9546fd2 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -33,6 +33,7 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
> >>      VirtIOBlockReq *req = g_slice_new(VirtIOBlockReq);
> >>      req->dev = s;
> >>      req->qiov.size = 0;
> >> +    req->in_len = 0;
> >>      req->next = NULL;
> >>      req->mr_next = NULL;
> >>      return req;
> >> @@ -54,7 +55,7 @@ static void virtio_blk_complete_request(VirtIOBlockReq 
> >> *req,
> >>      trace_virtio_blk_req_complete(req, status);
> >>  
> >>      stb_p(&req->in->status, status);
> >> -    virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
> >> +    virtqueue_push(s->vq, &req->elem, req->in_len);
> >>      virtio_notify(vdev, s->vq);
> >>  }
> >>  
> >> @@ -102,6 +103,14 @@ static void virtio_blk_rw_complete(void *opaque, int 
> >> ret)
> >>          if (ret) {
> >>              int p = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &req->out.type);
> >>              bool is_read = !(p & VIRTIO_BLK_T_OUT);
> >> +            /* Note that memory may be dirtied on read failure.  If the
> >> +             * virtio request is not completed here, as is the case for
> >> +             * BLOCK_ERROR_ACTION_STOP, the memory may not be copied
> >> +             * correctly during live migration.  While this is ugly,
> >> +             * it is acceptable because the device is free to write to
> >> +             * the memory until the request is completed (which will
> >> +             * happen on the other side of the migration).
> >> +             */
> >>              if (virtio_blk_handle_rw_error(req, -ret, is_read)) {
> >>                  continue;
> >>              }
> >> @@ -496,6 +505,8 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, 
> >> MultiReqBuffer *mrb)
> >>          exit(1);
> >>      }
> >>  
> >> +    /* We always touch the last byte, so just see how big in_iov is.  */
> >> +    req->in_len = iov_size(in_iov, in_num);
> >>      req->in = (void *)in_iov[in_num - 1].iov_base
> >>                + in_iov[in_num - 1].iov_len
> >>                - sizeof(struct virtio_blk_inhdr);
> >> diff --git a/include/hw/virtio/virtio-blk.h 
> >> b/include/hw/virtio/virtio-blk.h
> >> index b3ffcd9..6bf5905 100644
> >> --- a/include/hw/virtio/virtio-blk.h
> >> +++ b/include/hw/virtio/virtio-blk.h
> >> @@ -67,6 +67,7 @@ typedef struct VirtIOBlockReq {
> >>      struct virtio_blk_inhdr *in;
> >>      struct virtio_blk_outhdr out;
> >>      QEMUIOVector qiov;
> >> +    size_t in_len;
> >>      struct VirtIOBlockReq *next;
> >>      struct VirtIOBlockReq *mr_next;
> >>      BlockAcctCookie acct;
> >> -- 
> >> 2.3.4