diff mbox series

[ovs-dev] netdev-dpdk: fix incorrect shinfo initialization

Message ID 20201014072248.374384-1-yang_y_yi@163.com
State New
Headers show
Series [ovs-dev] netdev-dpdk: fix incorrect shinfo initialization | expand

Commit Message

yang_y_yi Oct. 14, 2020, 7:22 a.m. UTC
From: Yi Yang <yangyi01@inspur.com>

shinfo is used to store reference counter and free callback
of an external buffer, but it is stored in mbuf if the mbuf
has tailroom for it.

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

  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 shinfo at the tail of external buffer.

Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support")
Co-authored-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Yi Yang <yangyi01@inspur.com>
---
 lib/netdev-dpdk.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

Comments

Flavio Leitner Oct. 27, 2020, 11:34 a.m. UTC | #1
On Wed, Oct 14, 2020 at 03:22:48PM +0800, yang_y_yi@163.com wrote:
> From: Yi Yang <yangyi01@inspur.com>
> 
> shinfo is used to store reference counter and free callback
> of an external buffer, but it is stored in mbuf if the mbuf
> has tailroom for it.
> 
> This is wrong because the mbuf (and its data) can be freed
> before the external buffer, for example:
> 
>   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 shinfo at the tail of external buffer.
> 
> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support")
> Co-authored-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Yi Yang <yangyi01@inspur.com>
> ---

Acked-by: Flavio Leitner <fbl@sysclose.org>

Thanks Yi,
fbl
Ilya Maximets Oct. 27, 2020, 12:47 p.m. UTC | #2
On 10/27/20 12:34 PM, Flavio Leitner wrote:
> On Wed, Oct 14, 2020 at 03:22:48PM +0800, yang_y_yi@163.com wrote:
>> From: Yi Yang <yangyi01@inspur.com>
>>
>> shinfo is used to store reference counter and free callback
>> of an external buffer, but it is stored in mbuf if the mbuf
>> has tailroom for it.
>>
>> This is wrong because the mbuf (and its data) can be freed
>> before the external buffer, for example:
>>
>>   pkt2 = rte_pktmbuf_alloc(mp);
>>   rte_pktmbuf_attach(pkt2, pkt);
>>   rte_pktmbuf_free(pkt);

How is that possible with OVS?  Right now OVS doesn't support multi-segement
mbufs and will, likely, not support them in a near future because it requires
changes all other the codebase.

Is there any other scenario that could lead to issues with current OVS
implementation?

>>
>> After this, pkt is freed, but it still contains shinfo, which
>> is referenced by pkt2.
>>
>> Fix this by always storing shinfo at the tail of external buffer.
>>
>> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support")
>> Co-authored-by: Olivier Matz <olivier.matz@6wind.com>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> Signed-off-by: Yi Yang <yangyi01@inspur.com>
>> ---
> 
> Acked-by: Flavio Leitner <fbl@sysclose.org>
> 
> Thanks Yi,
> fbl
>
Flavio Leitner Oct. 27, 2020, 1:08 p.m. UTC | #3
On Tue, Oct 27, 2020 at 01:47:22PM +0100, Ilya Maximets wrote:
> On 10/27/20 12:34 PM, Flavio Leitner wrote:
> > On Wed, Oct 14, 2020 at 03:22:48PM +0800, yang_y_yi@163.com wrote:
> >> From: Yi Yang <yangyi01@inspur.com>
> >>
> >> shinfo is used to store reference counter and free callback
> >> of an external buffer, but it is stored in mbuf if the mbuf
> >> has tailroom for it.
> >>
> >> This is wrong because the mbuf (and its data) can be freed
> >> before the external buffer, for example:
> >>
> >>   pkt2 = rte_pktmbuf_alloc(mp);
> >>   rte_pktmbuf_attach(pkt2, pkt);
> >>   rte_pktmbuf_free(pkt);
> 
> How is that possible with OVS?  Right now OVS doesn't support multi-segement
> mbufs and will, likely, not support them in a near future because it requires
> changes all other the codebase.
> 
> Is there any other scenario that could lead to issues with current OVS
> implementation?

This is copying packets. The shinfo is allocated in the mbuf of
the first packet which could be deleted without any references
to the external buffer still using it.

fbl


> 
> >>
> >> After this, pkt is freed, but it still contains shinfo, which
> >> is referenced by pkt2.
> >>
> >> Fix this by always storing shinfo at the tail of external buffer.
> >>
> >> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support")
> >> Co-authored-by: Olivier Matz <olivier.matz@6wind.com>
> >> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >> Signed-off-by: Yi Yang <yangyi01@inspur.com>
> >> ---
> > 
> > Acked-by: Flavio Leitner <fbl@sysclose.org>
> > 
> > Thanks Yi,
> > fbl
> > 
>
Yi Yang (杨燚)-云服务集团 Oct. 28, 2020, 12:35 a.m. UTC | #4
-----邮件原件-----
发件人: dev [mailto:ovs-dev-bounces@openvswitch.org] 代表 Flavio Leitner
发送时间: 2020年10月27日 21:08
收件人: Ilya Maximets <i.maximets@ovn.org>
抄送: yang_y_yi@163.com; ovs-dev@openvswitch.org; olivier.matz@6wind.com
主题: Re: [ovs-dev] [PATCH] netdev-dpdk: fix incorrect shinfo initialization

On Tue, Oct 27, 2020 at 01:47:22PM +0100, Ilya Maximets wrote:
> On 10/27/20 12:34 PM, Flavio Leitner wrote:
> > On Wed, Oct 14, 2020 at 03:22:48PM +0800, yang_y_yi@163.com wrote:
> >> From: Yi Yang <yangyi01@inspur.com>
> >>
> >> shinfo is used to store reference counter and free callback of an 
> >> external buffer, but it is stored in mbuf if the mbuf has tailroom 
> >> for it.
> >>
> >> This is wrong because the mbuf (and its data) can be freed before 
> >> the external buffer, for example:
> >>
> >>   pkt2 = rte_pktmbuf_alloc(mp);
> >>   rte_pktmbuf_attach(pkt2, pkt);
> >>   rte_pktmbuf_free(pkt);
> 
> How is that possible with OVS?  Right now OVS doesn't support 
> multi-segement mbufs and will, likely, not support them in a near 
> future because it requires changes all other the codebase.
> 
> Is there any other scenario that could lead to issues with current OVS 
> implementation?

This is copying packets. The shinfo is allocated in the mbuf of the first packet which could be deleted without any references to the external buffer still using it.

Fbl

[Yi Yang]  Yes, this is not related with multi-segment mbuf, dpdk interfaces to system interfaces communication will use it if the packet size is greater than mtu size, i.e. TSO case from veth/tap to dpdk/vhost and backward will use it, this is a wrong use of shinfo, the same fix (which is used by virtio/vhost driver)has been merged into dpdk branch.
 
[Yi Yang] 



> 
> >>
> >> After this, pkt is freed, but it still contains shinfo, which is 
> >> referenced by pkt2.
> >>
> >> Fix this by always storing shinfo at the tail of external buffer.
> >>
> >> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload 
> >> support")
> >> Co-authored-by: Olivier Matz <olivier.matz@6wind.com>
> >> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >> Signed-off-by: Yi Yang <yangyi01@inspur.com>
> >> ---
> > 
> > Acked-by: Flavio Leitner <fbl@sysclose.org>
> > 
> > Thanks Yi,
> > fbl
> > 
> 

--
fbl
diff mbox series

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 0b830be..c7f9326 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2654,12 +2654,8 @@  dpdk_pktmbuf_attach_extbuf(struct rte_mbuf *pkt, uint32_t data_len)
     uint16_t buf_len;
     void *buf;
 
-    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 (OVS_UNLIKELY(total_len > UINT16_MAX)) {
         VLOG_ERR("Can't copy packet: too big %u", total_len);
@@ -2674,20 +2670,14 @@  dpdk_pktmbuf_attach_extbuf(struct rte_mbuf *pkt, uint32_t data_len)
     }
 
     /* Initialize shinfo. */
-    if (shinfo) {
-        shinfo->free_cb = netdev_dpdk_extbuf_free;
-        shinfo->fcb_opaque = buf;
-        rte_mbuf_ext_refcnt_set(shinfo, 1);
-    } else {
-        shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
-                                                    netdev_dpdk_extbuf_free,
-                                                    buf);
-        if (OVS_UNLIKELY(shinfo == NULL)) {
-            rte_free(buf);
-            VLOG_ERR("Failed to initialize shared info for mbuf while "
-                     "attempting to attach an external buffer.");
-            return NULL;
-        }
+    shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
+                                                netdev_dpdk_extbuf_free,
+                                                buf);
+    if (OVS_UNLIKELY(shinfo == NULL)) {
+        rte_free(buf);
+        VLOG_ERR("Failed to initialize shared info for mbuf while "
+                 "attempting to attach an external buffer.");
+        return NULL;
     }
 
     rte_pktmbuf_attach_extbuf(pkt, buf, rte_malloc_virt2iova(buf), buf_len,