Commit 65165d79 authored by Dariusz Stojaczyk's avatar Dariusz Stojaczyk Committed by Ben Walker
Browse files

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: default avatarDariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/419001


Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
parent e8762a41

lib/virtio/virtio.c

+2 −0
Original line number Diff line number Diff line
@@ -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;

	}

lib/virtio/virtio_pci.c

+1 −0
Original line number Diff line number Diff line
@@ -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;

	}

lib/virtio/virtio_user.c

+11 −8
Original line number Diff line number Diff line
@@ -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;

	}