From 65165d795d1d6e6fb1627204c5c888f1bed3528b Mon Sep 17 00:00:00 2001 From: Dariusz Stojaczyk Date: Wed, 11 Jul 2018 17:43:15 +0200 Subject: [PATCH] virtio: fix vq init error handling We didn't handle vq alloc or init failure, because queues are initialized all at once on device init and if one vq fails, all of them are to be destroyed. This behavior is really unintuitive, and with the latest changes we have a possible segfault scenario. (We could spdk_dma_free() a buffer that failed to allocate). It is now required that the queue allocation function cleans up after itself. Change-Id: I6cd1d30c710eb9266288905ab982db363f972a1d Signed-off-by: Dariusz Stojaczyk Reviewed-on: https://review.gerrithub.io/419001 Tested-by: SPDK Automated Test System Reviewed-by: Jim Harris Reviewed-by: Ben Walker --- lib/virtio/virtio.c | 2 ++ lib/virtio/virtio_pci.c | 1 + lib/virtio/virtio_user.c | 19 +++++++++++-------- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/virtio/virtio.c b/lib/virtio/virtio.c index 1a9709a90..1e2991a1a 100644 --- a/lib/virtio/virtio.c +++ b/lib/virtio/virtio.c @@ -165,6 +165,8 @@ virtio_init_queue(struct virtio_dev *dev, uint16_t vtpci_queue_idx) if (virtio_dev_backend_ops(dev)->setup_queue(dev, vq) < 0) { SPDK_ERRLOG("setup_queue failed\n"); + spdk_dma_free(vq); + dev->vqs[vtpci_queue_idx] = NULL; return -EINVAL; } diff --git a/lib/virtio/virtio_pci.c b/lib/virtio/virtio_pci.c index 24f9b3572..2dc1e6f6b 100644 --- a/lib/virtio/virtio_pci.c +++ b/lib/virtio/virtio_pci.c @@ -281,6 +281,7 @@ modern_setup_queue(struct virtio_dev *dev, struct virtqueue *vq) vq->vq_ring_virt_mem = queue_mem; if (!check_vq_phys_addr_ok(vq)) { + spdk_dma_free(queue_mem); return -1; } diff --git a/lib/virtio/virtio_user.c b/lib/virtio/virtio_user.c index 6e30bcef0..a40b9f791 100644 --- a/lib/virtio/virtio_user.c +++ b/lib/virtio/virtio_user.c @@ -366,14 +366,6 @@ virtio_user_setup_queue(struct virtio_dev *vdev, struct virtqueue *vq) return -1; } - queue_mem = spdk_dma_zmalloc(vq->vq_ring_size, VIRTIO_PCI_VRING_ALIGN, NULL); - if (queue_mem == NULL) { - return -ENOMEM; - } - - vq->vq_ring_mem = SPDK_VTOPHYS_ERROR; - vq->vq_ring_virt_mem = queue_mem; - /* May use invalid flag, but some backend uses kickfd and * callfd as criteria to judge if dev is alive. so finally we * use real event_fd. @@ -391,6 +383,16 @@ virtio_user_setup_queue(struct virtio_dev *vdev, struct virtqueue *vq) return -1; } + queue_mem = spdk_dma_zmalloc(vq->vq_ring_size, VIRTIO_PCI_VRING_ALIGN, NULL); + if (queue_mem == NULL) { + close(kickfd); + close(callfd); + return -ENOMEM; + } + + vq->vq_ring_mem = SPDK_VTOPHYS_ERROR; + vq->vq_ring_virt_mem = queue_mem; + state.index = vq->vq_queue_index; state.num = 0; @@ -398,6 +400,7 @@ virtio_user_setup_queue(struct virtio_dev *vdev, struct virtqueue *vq) dev->ops->send_request(dev, VHOST_USER_SET_VRING_ENABLE, &state) < 0) { SPDK_ERRLOG("failed to send VHOST_USER_SET_VRING_ENABLE: %s\n", spdk_strerror(errno)); + spdk_dma_free(queue_mem); return -1; }