[Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop

[Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop

[Fam Zheng]

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: qemu-stable@nongnu.org
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c          | 9 +++++++--
 include/hw/virtio/virtio-blk.h | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e70fccf..4871f2a 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;
 }
 
@@ -72,8 +73,11 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     VirtIOBlock *s = req->dev;
 
     if (action == BLOCK_ERROR_ACTION_STOP) {
-        req->next = s->rq;
-        s->rq = req;
+        /* Merged requests don't need to be restarted. */
+        if (!req->merged) {
+            req->next = s->rq;
+            s->rq = req;
+        }
     } else if (action == BLOCK_ERROR_ACTION_REPORT) {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         block_acct_failed(blk_get_stats(s->blk), &req->acct);
@@ -344,6 +348,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;
         }
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;
 
-- 
2.4.3

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

[Laurent Vivier]

On 13/11/2015 10:32, 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: qemu-stable@nongnu.org
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/block/virtio-blk.c          | 9 +++++++--
>  include/hw/virtio/virtio-blk.h | 1 +
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e70fccf..4871f2a 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;
>  }
>  
> @@ -72,8 +73,11 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
>      VirtIOBlock *s = req->dev;
>  
>      if (action == BLOCK_ERROR_ACTION_STOP) {
> -        req->next = s->rq;
> -        s->rq = req;
> +        /* Merged requests don't need to be restarted. */
> +        if (!req->merged) {
> +            req->next = s->rq;
> +            s->rq = req;
> +        }
>      } else if (action == BLOCK_ERROR_ACTION_REPORT) {
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
>          block_acct_failed(blk_get_stats(s->blk), &req->acct);
> @@ -344,6 +348,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;
>          }
> 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;
>  
> 

Tested-by: Laurent Vivier <lvivier@redhat.com>

In my case, this patch fixes a memory corruption when the VM is stopped
on io-error (no space) and restarted.

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

[Paolo Bonzini]

On 13/11/2015 10:32, 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: qemu-stable@nongnu.org
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/block/virtio-blk.c          | 9 +++++++--
>  include/hw/virtio/virtio-blk.h | 1 +
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e70fccf..4871f2a 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;
>  }
>  
> @@ -72,8 +73,11 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
>      VirtIOBlock *s = req->dev;
>  
>      if (action == BLOCK_ERROR_ACTION_STOP) {
> -        req->next = s->rq;
> -        s->rq = req;
> +        /* Merged requests don't need to be restarted. */
> +        if (!req->merged) {
> +            req->next = s->rq;
> +            s->rq = req;
> +        }

I think it still needs to be queued, so that the request is migrated
correctly.  Should req->merged be checked in virtio_blk_dma_restart_bh
instead?

Paolo

>      } else if (action == BLOCK_ERROR_ACTION_REPORT) {
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
>          block_acct_failed(blk_get_stats(s->blk), &req->acct);
> @@ -344,6 +348,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;
>          }
> 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;
>  
> 

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

[Fam Zheng]

On Fri, 11/13 14:17, Paolo Bonzini wrote:
> 
> 
> On 13/11/2015 10:32, 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: qemu-stable@nongnu.org
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/block/virtio-blk.c          | 9 +++++++--
> >  include/hw/virtio/virtio-blk.h | 1 +
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index e70fccf..4871f2a 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;
> >  }
> >  
> > @@ -72,8 +73,11 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> >      VirtIOBlock *s = req->dev;
> >  
> >      if (action == BLOCK_ERROR_ACTION_STOP) {
> > -        req->next = s->rq;
> > -        s->rq = req;
> > +        /* Merged requests don't need to be restarted. */
> > +        if (!req->merged) {
> > +            req->next = s->rq;
> > +            s->rq = req;
> > +        }
> 
> I think it still needs to be queued, so that the request is migrated
> correctly.  Should req->merged be checked in virtio_blk_dma_restart_bh
> instead?

Yes you're right.

Thanks,
Fam