Re: [Qemu-block] [PATCH v2] virtio-blk: Fix double completion for werror=stop

[Laurent Vivier]

On 16/11/2015 07:10, Fam Zheng wrote:
> When a request R is absorbed by request M, it is appended to the
> "mr_next" queue led by M, and is completed together with the completion
> of M, in virtio_blk_rw_complete.
> 
> With error policy equals stop, if M has an I/O error, now R also gets
> prepended to the per device DMA restart queue, which will be retried
> when VM resumes.  It leads to a double completion (in symptoms of memory
> corruption or use after free).
> 
> Adding R to the queue is superfluous, only M needs to be in the queue.
> 
> Fix this by marking request R as "merged" and skipping it in
> virtio_blk_handle_rw_error.
> 
> Cc: address@hidden
> Signed-off-by: Fam Zheng <address@hidden>
> 
> ---
> 
> v2: Don't lose the request in migration. [Paolo]

Tested-by: Laurent Vivier <address@hidden>


> ---
>  hw/block/virtio-blk.c          | 7 +++++++
>  include/hw/virtio/virtio-blk.h | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e70fccf..5cdb06f 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -36,6 +36,7 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
>      req->in_len = 0;
>      req->next = NULL;
>      req->mr_next = NULL;
> +    req->merged = false;
>      return req;
>  }
>  
> @@ -344,6 +345,7 @@ static inline void submit_requests(BlockBackend *blk, 
> MultiReqBuffer *mrb,
>          for (i = start + 1; i < start + num_reqs; i++) {
>              qemu_iovec_concat(qiov, &mrb->reqs[i]->qiov, 0,
>                                mrb->reqs[i]->qiov.size);
> +            mrb->reqs[i]->merged = true;
>              mrb->reqs[i - 1]->mr_next = mrb->reqs[i];
>              nb_sectors += mrb->reqs[i]->qiov.size / BDRV_SECTOR_SIZE;
>          }
> @@ -511,6 +513,11 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, 
> MultiReqBuffer *mrb)
>                - sizeof(struct virtio_blk_inhdr);
>      iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
>  
> +    if (req->merged) {
> +        /* Enough for restarting a (migrated) merged request, no need to
> +         * actually submit I/O. */
> +        return;
> +    }
>      type = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &req->out.type);
>  
>      /* VIRTIO_BLK_T_OUT defines the command direction. VIRTIO_BLK_T_BARRIER
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 6bf5905..db4adf4 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -70,6 +70,7 @@ typedef struct VirtIOBlockReq {
>      size_t in_len;
>      struct VirtIOBlockReq *next;
>      struct VirtIOBlockReq *mr_next;
> +    bool merged;
>      BlockAcctCookie acct;
>  } VirtIOBlockReq;
>  
>