| Message ID | 147447704092.30952.7983028961569052217.stgit@bahia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 21 Sep 2016 18:57:20 +0200 Greg Kurz <groug@kaod.org> wrote: > All these errors are caused by a buggy guest: let's switch the device to > the broken state instead of terminating QEMU. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > v2: - added a out_err: label in virtio_blk_handle_vq() > --- > hw/block/virtio-blk.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > @@ -586,13 +589,16 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) > blk_io_plug(s->blk); > > while ((req = virtio_blk_get_request(s, vq))) { > - virtio_blk_handle_request(req, &mrb); > + if (virtio_blk_handle_request(req, &mrb)) { > + goto out_err; > + } > } > > if (mrb.num_reqs) { > virtio_blk_submit_multireq(s->blk, &mrb); > } > > +out_err: I would probably have called that label out_unplug instead, but still Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> > blk_io_unplug(s->blk); > } >
On Wed, Sep 21, 2016 at 06:57:20PM +0200, Greg Kurz wrote: > @@ -586,13 +589,16 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) > blk_io_plug(s->blk); > > while ((req = virtio_blk_get_request(s, vq))) { > - virtio_blk_handle_request(req, &mrb); > + if (virtio_blk_handle_request(req, &mrb)) { > + goto out_err; > + } > } > > if (mrb.num_reqs) { > virtio_blk_submit_multireq(s->blk, &mrb); > } > > +out_err: > blk_io_unplug(s->blk); req is leaked. We must detach it from the virtqueue and free it. > } > > @@ -625,7 +631,9 @@ static void virtio_blk_dma_restart_bh(void *opaque) > > while (req) { > VirtIOBlockReq *next = req->next; > - virtio_blk_handle_request(req, &mrb); > + if (virtio_blk_handle_request(req, &mrb)) { > + return; s->rq is leaked. We must detach and free the remaining requests. See virtio_blk_reset().
On Fri, 23 Sep 2016 13:58:56 +0100 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Wed, Sep 21, 2016 at 06:57:20PM +0200, Greg Kurz wrote: > > @@ -586,13 +589,16 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) > > blk_io_plug(s->blk); > > > > while ((req = virtio_blk_get_request(s, vq))) { > > - virtio_blk_handle_request(req, &mrb); > > + if (virtio_blk_handle_request(req, &mrb)) { > > + goto out_err; > > + } > > } > > > > if (mrb.num_reqs) { > > virtio_blk_submit_multireq(s->blk, &mrb); > > } > > > > +out_err: > > blk_io_unplug(s->blk); > > req is leaked. We must detach it from the virtqueue and free it. > > > } > > > > @@ -625,7 +631,9 @@ static void virtio_blk_dma_restart_bh(void *opaque) > > > > while (req) { > > VirtIOBlockReq *next = req->next; > > - virtio_blk_handle_request(req, &mrb); > > + if (virtio_blk_handle_request(req, &mrb)) { > > + return; > > s->rq is leaked. We must detach and free the remaining requests. See > virtio_blk_reset(). I only see virtio_blk_free_request() being called there in QEMU master. I guess you are talking about the following series ? [PATCH 0/3] virtio: detach VirtQueueElements freed by reset <1474291685-24226-1-git-send-email-stefanha@redhat.com> Cheers. -- Greg
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 09579968ad89..9865fa6ca3c0 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -468,30 +468,32 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev, return true; } -static void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) +static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) { uint32_t type; struct iovec *in_iov = req->elem.in_sg; struct iovec *iov = req->elem.out_sg; unsigned in_num = req->elem.in_num; unsigned out_num = req->elem.out_num; + VirtIOBlock *s = req->dev; + VirtIODevice *vdev = VIRTIO_DEVICE(s); if (req->elem.out_num < 1 || req->elem.in_num < 1) { - error_report("virtio-blk missing headers"); - exit(1); + virtio_error(vdev, "virtio-blk missing headers"); + return -1; } if (unlikely(iov_to_buf(iov, out_num, 0, &req->out, sizeof(req->out)) != sizeof(req->out))) { - error_report("virtio-blk request outhdr too short"); - exit(1); + virtio_error(vdev, "virtio-blk request outhdr too short"); + return -1; } iov_discard_front(&iov, &out_num, sizeof(req->out)); if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) { - error_report("virtio-blk request inhdr too short"); - exit(1); + virtio_error(vdev, "virtio-blk request inhdr too short"); + return -1; } /* We always touch the last byte, so just see how big in_iov is. */ @@ -529,7 +531,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) block_acct_invalid(blk_get_stats(req->dev->blk), is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ); virtio_blk_free_request(req); - return; + return 0; } block_acct_start(blk_get_stats(req->dev->blk), @@ -576,6 +578,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP); virtio_blk_free_request(req); } + return 0; } void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) @@ -586,13 +589,16 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) blk_io_plug(s->blk); while ((req = virtio_blk_get_request(s, vq))) { - virtio_blk_handle_request(req, &mrb); + if (virtio_blk_handle_request(req, &mrb)) { + goto out_err; + } } if (mrb.num_reqs) { virtio_blk_submit_multireq(s->blk, &mrb); } +out_err: blk_io_unplug(s->blk); } @@ -625,7 +631,9 @@ static void virtio_blk_dma_restart_bh(void *opaque) while (req) { VirtIOBlockReq *next = req->next; - virtio_blk_handle_request(req, &mrb); + if (virtio_blk_handle_request(req, &mrb)) { + return; + } req = next; }
All these errors are caused by a buggy guest: let's switch the device to the broken state instead of terminating QEMU. Signed-off-by: Greg Kurz <groug@kaod.org> --- v2: - added a out_err: label in virtio_blk_handle_vq() --- hw/block/virtio-blk.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)