[PATCH 1/3] vhost: fix malloc in rte_vhost_get_mem_table

[PATCH 1/3] vhost: fix malloc in rte_vhost_get_mem_table

[Dariusz Stojaczyk]

Amount of allocated memory was too small, causing buffer overflow.

Change-Id: I02293cd4bcddef14d2bbf314c79d8c65fc44e555
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
 lib/librte_vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 0b19d2e..1f565fb 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -369,7 +369,7 @@ rte_vhost_get_mem_table(int vid, struct rte_vhost_memory **mem)
 		return -1;
 
 	size = dev->mem->nregions * sizeof(struct rte_vhost_mem_region);
-	m = malloc(size);
+	m = malloc(sizeof(struct rte_vhost_memory) + size);
 	if (!m)
 		return -1;
 
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH 2/3] vhost: free virtio_net::guest_pages in vhost_backend_cleanup()

[Dariusz Stojaczyk]

guest_pages is being allocated in vhost_setup_mem_table(), reallocated
in add_one_guest_page(), but never freed. This patch fixes a memory leak.

Change-Id: I6edff00298a668054f4d106ffdaea828f41912fe
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
 lib/librte_vhost/vhost_user.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 5c8058b..2cc0b66 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -114,6 +114,10 @@ vhost_backend_cleanup(struct virtio_net *dev)
 		rte_free(dev->mem);
 		dev->mem = NULL;
 	}
+
+	free(dev->guest_pages);
+	dev->guest_pages = NULL;
+
 	if (dev->log_addr) {
 		munmap((void *)(uintptr_t)dev->log_addr, dev->log_size);
 		dev->log_addr = 0;
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH 3/3] vhost: access VhostUsrMsg via packed struct

[Dariusz Stojaczyk]

From: Daniel Verkamp <daniel.verkamp@intel.com>

Fixes unaligned access to fields.

Change-Id: I2f8246481dc96d7e73134866f55a4227fc39031d
Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
 lib/librte_vhost/vhost_user.c | 56 +++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 2cc0b66..0035ce4 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -197,11 +197,11 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
  */
 static int
 vhost_user_set_vring_num(struct virtio_net *dev,
-			 struct vhost_vring_state *state)
+			 VhostUserMsg *msg)
 {
-	struct vhost_virtqueue *vq = dev->virtqueue[state->index];
+	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
-	vq->size = state->num;
+	vq->size = msg->payload.state.num;
 
 	if (dev->dequeue_zero_copy) {
 		vq->nr_zmbuf = 0;
@@ -334,7 +334,7 @@ qva_to_vva(struct virtio_net *dev, uint64_t qva)
  * This function then converts these to our address space.
  */
 static int
-vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
+vhost_user_set_vring_addr(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	struct vhost_virtqueue *vq;
 
@@ -342,11 +342,11 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		return -1;
 
 	/* addr->index refers to the queue index. The txq 1, rxq is 0. */
-	vq = dev->virtqueue[addr->index];
+	vq = dev->virtqueue[msg->payload.addr.index];
 
 	/* The addresses are converted from QEMU virtual to Vhost virtual. */
 	vq->desc = (struct vring_desc *)(uintptr_t)qva_to_vva(dev,
-			addr->desc_user_addr);
+			msg->payload.addr.desc_user_addr);
 	if (vq->desc == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find desc ring address.\n",
@@ -354,11 +354,11 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		return -1;
 	}
 
-	dev = numa_realloc(dev, addr->index);
-	vq = dev->virtqueue[addr->index];
+	dev = numa_realloc(dev, msg->payload.addr.index);
+	vq = dev->virtqueue[msg->payload.addr.index];
 
 	vq->avail = (struct vring_avail *)(uintptr_t)qva_to_vva(dev,
-			addr->avail_user_addr);
+			msg->payload.addr.avail_user_addr);
 	if (vq->avail == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find avail ring address.\n",
@@ -367,7 +367,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 	}
 
 	vq->used = (struct vring_used *)(uintptr_t)qva_to_vva(dev,
-			addr->used_user_addr);
+			msg->payload.addr.used_user_addr);
 	if (vq->used == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find used ring address.\n",
@@ -384,7 +384,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		vq->last_avail_idx = vq->used->idx;
 	}
 
-	vq->log_guest_addr = addr->log_guest_addr;
+	vq->log_guest_addr = msg->payload.addr.log_guest_addr;
 
 	LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n",
 			dev->vid, vq->desc);
@@ -403,10 +403,10 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
  */
 static int
 vhost_user_set_vring_base(struct virtio_net *dev,
-			  struct vhost_vring_state *state)
+			  VhostUserMsg *msg)
 {
-	dev->virtqueue[state->index]->last_used_idx  = state->num;
-	dev->virtqueue[state->index]->last_avail_idx = state->num;
+	dev->virtqueue[msg->payload.state.index]->last_used_idx  = msg->payload.state.num;
+	dev->virtqueue[msg->payload.state.index]->last_avail_idx = msg->payload.state.num;
 
 	return 0;
 }
@@ -697,9 +697,9 @@ free_zmbufs(struct vhost_virtqueue *vq)
  */
 static int
 vhost_user_get_vring_base(struct virtio_net *dev,
-			  struct vhost_vring_state *state)
+			  VhostUserMsg *msg)
 {
-	struct vhost_virtqueue *vq = dev->virtqueue[state->index];
+	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
 	/* We have to stop the queue (virtio) if it is running. */
 	if (dev->flags & VIRTIO_DEV_RUNNING) {
@@ -710,10 +710,10 @@ vhost_user_get_vring_base(struct virtio_net *dev,
 	dev->flags &= ~VIRTIO_DEV_READY;
 
 	/* Here we are safe to get the last used index */
-	state->num = vq->last_used_idx;
+	msg->payload.state.num = vq->last_used_idx;
 
 	RTE_LOG(INFO, VHOST_CONFIG,
-		"vring base idx:%d file:%d\n", state->index, state->num);
+		"vring base idx:%d file:%d\n", msg->payload.state.index, msg->payload.state.num);
 	/*
 	 * Based on current qemu vhost-user implementation, this message is
 	 * sent and only sent in vhost_vring_stop.
@@ -738,18 +738,18 @@ vhost_user_get_vring_base(struct virtio_net *dev,
  */
 static int
 vhost_user_set_vring_enable(struct virtio_net *dev,
-			    struct vhost_vring_state *state)
+			    VhostUserMsg *msg)
 {
-	int enable = (int)state->num;
+	int enable = (int)msg->payload.state.num;
 
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"set queue enable: %d to qp idx: %d\n",
-		enable, state->index);
+		enable, msg->payload.state.index);
 
 	if (dev->notify_ops->vring_state_changed)
-		dev->notify_ops->vring_state_changed(dev->vid, state->index, enable);
+		dev->notify_ops->vring_state_changed(dev->vid, msg->payload.state.index, enable);
 
-	dev->virtqueue[state->index]->enabled = enable;
+	dev->virtqueue[msg->payload.state.index]->enabled = enable;
 
 	return 0;
 }
@@ -1038,17 +1038,17 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
-		vhost_user_set_vring_num(dev, &msg.payload.state);
+		vhost_user_set_vring_num(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_ADDR:
-		vhost_user_set_vring_addr(dev, &msg.payload.addr);
+		vhost_user_set_vring_addr(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_BASE:
-		vhost_user_set_vring_base(dev, &msg.payload.state);
+		vhost_user_set_vring_base(dev, &msg);
 		break;
 
 	case VHOST_USER_GET_VRING_BASE:
-		vhost_user_get_vring_base(dev, &msg.payload.state);
+		vhost_user_get_vring_base(dev, &msg);
 		msg.size = sizeof(msg.payload.state);
 		send_vhost_message(fd, &msg);
 		break;
@@ -1073,7 +1073,7 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_ENABLE:
-		vhost_user_set_vring_enable(dev, &msg.payload.state);
+		vhost_user_set_vring_enable(dev, &msg);
 		break;
 	case VHOST_USER_SEND_RARP:
 		vhost_user_send_rarp(dev, &msg);
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH v2 1/3] vhost: fix malloc in rte_vhost_get_mem_table

[Dariusz Stojaczyk]

Amount of allocated memory was too small, causing buffer overflow.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
Removed Gerrit Change-Id
 lib/librte_vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 0b19d2e..1f565fb 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -369,7 +369,7 @@ rte_vhost_get_mem_table(int vid, struct rte_vhost_memory **mem)
 		return -1;
 
 	size = dev->mem->nregions * sizeof(struct rte_vhost_mem_region);
-	m = malloc(size);
+	m = malloc(sizeof(struct rte_vhost_memory) + size);
 	if (!m)
 		return -1;
 
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH v2 2/3] vhost: free virtio_net::guest_pages in vhost_backend_cleanup()

[Dariusz Stojaczyk]

guest_pages is being allocated in vhost_setup_mem_table(), reallocated
in add_one_guest_page(), but never freed. This patch fixes a memory leak.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
Removed Gerrit Change-Id
 lib/librte_vhost/vhost_user.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 5c8058b..2cc0b66 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -114,6 +114,10 @@ vhost_backend_cleanup(struct virtio_net *dev)
 		rte_free(dev->mem);
 		dev->mem = NULL;
 	}
+
+	free(dev->guest_pages);
+	dev->guest_pages = NULL;
+
 	if (dev->log_addr) {
 		munmap((void *)(uintptr_t)dev->log_addr, dev->log_size);
 		dev->log_addr = 0;
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH v2 3/3] vhost: access VhostUsrMsg via packed struct

[Dariusz Stojaczyk]

From: Daniel Verkamp <daniel.verkamp@intel.com>

Fixes unaligned access to fields.

Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
Removed Gerrit Change-Id
 lib/librte_vhost/vhost_user.c | 56 +++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 2cc0b66..0035ce4 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -197,11 +197,11 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
  */
 static int
 vhost_user_set_vring_num(struct virtio_net *dev,
-			 struct vhost_vring_state *state)
+			 VhostUserMsg *msg)
 {
-	struct vhost_virtqueue *vq = dev->virtqueue[state->index];
+	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
-	vq->size = state->num;
+	vq->size = msg->payload.state.num;
 
 	if (dev->dequeue_zero_copy) {
 		vq->nr_zmbuf = 0;
@@ -334,7 +334,7 @@ qva_to_vva(struct virtio_net *dev, uint64_t qva)
  * This function then converts these to our address space.
  */
 static int
-vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
+vhost_user_set_vring_addr(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	struct vhost_virtqueue *vq;
 
@@ -342,11 +342,11 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		return -1;
 
 	/* addr->index refers to the queue index. The txq 1, rxq is 0. */
-	vq = dev->virtqueue[addr->index];
+	vq = dev->virtqueue[msg->payload.addr.index];
 
 	/* The addresses are converted from QEMU virtual to Vhost virtual. */
 	vq->desc = (struct vring_desc *)(uintptr_t)qva_to_vva(dev,
-			addr->desc_user_addr);
+			msg->payload.addr.desc_user_addr);
 	if (vq->desc == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find desc ring address.\n",
@@ -354,11 +354,11 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		return -1;
 	}
 
-	dev = numa_realloc(dev, addr->index);
-	vq = dev->virtqueue[addr->index];
+	dev = numa_realloc(dev, msg->payload.addr.index);
+	vq = dev->virtqueue[msg->payload.addr.index];
 
 	vq->avail = (struct vring_avail *)(uintptr_t)qva_to_vva(dev,
-			addr->avail_user_addr);
+			msg->payload.addr.avail_user_addr);
 	if (vq->avail == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find avail ring address.\n",
@@ -367,7 +367,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 	}
 
 	vq->used = (struct vring_used *)(uintptr_t)qva_to_vva(dev,
-			addr->used_user_addr);
+			msg->payload.addr.used_user_addr);
 	if (vq->used == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find used ring address.\n",
@@ -384,7 +384,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		vq->last_avail_idx = vq->used->idx;
 	}
 
-	vq->log_guest_addr = addr->log_guest_addr;
+	vq->log_guest_addr = msg->payload.addr.log_guest_addr;
 
 	LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n",
 			dev->vid, vq->desc);
@@ -403,10 +403,10 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
  */
 static int
 vhost_user_set_vring_base(struct virtio_net *dev,
-			  struct vhost_vring_state *state)
+			  VhostUserMsg *msg)
 {
-	dev->virtqueue[state->index]->last_used_idx  = state->num;
-	dev->virtqueue[state->index]->last_avail_idx = state->num;
+	dev->virtqueue[msg->payload.state.index]->last_used_idx  = msg->payload.state.num;
+	dev->virtqueue[msg->payload.state.index]->last_avail_idx = msg->payload.state.num;
 
 	return 0;
 }
@@ -697,9 +697,9 @@ free_zmbufs(struct vhost_virtqueue *vq)
  */
 static int
 vhost_user_get_vring_base(struct virtio_net *dev,
-			  struct vhost_vring_state *state)
+			  VhostUserMsg *msg)
 {
-	struct vhost_virtqueue *vq = dev->virtqueue[state->index];
+	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
 	/* We have to stop the queue (virtio) if it is running. */
 	if (dev->flags & VIRTIO_DEV_RUNNING) {
@@ -710,10 +710,10 @@ vhost_user_get_vring_base(struct virtio_net *dev,
 	dev->flags &= ~VIRTIO_DEV_READY;
 
 	/* Here we are safe to get the last used index */
-	state->num = vq->last_used_idx;
+	msg->payload.state.num = vq->last_used_idx;
 
 	RTE_LOG(INFO, VHOST_CONFIG,
-		"vring base idx:%d file:%d\n", state->index, state->num);
+		"vring base idx:%d file:%d\n", msg->payload.state.index, msg->payload.state.num);
 	/*
 	 * Based on current qemu vhost-user implementation, this message is
 	 * sent and only sent in vhost_vring_stop.
@@ -738,18 +738,18 @@ vhost_user_get_vring_base(struct virtio_net *dev,
  */
 static int
 vhost_user_set_vring_enable(struct virtio_net *dev,
-			    struct vhost_vring_state *state)
+			    VhostUserMsg *msg)
 {
-	int enable = (int)state->num;
+	int enable = (int)msg->payload.state.num;
 
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"set queue enable: %d to qp idx: %d\n",
-		enable, state->index);
+		enable, msg->payload.state.index);
 
 	if (dev->notify_ops->vring_state_changed)
-		dev->notify_ops->vring_state_changed(dev->vid, state->index, enable);
+		dev->notify_ops->vring_state_changed(dev->vid, msg->payload.state.index, enable);
 
-	dev->virtqueue[state->index]->enabled = enable;
+	dev->virtqueue[msg->payload.state.index]->enabled = enable;
 
 	return 0;
 }
@@ -1038,17 +1038,17 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
-		vhost_user_set_vring_num(dev, &msg.payload.state);
+		vhost_user_set_vring_num(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_ADDR:
-		vhost_user_set_vring_addr(dev, &msg.payload.addr);
+		vhost_user_set_vring_addr(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_BASE:
-		vhost_user_set_vring_base(dev, &msg.payload.state);
+		vhost_user_set_vring_base(dev, &msg);
 		break;
 
 	case VHOST_USER_GET_VRING_BASE:
-		vhost_user_get_vring_base(dev, &msg.payload.state);
+		vhost_user_get_vring_base(dev, &msg);
 		msg.size = sizeof(msg.payload.state);
 		send_vhost_message(fd, &msg);
 		break;
@@ -1073,7 +1073,7 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_ENABLE:
-		vhost_user_set_vring_enable(dev, &msg.payload.state);
+		vhost_user_set_vring_enable(dev, &msg);
 		break;
 	case VHOST_USER_SEND_RARP:
 		vhost_user_send_rarp(dev, &msg);
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

Re: [PATCH v2 1/3] vhost: fix malloc in rte_vhost_get_mem_table

[Jens Freimann]

On Thu, May 11, 2017 at 12:56:46PM +0200, Dariusz Stojaczyk wrote:
> Amount of allocated memory was too small, causing buffer overflow.
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> ---
> Removed Gerrit Change-Id
>  lib/librte_vhost/vhost.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 0b19d2e..1f565fb 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -369,7 +369,7 @@ rte_vhost_get_mem_table(int vid, struct rte_vhost_memory **mem)
>  		return -1;
>  
>  	size = dev->mem->nregions * sizeof(struct rte_vhost_mem_region);
> -	m = malloc(size);
> +	m = malloc(sizeof(struct rte_vhost_memory) + size);

Why not just add it to the line above where size is calculated?
With that changed,

Reviewed-by: Jens Freimann <jfreimann@redhat.com> 


regards,
Jens

Re: [PATCH v2 1/3] vhost: fix malloc in rte_vhost_get_mem_table

[Stojaczyk, DariuszX]

The size variable is still used a few lines later:
memcpy(m->regions, dev->mem->regions, size);
That line is ok. Only the amount of malloc'ed memory was too small.

-----Original Message-----
From: Jens Freimann [mailto:jfreiman@redhat.com] 
Sent: Thursday, May 11, 2017 1:42 PM
To: Stojaczyk, DariuszX <dariuszx.stojaczyk@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2 1/3] vhost: fix malloc in rte_vhost_get_mem_table

On Thu, May 11, 2017 at 12:56:46PM +0200, Dariusz Stojaczyk wrote:
> Amount of allocated memory was too small, causing buffer overflow.
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> ---
> Removed Gerrit Change-Id
>  lib/librte_vhost/vhost.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index 
> 0b19d2e..1f565fb 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -369,7 +369,7 @@ rte_vhost_get_mem_table(int vid, struct rte_vhost_memory **mem)
>  		return -1;
>  
>  	size = dev->mem->nregions * sizeof(struct rte_vhost_mem_region);
> -	m = malloc(size);
> +	m = malloc(sizeof(struct rte_vhost_memory) + size);

Why not just add it to the line above where size is calculated?
With that changed,

Reviewed-by: Jens Freimann <jfreimann@redhat.com> 


regards,
Jens

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

Re: [PATCH v3 1/3] vhost: fix malloc in rte_vhost_get_mem_table()

[Jens Freimann]

On Thu, May 11, 2017 at 04:33:10PM +0200, Dariusz Stojaczyk wrote:
> Amount of allocated memory was too small, causing buffer overflow.
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> ---
>  lib/librte_vhost/vhost.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

Re: [PATCH v3 2/3] vhost: free guest_pages in vhost_backend_cleanup()

[Jens Freimann]

On Thu, May 11, 2017 at 04:33:11PM +0200, Dariusz Stojaczyk wrote:
> This patch fixes a memory leak.
> virtio_net::guest_pages is allocated in vhost_setup_mem_table(),
> reallocated in add_one_guest_page(), but never freed.
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

[PATCH v3 0/3] vhost undefined behavior fixes

[Dariusz Stojaczyk]

Fixes for memory-related undefined behavior issues in rte_vhost.

Daniel Verkamp (1):
  vhost: access VhostUsrMsg via packed struct

Dariusz Stojaczyk (2):
  vhost: fix malloc in rte_vhost_get_mem_table()
  vhost: free guest_pages in vhost_backend_cleanup()

 lib/librte_vhost/vhost.c      |  2 +-
 lib/librte_vhost/vhost_user.c | 64 ++++++++++++++++++++++++-------------------
 2 files changed, 37 insertions(+), 29 deletions(-)

-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH v3 1/3] vhost: fix malloc in rte_vhost_get_mem_table()

[Dariusz Stojaczyk]

Amount of allocated memory was too small, causing buffer overflow.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
 lib/librte_vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 0b19d2e..1f565fb 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -369,7 +369,7 @@ rte_vhost_get_mem_table(int vid, struct rte_vhost_memory **mem)
 		return -1;
 
 	size = dev->mem->nregions * sizeof(struct rte_vhost_mem_region);
-	m = malloc(size);
+	m = malloc(sizeof(struct rte_vhost_memory) + size);
 	if (!m)
 		return -1;
 
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH v3 2/3] vhost: free guest_pages in vhost_backend_cleanup()

[Dariusz Stojaczyk]

This patch fixes a memory leak.
virtio_net::guest_pages is allocated in vhost_setup_mem_table(),
reallocated in add_one_guest_page(), but never freed.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
 lib/librte_vhost/vhost_user.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 5c8058b..2cc0b66 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -114,6 +114,10 @@ vhost_backend_cleanup(struct virtio_net *dev)
 		rte_free(dev->mem);
 		dev->mem = NULL;
 	}
+
+	free(dev->guest_pages);
+	dev->guest_pages = NULL;
+
 	if (dev->log_addr) {
 		munmap((void *)(uintptr_t)dev->log_addr, dev->log_size);
 		dev->log_addr = 0;
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH v3 3/3] vhost: access VhostUsrMsg via packed struct

[Dariusz Stojaczyk]

From: Daniel Verkamp <daniel.verkamp@intel.com>

Fixes unaligned access to fields.

Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
Fixed checkpatch warnings
 lib/librte_vhost/vhost_user.c | 60 +++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 2cc0b66..ab2f40a 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -197,11 +197,11 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
  */
 static int
 vhost_user_set_vring_num(struct virtio_net *dev,
-			 struct vhost_vring_state *state)
+			 VhostUserMsg *msg)
 {
-	struct vhost_virtqueue *vq = dev->virtqueue[state->index];
+	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
-	vq->size = state->num;
+	vq->size = msg->payload.state.num;
 
 	if (dev->dequeue_zero_copy) {
 		vq->nr_zmbuf = 0;
@@ -334,7 +334,7 @@ qva_to_vva(struct virtio_net *dev, uint64_t qva)
  * This function then converts these to our address space.
  */
 static int
-vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
+vhost_user_set_vring_addr(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	struct vhost_virtqueue *vq;
 
@@ -342,11 +342,11 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		return -1;
 
 	/* addr->index refers to the queue index. The txq 1, rxq is 0. */
-	vq = dev->virtqueue[addr->index];
+	vq = dev->virtqueue[msg->payload.addr.index];
 
 	/* The addresses are converted from QEMU virtual to Vhost virtual. */
 	vq->desc = (struct vring_desc *)(uintptr_t)qva_to_vva(dev,
-			addr->desc_user_addr);
+			msg->payload.addr.desc_user_addr);
 	if (vq->desc == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find desc ring address.\n",
@@ -354,11 +354,11 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		return -1;
 	}
 
-	dev = numa_realloc(dev, addr->index);
-	vq = dev->virtqueue[addr->index];
+	dev = numa_realloc(dev, msg->payload.addr.index);
+	vq = dev->virtqueue[msg->payload.addr.index];
 
 	vq->avail = (struct vring_avail *)(uintptr_t)qva_to_vva(dev,
-			addr->avail_user_addr);
+			msg->payload.addr.avail_user_addr);
 	if (vq->avail == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find avail ring address.\n",
@@ -367,7 +367,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 	}
 
 	vq->used = (struct vring_used *)(uintptr_t)qva_to_vva(dev,
-			addr->used_user_addr);
+			msg->payload.addr.used_user_addr);
 	if (vq->used == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find used ring address.\n",
@@ -384,7 +384,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		vq->last_avail_idx = vq->used->idx;
 	}
 
-	vq->log_guest_addr = addr->log_guest_addr;
+	vq->log_guest_addr = msg->payload.addr.log_guest_addr;
 
 	LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n",
 			dev->vid, vq->desc);
@@ -403,10 +403,12 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
  */
 static int
 vhost_user_set_vring_base(struct virtio_net *dev,
-			  struct vhost_vring_state *state)
+			  VhostUserMsg *msg)
 {
-	dev->virtqueue[state->index]->last_used_idx  = state->num;
-	dev->virtqueue[state->index]->last_avail_idx = state->num;
+	dev->virtqueue[msg->payload.state.index]->last_used_idx  =
+			msg->payload.state.num;
+	dev->virtqueue[msg->payload.state.index]->last_avail_idx =
+			msg->payload.state.num;
 
 	return 0;
 }
@@ -697,9 +699,9 @@ free_zmbufs(struct vhost_virtqueue *vq)
  */
 static int
 vhost_user_get_vring_base(struct virtio_net *dev,
-			  struct vhost_vring_state *state)
+			  VhostUserMsg *msg)
 {
-	struct vhost_virtqueue *vq = dev->virtqueue[state->index];
+	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
 	/* We have to stop the queue (virtio) if it is running. */
 	if (dev->flags & VIRTIO_DEV_RUNNING) {
@@ -710,10 +712,11 @@ vhost_user_get_vring_base(struct virtio_net *dev,
 	dev->flags &= ~VIRTIO_DEV_READY;
 
 	/* Here we are safe to get the last used index */
-	state->num = vq->last_used_idx;
+	msg->payload.state.num = vq->last_used_idx;
 
 	RTE_LOG(INFO, VHOST_CONFIG,
-		"vring base idx:%d file:%d\n", state->index, state->num);
+		"vring base idx:%d file:%d\n", msg->payload.state.index,
+		msg->payload.state.num);
 	/*
 	 * Based on current qemu vhost-user implementation, this message is
 	 * sent and only sent in vhost_vring_stop.
@@ -738,18 +741,19 @@ vhost_user_get_vring_base(struct virtio_net *dev,
  */
 static int
 vhost_user_set_vring_enable(struct virtio_net *dev,
-			    struct vhost_vring_state *state)
+			    VhostUserMsg *msg)
 {
-	int enable = (int)state->num;
+	int enable = (int)msg->payload.state.num;
 
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"set queue enable: %d to qp idx: %d\n",
-		enable, state->index);
+		enable, msg->payload.state.index);
 
 	if (dev->notify_ops->vring_state_changed)
-		dev->notify_ops->vring_state_changed(dev->vid, state->index, enable);
+		dev->notify_ops->vring_state_changed(dev->vid,
+				msg->payload.state.index, enable);
 
-	dev->virtqueue[state->index]->enabled = enable;
+	dev->virtqueue[msg->payload.state.index]->enabled = enable;
 
 	return 0;
 }
@@ -1038,17 +1042,17 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
-		vhost_user_set_vring_num(dev, &msg.payload.state);
+		vhost_user_set_vring_num(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_ADDR:
-		vhost_user_set_vring_addr(dev, &msg.payload.addr);
+		vhost_user_set_vring_addr(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_BASE:
-		vhost_user_set_vring_base(dev, &msg.payload.state);
+		vhost_user_set_vring_base(dev, &msg);
 		break;
 
 	case VHOST_USER_GET_VRING_BASE:
-		vhost_user_get_vring_base(dev, &msg.payload.state);
+		vhost_user_get_vring_base(dev, &msg);
 		msg.size = sizeof(msg.payload.state);
 		send_vhost_message(fd, &msg);
 		break;
@@ -1073,7 +1077,7 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_ENABLE:
-		vhost_user_set_vring_enable(dev, &msg.payload.state);
+		vhost_user_set_vring_enable(dev, &msg);
 		break;
 	case VHOST_USER_SEND_RARP:
 		vhost_user_send_rarp(dev, &msg);
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

Re: [PATCH v3 3/3] vhost: access VhostUsrMsg via packed struct

[Yuanhan Liu]

On Thu, May 11, 2017 at 04:33:12PM +0200, Dariusz Stojaczyk wrote:
> From: Daniel Verkamp <daniel.verkamp@intel.com>
> 
> Fixes unaligned access to fields.

This is for fixing compile warnings with new clang 4.0? 

    http://dpdk.org/ml/archives/dev/2017-April/064089.html

If so, please show the exact warning in the commit log.

> 
> Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> ---
> Fixed checkpatch warnings

It's likely it will be easily missed while review. We normally do that:

---

v3: fix checkpatch warnings

v2: remove gerrit id

	--yliu

Re: [PATCH v3 3/3] vhost: access VhostUsrMsg via packed struct

[Stojaczyk, DariuszX]

> This is for fixing compile warnings with new clang 4.0?
> 
>     http://dpdk.org/ml/archives/dev/2017-April/064089.html
> 
> If so, please show the exact warning in the commit log.
> 

Everything compiles, but is undefined behavior.  Accessing packed struct's fields through pointers would have to be done as following:
e.g vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr __attribute__((aligned(1)))
Since the code above is unacceptable, this patch makes all functions take pointer to the parent struct (VhostUserMsg)

> >
> > Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
> > Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> > ---
> > Fixed checkpatch warnings
> 
> It's likely it will be easily missed while review. We normally do that:
> 
> ---
> 
> v3: fix checkpatch warnings
> 
> v2: remove gerrit id
> 
> 	--yliu

Thanks, I'll stick with it from now on

Re: [PATCH v3 3/3] vhost: access VhostUsrMsg via packed struct

[Yuanhan Liu]

On Wed, May 24, 2017 at 01:12:07PM +0000, Stojaczyk, DariuszX wrote:
> > This is for fixing compile warnings with new clang 4.0?
> > 
> >     http://dpdk.org/ml/archives/dev/2017-April/064089.html
> > 
> > If so, please show the exact warning in the commit log.
> > 
> 
> Everything compiles, but is undefined behavior.

Would you be a bit more informative about the "undefined behavior"? This
patch set (including this one) looks good to me. I just want the commit
log be more informative. Something like "Fixes unaligned access to fields"
is a bit too vague.

Thanks.

	--yliu

>  Accessing packed struct's fields through pointers would have to be done as following:
> e.g vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr __attribute__((aligned(1)))
> Since the code above is unacceptable, this patch makes all functions take pointer to the parent struct (VhostUserMsg)
> 
> > >
> > > Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
> > > Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> > > ---
> > > Fixed checkpatch warnings
> > 
> > It's likely it will be easily missed while review. We normally do that:
> > 
> > ---
> > 
> > v3: fix checkpatch warnings
> > 
> > v2: remove gerrit id
> > 
> > 	--yliu
> 
> Thanks, I'll stick with it from now on

Re: [PATCH v4 0/3] vhost: undefined behavior fixes

[Yuanhan Liu]

On Fri, May 26, 2017 at 01:59:12PM +0200, Dariusz Stojaczyk wrote:
> Fixes for memory-related undefined behavior issues in rte_vhost.
> 
> Daniel Verkamp (1):
>   vhost: access VhostUsrMsg via packed struct
> 
> Dariusz Stojaczyk (2):
>   vhost: fix malloc in rte_vhost_get_mem_table()
>   vhost: free guest_pages in vhost_backend_cleanup()

Applied to dpdk-next-virtio.

Thanks!

	--yliu
> 
>  lib/librte_vhost/vhost.c      |  2 +-
>  lib/librte_vhost/vhost_user.c | 64 ++++++++++++++++++++++++-------------------
>  2 files changed, 37 insertions(+), 29 deletions(-)
> 
> -- 
> 2.7.4

[PATCH v4 0/3] vhost: undefined behavior fixes

[Dariusz Stojaczyk]

Fixes for memory-related undefined behavior issues in rte_vhost.

Daniel Verkamp (1):
  vhost: access VhostUsrMsg via packed struct

Dariusz Stojaczyk (2):
  vhost: fix malloc in rte_vhost_get_mem_table()
  vhost: free guest_pages in vhost_backend_cleanup()

 lib/librte_vhost/vhost.c      |  2 +-
 lib/librte_vhost/vhost_user.c | 64 ++++++++++++++++++++++++-------------------
 2 files changed, 37 insertions(+), 29 deletions(-)

-- 
2.7.4

[PATCH v4 1/3] vhost: fix malloc in rte_vhost_get_mem_table()

[Dariusz Stojaczyk]

Amount of allocated memory was too small, causing buffer overflow.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
v2: remove gerrit id
 lib/librte_vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 0b19d2e..1f565fb 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -369,7 +369,7 @@ rte_vhost_get_mem_table(int vid, struct rte_vhost_memory **mem)
 		return -1;
 
 	size = dev->mem->nregions * sizeof(struct rte_vhost_mem_region);
-	m = malloc(size);
+	m = malloc(sizeof(struct rte_vhost_memory) + size);
 	if (!m)
 		return -1;
 
-- 
2.7.4

[PATCH v4 2/3] vhost: free guest_pages in vhost_backend_cleanup()

[Dariusz Stojaczyk]

This patch fixes a memory leak.
virtio_net::guest_pages is allocated in vhost_setup_mem_table(),
reallocated in add_one_guest_page(), but never freed.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
v2: remove gerrit id
 lib/librte_vhost/vhost_user.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 5c8058b..2cc0b66 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -114,6 +114,10 @@ vhost_backend_cleanup(struct virtio_net *dev)
 		rte_free(dev->mem);
 		dev->mem = NULL;
 	}
+
+	free(dev->guest_pages);
+	dev->guest_pages = NULL;
+
 	if (dev->log_addr) {
 		munmap((void *)(uintptr_t)dev->log_addr, dev->log_size);
 		dev->log_addr = 0;
-- 
2.7.4

[PATCH v4 3/3] vhost: access VhostUsrMsg via packed struct

[Dariusz Stojaczyk]

From: Daniel Verkamp <daniel.verkamp@intel.com>

Accessing fields of a packed struct through unaligned pointers is
undefined behavior. Instead of passing pointers to particular fields,
a pointer to the root struct should be used. This patch does exactly
that.

Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
v4: updated commit message
v3: fix checkpatch warnings
v2: remove gerrit id
 lib/librte_vhost/vhost_user.c | 60 +++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 2cc0b66..ab2f40a 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -197,11 +197,11 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
  */
 static int
 vhost_user_set_vring_num(struct virtio_net *dev,
-			 struct vhost_vring_state *state)
+			 VhostUserMsg *msg)
 {
-	struct vhost_virtqueue *vq = dev->virtqueue[state->index];
+	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
-	vq->size = state->num;
+	vq->size = msg->payload.state.num;
 
 	if (dev->dequeue_zero_copy) {
 		vq->nr_zmbuf = 0;
@@ -334,7 +334,7 @@ qva_to_vva(struct virtio_net *dev, uint64_t qva)
  * This function then converts these to our address space.
  */
 static int
-vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
+vhost_user_set_vring_addr(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	struct vhost_virtqueue *vq;
 
@@ -342,11 +342,11 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		return -1;
 
 	/* addr->index refers to the queue index. The txq 1, rxq is 0. */
-	vq = dev->virtqueue[addr->index];
+	vq = dev->virtqueue[msg->payload.addr.index];
 
 	/* The addresses are converted from QEMU virtual to Vhost virtual. */
 	vq->desc = (struct vring_desc *)(uintptr_t)qva_to_vva(dev,
-			addr->desc_user_addr);
+			msg->payload.addr.desc_user_addr);
 	if (vq->desc == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find desc ring address.\n",
@@ -354,11 +354,11 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		return -1;
 	}
 
-	dev = numa_realloc(dev, addr->index);
-	vq = dev->virtqueue[addr->index];
+	dev = numa_realloc(dev, msg->payload.addr.index);
+	vq = dev->virtqueue[msg->payload.addr.index];
 
 	vq->avail = (struct vring_avail *)(uintptr_t)qva_to_vva(dev,
-			addr->avail_user_addr);
+			msg->payload.addr.avail_user_addr);
 	if (vq->avail == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find avail ring address.\n",
@@ -367,7 +367,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 	}
 
 	vq->used = (struct vring_used *)(uintptr_t)qva_to_vva(dev,
-			addr->used_user_addr);
+			msg->payload.addr.used_user_addr);
 	if (vq->used == 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to find used ring address.\n",
@@ -384,7 +384,7 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
 		vq->last_avail_idx = vq->used->idx;
 	}
 
-	vq->log_guest_addr = addr->log_guest_addr;
+	vq->log_guest_addr = msg->payload.addr.log_guest_addr;
 
 	LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n",
 			dev->vid, vq->desc);
@@ -403,10 +403,12 @@ vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr *addr)
  */
 static int
 vhost_user_set_vring_base(struct virtio_net *dev,
-			  struct vhost_vring_state *state)
+			  VhostUserMsg *msg)
 {
-	dev->virtqueue[state->index]->last_used_idx  = state->num;
-	dev->virtqueue[state->index]->last_avail_idx = state->num;
+	dev->virtqueue[msg->payload.state.index]->last_used_idx  =
+			msg->payload.state.num;
+	dev->virtqueue[msg->payload.state.index]->last_avail_idx =
+			msg->payload.state.num;
 
 	return 0;
 }
@@ -697,9 +699,9 @@ free_zmbufs(struct vhost_virtqueue *vq)
  */
 static int
 vhost_user_get_vring_base(struct virtio_net *dev,
-			  struct vhost_vring_state *state)
+			  VhostUserMsg *msg)
 {
-	struct vhost_virtqueue *vq = dev->virtqueue[state->index];
+	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
 	/* We have to stop the queue (virtio) if it is running. */
 	if (dev->flags & VIRTIO_DEV_RUNNING) {
@@ -710,10 +712,11 @@ vhost_user_get_vring_base(struct virtio_net *dev,
 	dev->flags &= ~VIRTIO_DEV_READY;
 
 	/* Here we are safe to get the last used index */
-	state->num = vq->last_used_idx;
+	msg->payload.state.num = vq->last_used_idx;
 
 	RTE_LOG(INFO, VHOST_CONFIG,
-		"vring base idx:%d file:%d\n", state->index, state->num);
+		"vring base idx:%d file:%d\n", msg->payload.state.index,
+		msg->payload.state.num);
 	/*
 	 * Based on current qemu vhost-user implementation, this message is
 	 * sent and only sent in vhost_vring_stop.
@@ -738,18 +741,19 @@ vhost_user_get_vring_base(struct virtio_net *dev,
  */
 static int
 vhost_user_set_vring_enable(struct virtio_net *dev,
-			    struct vhost_vring_state *state)
+			    VhostUserMsg *msg)
 {
-	int enable = (int)state->num;
+	int enable = (int)msg->payload.state.num;
 
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"set queue enable: %d to qp idx: %d\n",
-		enable, state->index);
+		enable, msg->payload.state.index);
 
 	if (dev->notify_ops->vring_state_changed)
-		dev->notify_ops->vring_state_changed(dev->vid, state->index, enable);
+		dev->notify_ops->vring_state_changed(dev->vid,
+				msg->payload.state.index, enable);
 
-	dev->virtqueue[state->index]->enabled = enable;
+	dev->virtqueue[msg->payload.state.index]->enabled = enable;
 
 	return 0;
 }
@@ -1038,17 +1042,17 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
-		vhost_user_set_vring_num(dev, &msg.payload.state);
+		vhost_user_set_vring_num(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_ADDR:
-		vhost_user_set_vring_addr(dev, &msg.payload.addr);
+		vhost_user_set_vring_addr(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_BASE:
-		vhost_user_set_vring_base(dev, &msg.payload.state);
+		vhost_user_set_vring_base(dev, &msg);
 		break;
 
 	case VHOST_USER_GET_VRING_BASE:
-		vhost_user_get_vring_base(dev, &msg.payload.state);
+		vhost_user_get_vring_base(dev, &msg);
 		msg.size = sizeof(msg.payload.state);
 		send_vhost_message(fd, &msg);
 		break;
@@ -1073,7 +1077,7 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_ENABLE:
-		vhost_user_set_vring_enable(dev, &msg.payload.state);
+		vhost_user_set_vring_enable(dev, &msg);
 		break;
 	case VHOST_USER_SEND_RARP:
 		vhost_user_send_rarp(dev, &msg);
-- 
2.7.4