[Qemu-devel] [PATCH for 3.1 0/4] virtio migration load path

[Qemu-devel] [PATCH for 3.1 0/4] virtio migration load path

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

One day I'll figure out how to convert the
virtio load path and qemu_get_virtqeueue_element into
vmstate, but until then clean up some of the error paths.

Dave

Dr. David Alan Gilbert (4):
  scsi/migration: Allow bus load request to fail
  virtio: Check qemu_get_virtqueue_element returns
  virtio-scsi/migration: Allow load_request to fail
  virtio: qemu_get_virtqueue_element fail rather than assert

 hw/block/virtio-blk.c       |  4 ++++
 hw/char/virtio-serial-bus.c |  4 ++++
 hw/scsi/scsi-bus.c          |  5 +++++
 hw/scsi/virtio-scsi.c       | 19 ++++++++++++++++---
 hw/virtio/virtio.c          | 13 ++++++-------
 5 files changed, 35 insertions(+), 10 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH for 3.1 1/4] scsi/migration: Allow bus load request to fail

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Allow the load_request method on a bus to fail and error
the migration cleanly.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/scsi/scsi-bus.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 5905f6bf29..34a29c2ffd 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1618,6 +1618,11 @@ static int get_scsi_requests(QEMUFile *f, void *pv, size_t size,
         req->retry = (sbyte == 1);
         if (bus->info->load_request) {
             req->hba_private = bus->info->load_request(f, req);
+
+            if (!req->hba_private) {
+                scsi_req_unref(req);
+                return -1;
+            }
         }
         if (req->ops->load_request) {
             req->ops->load_request(f, req);
-- 
2.17.1

Re: [Qemu-devel] [PATCH for 3.1 1/4] scsi/migration: Allow bus load request to fail

[Cornelia Huck]

On Mon, 16 Jul 2018 18:37:40 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Allow the load_request method on a bus to fail and error
> the migration cleanly.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/scsi/scsi-bus.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[Qemu-devel] [PATCH for 3.1 2/4] virtio: Check qemu_get_virtqueue_element returns

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Check calls to qemu_get_virtqueue_element for NULL and pass
up the chain.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/block/virtio-blk.c       | 4 ++++
 hw/char/virtio-serial-bus.c | 4 ++++
 hw/scsi/virtio-scsi.c       | 4 ++++
 3 files changed, 12 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 50b5c869e3..324c6b2b27 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -888,6 +888,10 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
         }
 
         req = qemu_get_virtqueue_element(vdev, f, sizeof(VirtIOBlockReq));
+        if (!req) {
+            error_report("%s: Bad vq element %u", __func__, vq_idx);
+            return -EINVAL;
+        }
         virtio_blk_init_request(s, virtio_get_queue(vdev, vq_idx), req);
         req->next = s->rq;
         s->rq = req;
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index d2dd8ab502..e99dc9bf59 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -781,6 +781,10 @@ static int fetch_active_ports_list(QEMUFile *f,
 
             port->elem =
                 qemu_get_virtqueue_element(vdev, f, sizeof(VirtQueueElement));
+            if (!port->elem) {
+                error_report("%s: Bad vq element", __func__);
+                return -EINVAL;
+            }
 
             /*
              *  Port was throttled on source machine.  Let's
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3aa99717e2..6301af76ad 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -207,6 +207,10 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     assert(n < vs->conf.num_queues);
     req = qemu_get_virtqueue_element(vdev, f,
                                      sizeof(VirtIOSCSIReq) + vs->cdb_size);
+    if (!req) {
+        error_report("%s: Bad vq element", __func__);
+        return NULL;
+    }
     virtio_scsi_init_req(s, vs->cmd_vqs[n], req);
 
     if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
-- 
2.17.1

Re: [Qemu-devel] [PATCH for 3.1 2/4] virtio: Check qemu_get_virtqueue_element returns

[Cornelia Huck]

On Mon, 16 Jul 2018 18:37:41 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Check calls to qemu_get_virtqueue_element for NULL and pass
> up the chain.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/block/virtio-blk.c       | 4 ++++
>  hw/char/virtio-serial-bus.c | 4 ++++
>  hw/scsi/virtio-scsi.c       | 4 ++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 50b5c869e3..324c6b2b27 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -888,6 +888,10 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
>          }
>  
>          req = qemu_get_virtqueue_element(vdev, f, sizeof(VirtIOBlockReq));
> +        if (!req) {
> +            error_report("%s: Bad vq element %u", __func__, vq_idx);

Minor nit: vq_idx is the virtqueue index, and this message makes it
look like it is the 'bad vq element'... either add 'vq index', or drop
it completely from the error message?

> +            return -EINVAL;
> +        }
>          virtio_blk_init_request(s, virtio_get_queue(vdev, vq_idx), req);
>          req->next = s->rq;
>          s->rq = req;

Anyway,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH for 3.1 2/4] virtio: Check qemu_get_virtqueue_element returns

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

On Mon, Jul 16, 2018 at 06:37:41PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Check calls to qemu_get_virtqueue_element for NULL and pass
> up the chain.

What happens to the device state that has been partially deserialized
(e.g. virtio-blk's s->rq linked list)?

It's not clear to me that simply returning NULL is enough to put QEMU
into a sane state without memory leaks or crashes if we decide to retry.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH for 3.1 3/4] virtio-scsi/migration: Allow load_request to fail

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Rather than asserting, check values and return NULL
on failure.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/scsi/virtio-scsi.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 6301af76ad..3441bfe6d7 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -204,7 +204,11 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     uint32_t n;
 
     qemu_get_be32s(f, &n);
-    assert(n < vs->conf.num_queues);
+    if (n >= vs->conf.num_queues) {
+        error_report("%s: Bad queue number (%d vs %d)",
+                     __func__, n, vs->conf.num_queues);
+        return NULL;
+    }
     req = qemu_get_virtqueue_element(vdev, f,
                                      sizeof(VirtIOSCSIReq) + vs->cdb_size);
     if (!req) {
@@ -216,13 +220,18 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
                               sizeof(VirtIOSCSICmdResp) + vs->sense_size) < 0) {
         error_report("invalid SCSI request migration data");
-        exit(1);
+        return NULL;
     }
 
     scsi_req_ref(sreq);
     req->sreq = sreq;
     if (req->sreq->cmd.mode != SCSI_XFER_NONE) {
-        assert(req->sreq->cmd.mode == req->mode);
+        if (req->sreq->cmd.mode != req->mode) {
+            error_report("%s: Bad mode (%d vs %d)",
+                         __func__, req->sreq->cmd.mode, req->mode);
+            scsi_req_unref(sreq);
+            return NULL;
+        }
     }
     return req;
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH for 3.1 3/4] virtio-scsi/migration: Allow load_request to fail

[Cornelia Huck]

On Mon, 16 Jul 2018 18:37:42 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Rather than asserting, check values and return NULL
> on failure.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 6301af76ad..3441bfe6d7 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -204,7 +204,11 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
>      uint32_t n;
>  
>      qemu_get_be32s(f, &n);
> -    assert(n < vs->conf.num_queues);
> +    if (n >= vs->conf.num_queues) {
> +        error_report("%s: Bad queue number (%d vs %d)",
> +                     __func__, n, vs->conf.num_queues);

Hm... "(%d, num_queues %d)" might be a bit clearer?

> +        return NULL;
> +    }
>      req = qemu_get_virtqueue_element(vdev, f,
>                                       sizeof(VirtIOSCSIReq) + vs->cdb_size);
>      if (!req) {

Still,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[Qemu-devel] [PATCH for 3.1 4/4] virtio: qemu_get_virtqueue_element fail rather than assert

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Make it return NULL rather than assert.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/virtio.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d4e4d98b59..aedd390240 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1037,13 +1037,12 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
 
     qemu_get_buffer(f, (uint8_t *)&data, sizeof(VirtQueueElementOld));
 
-    /* TODO: teach all callers that this can fail, and return failure instead
-     * of asserting here.
-     * This is just one thing (there are probably more) that must be
-     * fixed before we can allow NDEBUG compilation.
-     */
-    assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
-    assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
+    if (data.in_num > ARRAY_SIZE(data.in_addr) ||
+        data.out_num > ARRAY_SIZE(data.out_addr)) {
+        error_report("%s: Bad index: in=%d out=%d",
+                    __func__, data.in_num, data.out_num);
+        return NULL;
+    }
 
     elem = virtqueue_alloc_element(sz, data.out_num, data.in_num);
     elem->index = data.index;
-- 
2.17.1

Re: [Qemu-devel] [PATCH for 3.1 4/4] virtio: qemu_get_virtqueue_element fail rather than assert

[Cornelia Huck]

On Mon, 16 Jul 2018 18:37:43 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

$SUBJECT reads a bit odd without a 'make', but it is already long...

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Make it return NULL rather than assert.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/virtio/virtio.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>