* [Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop
@ 2015-11-13 9:32 Fam Zheng
2015-11-13 10:48 ` Laurent Vivier
2015-11-13 13:17 ` Paolo Bonzini
0 siblings, 2 replies; 4+ messages in thread
From: Fam Zheng @ 2015-11-13 9:32 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, lvivier, qemu-block, pl, qemu-stable, Stefan Hajnoczi,
dgibson
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop
2015-11-13 9:32 [Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop Fam Zheng
@ 2015-11-13 10:48 ` Laurent Vivier
2015-11-13 13:17 ` Paolo Bonzini
1 sibling, 0 replies; 4+ messages in thread
From: Laurent Vivier @ 2015-11-13 10:48 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, pl, qemu-stable, Stefan Hajnoczi, dgibson
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.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop
2015-11-13 9:32 [Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop Fam Zheng
2015-11-13 10:48 ` Laurent Vivier
@ 2015-11-13 13:17 ` Paolo Bonzini
2015-11-16 5:52 ` Fam Zheng
1 sibling, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2015-11-13 13:17 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, lvivier, qemu-block, pl, qemu-stable, Stefan Hajnoczi,
dgibson
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;
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop
2015-11-13 13:17 ` Paolo Bonzini
@ 2015-11-16 5:52 ` Fam Zheng
0 siblings, 0 replies; 4+ messages in thread
From: Fam Zheng @ 2015-11-16 5:52 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, lvivier, qemu-block, pl, qemu-devel, qemu-stable,
Stefan Hajnoczi, dgibson
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-11-16 5:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-13 9:32 [Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop Fam Zheng
2015-11-13 10:48 ` Laurent Vivier
2015-11-13 13:17 ` Paolo Bonzini
2015-11-16 5:52 ` Fam Zheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).