DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] vhost: fix external mbuf creation
@ 2020-10-07 12:53 Olivier Matz
  2020-10-09  7:15 ` Maxime Coquelin
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Olivier Matz @ 2020-10-07 12:53 UTC (permalink / raw)
  To: dev
  Cc: Maxime Coquelin, Chenbo Xia, Zhihong Wang, Flavio Leitner,
	yang_y_yi, stable

In virtio_dev_extbuf_alloc(), the shinfo structure used to store
the reference counter and the free callback of the external buffer
is by default stored inside the mbuf data.

This is wrong because the mbuf (and its data) can be freed before
the external buffer, for instance in the following situation:

  pkt2 = rte_pktmbuf_alloc(mp);
  rte_pktmbuf_attach(pkt2, pkt);
  rte_pktmbuf_free(pkt);

After this, pkt is freed, but it still contains shinfo, which is
referenced by pkt2.

Fix this by always storing the shinfo beside the external buffer.

Fixes: c3ff0ac70acb ("vhost: improve performance by supporting large buffer")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---

Hi,

I found this issue by code review while discussing about this
patchset [1]. I did not tested the patch, but as I'm only removing
one path among the two, I don't expect that it breaks anything.

[1] http://inbox.dpdk.org/dev/20200730120900.108232-1-yang_y_yi@163.com/

Regards,
Olivier

 lib/librte_vhost/virtio_net.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 0a0bea1a5a..008f5ceb04 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -2098,16 +2098,8 @@ virtio_dev_extbuf_alloc(struct rte_mbuf *pkt, uint32_t size)
 	rte_iova_t iova;
 	void *buf;
 
-	/* Try to use pkt buffer to store shinfo to reduce the amount of memory
-	 * required, otherwise store shinfo in the new buffer.
-	 */
-	if (rte_pktmbuf_tailroom(pkt) >= sizeof(*shinfo))
-		shinfo = rte_pktmbuf_mtod(pkt,
-					  struct rte_mbuf_ext_shared_info *);
-	else {
-		total_len += sizeof(*shinfo) + sizeof(uintptr_t);
-		total_len = RTE_ALIGN_CEIL(total_len, sizeof(uintptr_t));
-	}
+	total_len += sizeof(*shinfo) + sizeof(uintptr_t);
+	total_len = RTE_ALIGN_CEIL(total_len, sizeof(uintptr_t));
 
 	if (unlikely(total_len > UINT16_MAX))
 		return -ENOSPC;
@@ -2118,18 +2110,12 @@ virtio_dev_extbuf_alloc(struct rte_mbuf *pkt, uint32_t size)
 		return -ENOMEM;
 
 	/* Initialize shinfo */
-	if (shinfo) {
-		shinfo->free_cb = virtio_dev_extbuf_free;
-		shinfo->fcb_opaque = buf;
-		rte_mbuf_ext_refcnt_set(shinfo, 1);
-	} else {
-		shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
-					      virtio_dev_extbuf_free, buf);
-		if (unlikely(shinfo == NULL)) {
-			rte_free(buf);
-			VHOST_LOG_DATA(ERR, "Failed to init shinfo\n");
-			return -1;
-		}
+	shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
+						virtio_dev_extbuf_free, buf);
+	if (unlikely(shinfo == NULL)) {
+		rte_free(buf);
+		VHOST_LOG_DATA(ERR, "Failed to init shinfo\n");
+		return -1;
 	}
 
 	iova = rte_malloc_virt2iova(buf);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: fix external mbuf creation
  2020-10-07 12:53 [dpdk-dev] [PATCH] vhost: fix external mbuf creation Olivier Matz
@ 2020-10-09  7:15 ` Maxime Coquelin
  2020-10-09  7:24 ` Maxime Coquelin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Maxime Coquelin @ 2020-10-09  7:15 UTC (permalink / raw)
  To: Olivier Matz, dev
  Cc: Chenbo Xia, Zhihong Wang, Flavio Leitner, yang_y_yi, stable



On 10/7/20 2:53 PM, Olivier Matz wrote:
> In virtio_dev_extbuf_alloc(), the shinfo structure used to store
> the reference counter and the free callback of the external buffer
> is by default stored inside the mbuf data.
> 
> This is wrong because the mbuf (and its data) can be freed before
> the external buffer, for instance in the following situation:
> 
>   pkt2 = rte_pktmbuf_alloc(mp);
>   rte_pktmbuf_attach(pkt2, pkt);
>   rte_pktmbuf_free(pkt);
> 
> After this, pkt is freed, but it still contains shinfo, which is
> referenced by pkt2.
> 
> Fix this by always storing the shinfo beside the external buffer.
> 
> Fixes: c3ff0ac70acb ("vhost: improve performance by supporting large buffer")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> 
> Hi,
> 
> I found this issue by code review while discussing about this
> patchset [1]. I did not tested the patch, but as I'm only removing
> one path among the two, I don't expect that it breaks anything.
> 
> [1] http://inbox.dpdk.org/dev/20200730120900.108232-1-yang_y_yi@163.com/
> 
> Regards,
> Olivier
> 
>  lib/librte_vhost/virtio_net.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: fix external mbuf creation
  2020-10-07 12:53 [dpdk-dev] [PATCH] vhost: fix external mbuf creation Olivier Matz
  2020-10-09  7:15 ` Maxime Coquelin
@ 2020-10-09  7:24 ` Maxime Coquelin
  2020-10-09  8:47 ` yang_y_yi
  2020-10-09  9:04 ` yang_y_yi
  3 siblings, 0 replies; 5+ messages in thread
From: Maxime Coquelin @ 2020-10-09  7:24 UTC (permalink / raw)
  To: Olivier Matz, dev
  Cc: Chenbo Xia, Zhihong Wang, Flavio Leitner, yang_y_yi, stable



On 10/7/20 2:53 PM, Olivier Matz wrote:
> In virtio_dev_extbuf_alloc(), the shinfo structure used to store
> the reference counter and the free callback of the external buffer
> is by default stored inside the mbuf data.
> 
> This is wrong because the mbuf (and its data) can be freed before
> the external buffer, for instance in the following situation:
> 
>   pkt2 = rte_pktmbuf_alloc(mp);
>   rte_pktmbuf_attach(pkt2, pkt);
>   rte_pktmbuf_free(pkt);
> 
> After this, pkt is freed, but it still contains shinfo, which is
> referenced by pkt2.
> 
> Fix this by always storing the shinfo beside the external buffer.
> 
> Fixes: c3ff0ac70acb ("vhost: improve performance by supporting large buffer")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> 
> Hi,
> 
> I found this issue by code review while discussing about this
> patchset [1]. I did not tested the patch, but as I'm only removing
> one path among the two, I don't expect that it breaks anything.
> 
> [1] http://inbox.dpdk.org/dev/20200730120900.108232-1-yang_y_yi@163.com/
> 
> Regards,
> Olivier
> 
>  lib/librte_vhost/virtio_net.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)

Applied to dpdk-next-virtio/main.

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: fix external mbuf creation
  2020-10-07 12:53 [dpdk-dev] [PATCH] vhost: fix external mbuf creation Olivier Matz
  2020-10-09  7:15 ` Maxime Coquelin
  2020-10-09  7:24 ` Maxime Coquelin
@ 2020-10-09  8:47 ` yang_y_yi
  2020-10-09  9:04 ` yang_y_yi
  3 siblings, 0 replies; 5+ messages in thread
From: yang_y_yi @ 2020-10-09  8:47 UTC (permalink / raw)
  To: Olivier Matz
  Cc: dev, Maxime Coquelin, Chenbo Xia, Zhihong Wang, Flavio Leitner, stable

Tested-by: Yi Yang <yangyi01@inspur.com>





















At 2020-10-07 20:53:18, "Olivier Matz" <olivier.matz@6wind.com> wrote:
>In virtio_dev_extbuf_alloc(), the shinfo structure used to store
>the reference counter and the free callback of the external buffer
>is by default stored inside the mbuf data.
>
>This is wrong because the mbuf (and its data) can be freed before
>the external buffer, for instance in the following situation:
>
>  pkt2 = rte_pktmbuf_alloc(mp);
>  rte_pktmbuf_attach(pkt2, pkt);
>  rte_pktmbuf_free(pkt);
>
>After this, pkt is freed, but it still contains shinfo, which is
>referenced by pkt2.
>
>Fix this by always storing the shinfo beside the external buffer.
>
>Fixes: c3ff0ac70acb ("vhost: improve performance by supporting large buffer")
>Cc: stable@dpdk.org
>
>Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>---
>
>Hi,
>
>I found this issue by code review while discussing about this
>patchset [1]. I did not tested the patch, but as I'm only removing
>one path among the two, I don't expect that it breaks anything.
>
>[1] http://inbox.dpdk.org/dev/20200730120900.108232-1-yang_y_yi@163.com/
>
>Regards,
>Olivier
>
> lib/librte_vhost/virtio_net.c | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
>diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
>index 0a0bea1a5a..008f5ceb04 100644
>--- a/lib/librte_vhost/virtio_net.c
>+++ b/lib/librte_vhost/virtio_net.c
>@@ -2098,16 +2098,8 @@ virtio_dev_extbuf_alloc(struct rte_mbuf *pkt, uint32_t size)
> 	rte_iova_t iova;
> 	void *buf;
> 
>-	/* Try to use pkt buffer to store shinfo to reduce the amount of memory
>-	 * required, otherwise store shinfo in the new buffer.
>-	 */
>-	if (rte_pktmbuf_tailroom(pkt) >= sizeof(*shinfo))
>-		shinfo = rte_pktmbuf_mtod(pkt,
>-					  struct rte_mbuf_ext_shared_info *);
>-	else {
>-		total_len += sizeof(*shinfo) + sizeof(uintptr_t);
>-		total_len = RTE_ALIGN_CEIL(total_len, sizeof(uintptr_t));
>-	}
>+	total_len += sizeof(*shinfo) + sizeof(uintptr_t);
>+	total_len = RTE_ALIGN_CEIL(total_len, sizeof(uintptr_t));
> 
> 	if (unlikely(total_len > UINT16_MAX))
> 		return -ENOSPC;
>@@ -2118,18 +2110,12 @@ virtio_dev_extbuf_alloc(struct rte_mbuf *pkt, uint32_t size)
> 		return -ENOMEM;
> 
> 	/* Initialize shinfo */
>-	if (shinfo) {
>-		shinfo->free_cb = virtio_dev_extbuf_free;
>-		shinfo->fcb_opaque = buf;
>-		rte_mbuf_ext_refcnt_set(shinfo, 1);
>-	} else {
>-		shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
>-					      virtio_dev_extbuf_free, buf);
>-		if (unlikely(shinfo == NULL)) {
>-			rte_free(buf);
>-			VHOST_LOG_DATA(ERR, "Failed to init shinfo\n");
>-			return -1;
>-		}
>+	shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
>+						virtio_dev_extbuf_free, buf);
>+	if (unlikely(shinfo == NULL)) {
>+		rte_free(buf);
>+		VHOST_LOG_DATA(ERR, "Failed to init shinfo\n");
>+		return -1;
> 	}
> 
> 	iova = rte_malloc_virt2iova(buf);
>-- 
>2.25.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: fix external mbuf creation
  2020-10-07 12:53 [dpdk-dev] [PATCH] vhost: fix external mbuf creation Olivier Matz
                   ` (2 preceding siblings ...)
  2020-10-09  8:47 ` yang_y_yi
@ 2020-10-09  9:04 ` yang_y_yi
  3 siblings, 0 replies; 5+ messages in thread
From: yang_y_yi @ 2020-10-09  9:04 UTC (permalink / raw)
  To: Olivier Matz
  Cc: dev, Maxime Coquelin, Chenbo Xia, Zhihong Wang, Flavio Leitner, stable

Thanks Oliver for posting this patch, it clearly explain his concern, I totally agree this change. I tested it by OVS DPDK, it works well.

At 2020-10-07 20:53:18, "Olivier Matz" <olivier.matz@6wind.com> wrote:
>In virtio_dev_extbuf_alloc(), the shinfo structure used to store
>the reference counter and the free callback of the external buffer
>is by default stored inside the mbuf data.
>
>This is wrong because the mbuf (and its data) can be freed before
>the external buffer, for instance in the following situation:
>
>  pkt2 = rte_pktmbuf_alloc(mp);
>  rte_pktmbuf_attach(pkt2, pkt);
>  rte_pktmbuf_free(pkt);
>
>After this, pkt is freed, but it still contains shinfo, which is
>referenced by pkt2.
>
>Fix this by always storing the shinfo beside the external buffer.
>
>Fixes: c3ff0ac70acb ("vhost: improve performance by supporting large buffer")
>Cc: stable@dpdk.org
>
>Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>---
>
>Hi,
>
>I found this issue by code review while discussing about this
>patchset [1]. I did not tested the patch, but as I'm only removing
>one path among the two, I don't expect that it breaks anything.
>
>[1] http://inbox.dpdk.org/dev/20200730120900.108232-1-yang_y_yi@163.com/
>
>Regards,
>Olivier
>
> lib/librte_vhost/virtio_net.c | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
>diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
>index 0a0bea1a5a..008f5ceb04 100644
>--- a/lib/librte_vhost/virtio_net.c
>+++ b/lib/librte_vhost/virtio_net.c
>@@ -2098,16 +2098,8 @@ virtio_dev_extbuf_alloc(struct rte_mbuf *pkt, uint32_t size)
> 	rte_iova_t iova;
> 	void *buf;
> 
>-	/* Try to use pkt buffer to store shinfo to reduce the amount of memory
>-	 * required, otherwise store shinfo in the new buffer.
>-	 */
>-	if (rte_pktmbuf_tailroom(pkt) >= sizeof(*shinfo))
>-		shinfo = rte_pktmbuf_mtod(pkt,
>-					  struct rte_mbuf_ext_shared_info *);
>-	else {
>-		total_len += sizeof(*shinfo) + sizeof(uintptr_t);
>-		total_len = RTE_ALIGN_CEIL(total_len, sizeof(uintptr_t));
>-	}
>+	total_len += sizeof(*shinfo) + sizeof(uintptr_t);
>+	total_len = RTE_ALIGN_CEIL(total_len, sizeof(uintptr_t));
> 
> 	if (unlikely(total_len > UINT16_MAX))
> 		return -ENOSPC;
>@@ -2118,18 +2110,12 @@ virtio_dev_extbuf_alloc(struct rte_mbuf *pkt, uint32_t size)
> 		return -ENOMEM;
> 
> 	/* Initialize shinfo */
>-	if (shinfo) {
>-		shinfo->free_cb = virtio_dev_extbuf_free;
>-		shinfo->fcb_opaque = buf;
>-		rte_mbuf_ext_refcnt_set(shinfo, 1);
>-	} else {
>-		shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
>-					      virtio_dev_extbuf_free, buf);
>-		if (unlikely(shinfo == NULL)) {
>-			rte_free(buf);
>-			VHOST_LOG_DATA(ERR, "Failed to init shinfo\n");
>-			return -1;
>-		}
>+	shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
>+						virtio_dev_extbuf_free, buf);
>+	if (unlikely(shinfo == NULL)) {
>+		rte_free(buf);
>+		VHOST_LOG_DATA(ERR, "Failed to init shinfo\n");
>+		return -1;
> 	}
> 
> 	iova = rte_malloc_virt2iova(buf);
>-- 
>2.25.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-10-09  9:04 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 12:53 [dpdk-dev] [PATCH] vhost: fix external mbuf creation Olivier Matz
2020-10-09  7:15 ` Maxime Coquelin
2020-10-09  7:24 ` Maxime Coquelin
2020-10-09  8:47 ` yang_y_yi
2020-10-09  9:04 ` yang_y_yi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).