All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] vhost: fix avail idx update error when desc copy failed
@ 2022-06-22  0:56 Gaoxiang Liu
  2022-06-22  1:19 ` [PATCH v2] " Gaoxiang Liu
  0 siblings, 1 reply; 7+ messages in thread
From: Gaoxiang Liu @ 2022-06-22  0:56 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia; +Cc: dev, liugaoxiang, Gaoxiang Liu, stable

When copy_desc_to_mbuf function failed, i added 1.
And last_avail_idx added i, other than i - 1.
It may cause that the first mbuf in mbuf-list is dropped,
the second mbuf in mbuf-list is received in the following
rx procedure.
And The pkt_len of the second mbuf is zero, resulting in
segment fault when parsing the mbuf.

Fixes: 0fd5608ef97f ("vhost: handle mbuf allocation failure")
Cc: stable@dpdk.org

Signed-off-by: Gaoxiang Liu <liugaoxiang@huawei.com>
---
 lib/vhost/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 68a26eb17d..86f1c04b43 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2850,7 +2850,7 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	if (dropped)
 		rte_pktmbuf_free_bulk(&pkts[i - 1], count - i + 1);
 
-	vq->last_avail_idx += i;
+	vq->last_avail_idx += i - 1;
 
 	do_data_copy_dequeue(vq);
 	if (unlikely(i < count))
-- 
2.32.0


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

* [PATCH v2] vhost: fix avail idx update error when desc copy failed
  2022-06-22  0:56 [PATCH] vhost: fix avail idx update error when desc copy failed Gaoxiang Liu
@ 2022-06-22  1:19 ` Gaoxiang Liu
  2022-06-23  3:07   ` Xia, Chenbo
  2022-07-01 13:05   ` Xia, Chenbo
  0 siblings, 2 replies; 7+ messages in thread
From: Gaoxiang Liu @ 2022-06-22  1:19 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia; +Cc: dev, liugaoxiang, Gaoxiang Liu, stable

When copy_desc_to_mbuf function failed, i added 1.
And last_avail_idx added i, other than i - 1.
It may cause that the first mbuf in mbuf-list is dropped,
the second mbuf in mbuf-list is received in the following
rx procedure.
And The pkt_len of the second mbuf is zero, resulting in
segment fault when parsing the mbuf.

Fixes: 0fd5608ef97f ("vhost: handle mbuf allocation failure")
Cc: stable@dpdk.org

Signed-off-by: Gaoxiang Liu <liugaoxiang@huawei.com>

---
v2:
* Fixed other idx update errors.
---
 lib/vhost/virtio_net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 68a26eb17d..eb254e1024 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2850,11 +2850,11 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	if (dropped)
 		rte_pktmbuf_free_bulk(&pkts[i - 1], count - i + 1);
 
-	vq->last_avail_idx += i;
+	vq->last_avail_idx += i - dropped;
 
 	do_data_copy_dequeue(vq);
-	if (unlikely(i < count))
-		vq->shadow_used_idx = i;
+	if (unlikely((i - dropped) < count))
+		vq->shadow_used_idx = i - dropped;
 	if (likely(vq->shadow_used_idx)) {
 		flush_shadow_used_ring_split(dev, vq);
 		vhost_vring_call_split(dev, vq);
-- 
2.32.0


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

* RE: [PATCH v2] vhost: fix avail idx update error when desc copy failed
  2022-06-22  1:19 ` [PATCH v2] " Gaoxiang Liu
@ 2022-06-23  3:07   ` Xia, Chenbo
  2022-06-23  6:23     ` Gaoxiang Liu
  2022-07-01 13:05   ` Xia, Chenbo
  1 sibling, 1 reply; 7+ messages in thread
From: Xia, Chenbo @ 2022-06-23  3:07 UTC (permalink / raw)
  To: Gaoxiang Liu, maxime.coquelin; +Cc: dev, liugaoxiang, stable

Hi Gaoxiang,

> -----Original Message-----
> From: Gaoxiang Liu <gaoxiangliu0@163.com>
> Sent: Wednesday, June 22, 2022 9:20 AM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; liugaoxiang@huawei.com; Gaoxiang Liu
> <gaoxiangliu0@163.com>; stable@dpdk.org
> Subject: [PATCH v2] vhost: fix avail idx update error when desc copy
> failed
> 
> When copy_desc_to_mbuf function failed, i added 1.
> And last_avail_idx added i, other than i - 1.
> It may cause that the first mbuf in mbuf-list is dropped,
> the second mbuf in mbuf-list is received in the following
> rx procedure.
> And The pkt_len of the second mbuf is zero, resulting in
> segment fault when parsing the mbuf.

Guess this one is the latest? I believe something is wrong when you forward this,
it generates another patch because of your mail forwarding:

http://patchwork.dpdk.org/project/dpdk/list/?series=&submitter=&state=&q=&archive=&delegate=2642

/Chenbo

> 
> Fixes: 0fd5608ef97f ("vhost: handle mbuf allocation failure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Gaoxiang Liu <liugaoxiang@huawei.com>
> 
> ---
> v2:
> * Fixed other idx update errors.
> ---
>  lib/vhost/virtio_net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 68a26eb17d..eb254e1024 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2850,11 +2850,11 @@ virtio_dev_tx_split(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
>  	if (dropped)
>  		rte_pktmbuf_free_bulk(&pkts[i - 1], count - i + 1);
> 
> -	vq->last_avail_idx += i;
> +	vq->last_avail_idx += i - dropped;
> 
>  	do_data_copy_dequeue(vq);
> -	if (unlikely(i < count))
> -		vq->shadow_used_idx = i;
> +	if (unlikely((i - dropped) < count))
> +		vq->shadow_used_idx = i - dropped;
>  	if (likely(vq->shadow_used_idx)) {
>  		flush_shadow_used_ring_split(dev, vq);
>  		vhost_vring_call_split(dev, vq);
> --
> 2.32.0


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

* Re: [PATCH v2] vhost: fix avail idx update error when desc copy failed
  2022-06-23  3:07   ` Xia, Chenbo
@ 2022-06-23  6:23     ` Gaoxiang Liu
  0 siblings, 0 replies; 7+ messages in thread
From: Gaoxiang Liu @ 2022-06-23  6:23 UTC (permalink / raw)
  To: Xia, Chenbo; +Cc: maxime.coquelin, dev, liugaoxiang, stable

[-- Attachment #1: Type: text/plain, Size: 2465 bytes --]

Hi, Chenbo
 Yes. It is the latest patch.



---- Replied Message ----
| From | Xia, Chenbo<chenbo.xia@intel.com> |
| Date | 06/23/2022 11:07 |
| To | Gaoxiang Liu<gaoxiangliu0@163.com>,
maxime.coquelin@redhat.com<maxime.coquelin@redhat.com> |
| Cc | dev@dpdk.org<dev@dpdk.org>,
liugaoxiang@huawei.com<liugaoxiang@huawei.com>,
stable@dpdk.org<stable@dpdk.org> |
| Subject | RE: [PATCH v2] vhost: fix avail idx update error when desc copy failed |
Hi Gaoxiang,

> -----Original Message-----
> From: Gaoxiang Liu <gaoxiangliu0@163.com>
> Sent: Wednesday, June 22, 2022 9:20 AM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; liugaoxiang@huawei.com; Gaoxiang Liu
> <gaoxiangliu0@163.com>; stable@dpdk.org
> Subject: [PATCH v2] vhost: fix avail idx update error when desc copy
> failed
>
> When copy_desc_to_mbuf function failed, i added 1.
> And last_avail_idx added i, other than i - 1.
> It may cause that the first mbuf in mbuf-list is dropped,
> the second mbuf in mbuf-list is received in the following
> rx procedure.
> And The pkt_len of the second mbuf is zero, resulting in
> segment fault when parsing the mbuf.

Guess this one is the latest? I believe something is wrong when you forward this,
it generates another patch because of your mail forwarding:

http://patchwork.dpdk.org/project/dpdk/list/?series=&submitter=&state=&q=&archive=&delegate=2642

/Chenbo

>
> Fixes: 0fd5608ef97f ("vhost: handle mbuf allocation failure")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gaoxiang Liu <liugaoxiang@huawei.com>
>
> ---
> v2:
> * Fixed other idx update errors.
> ---
>  lib/vhost/virtio_net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 68a26eb17d..eb254e1024 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2850,11 +2850,11 @@ virtio_dev_tx_split(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
>      if (dropped)
>          rte_pktmbuf_free_bulk(&pkts[i - 1], count - i + 1);
>
> -    vq->last_avail_idx += i;
> +    vq->last_avail_idx += i - dropped;
>
>      do_data_copy_dequeue(vq);
> -    if (unlikely(i < count))
> -        vq->shadow_used_idx = i;
> +    if (unlikely((i - dropped) < count))
> +        vq->shadow_used_idx = i - dropped;
>      if (likely(vq->shadow_used_idx)) {
>          flush_shadow_used_ring_split(dev, vq);
>          vhost_vring_call_split(dev, vq);
> --
> 2.32.0

[-- Attachment #2: Type: text/html, Size: 5737 bytes --]

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

* RE: [PATCH v2] vhost: fix avail idx update error when desc copy failed
  2022-06-22  1:19 ` [PATCH v2] " Gaoxiang Liu
  2022-06-23  3:07   ` Xia, Chenbo
@ 2022-07-01 13:05   ` Xia, Chenbo
  2022-07-02  3:00     ` Gaoxiang Liu
  1 sibling, 1 reply; 7+ messages in thread
From: Xia, Chenbo @ 2022-07-01 13:05 UTC (permalink / raw)
  To: Gaoxiang Liu, maxime.coquelin; +Cc: dev, liugaoxiang, stable

> -----Original Message-----
> From: Gaoxiang Liu <gaoxiangliu0@163.com>
> Sent: Wednesday, June 22, 2022 9:20 AM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; liugaoxiang@huawei.com; Gaoxiang Liu
> <gaoxiangliu0@163.com>; stable@dpdk.org
> Subject: [PATCH v2] vhost: fix avail idx update error when desc copy
> failed
> 
> When copy_desc_to_mbuf function failed, i added 1.

Function name now is desc_to_mbuf

> And last_avail_idx added i, other than i - 1.
> It may cause that the first mbuf in mbuf-list is dropped,
> the second mbuf in mbuf-list is received in the following
> rx procedure.
> And The pkt_len of the second mbuf is zero, resulting in
> segment fault when parsing the mbuf.

Could you help elaborate more? Do you mean first mbuf len is zero
as it's dropped? And where does the segfault happen? APP? Please
describe more to help understand the issue.

But I do notice one problem here is if vhost APP does not handle
the mbuf array correctly, some packets will be missed in the case
of pkts got dropped in the middle of a burst.

Thanks,
Chenbo

> 
> Fixes: 0fd5608ef97f ("vhost: handle mbuf allocation failure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Gaoxiang Liu <liugaoxiang@huawei.com>
> 
> ---
> v2:
> * Fixed other idx update errors.
> ---
>  lib/vhost/virtio_net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 68a26eb17d..eb254e1024 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2850,11 +2850,11 @@ virtio_dev_tx_split(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
>  	if (dropped)
>  		rte_pktmbuf_free_bulk(&pkts[i - 1], count - i + 1);
> 
> -	vq->last_avail_idx += i;
> +	vq->last_avail_idx += i - dropped;
> 
>  	do_data_copy_dequeue(vq);
> -	if (unlikely(i < count))
> -		vq->shadow_used_idx = i;
> +	if (unlikely((i - dropped) < count))
> +		vq->shadow_used_idx = i - dropped;
>  	if (likely(vq->shadow_used_idx)) {
>  		flush_shadow_used_ring_split(dev, vq);
>  		vhost_vring_call_split(dev, vq);
> --
> 2.32.0


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

* Re: [PATCH v2] vhost: fix avail idx update error when desc copy failed
  2022-07-01 13:05   ` Xia, Chenbo
@ 2022-07-02  3:00     ` Gaoxiang Liu
  2022-07-04  7:51       ` Xia, Chenbo
  0 siblings, 1 reply; 7+ messages in thread
From: Gaoxiang Liu @ 2022-07-02  3:00 UTC (permalink / raw)
  To: Xia, Chenbo; +Cc: maxime.coquelin, dev, liugaoxiang, stable

[-- Attachment #1: Type: text/plain, Size: 3569 bytes --]

Hi,Chenbo


If vhost driver receives a mbuf list,the mbuf list has two mbuf,
and the pkt_len of the first mbuf in the mbuf list is the sum of data_len of all mbuf,and the pkt_len of the second mbuf is 0.
When desc_to_mbuf failed,i added 1 and last_avail_idx added i.

It may cause the first mbuf to be dropped and the second mbuf to be received.
It is abnormal,because the received mbuf is not
complete due to lack of the first mbuf,and its pkt_len is 0.
Because the sender sends a normal mbuf-list packet,the vhost app receives the mbuf and considers it should be a normal pkt.
The pkt_len is used ,but is not checked,when the vhost app calculates the checksum of the pkt.
The pkt_len minus the length of the UDP header is a large value because of the negative number reverse.
It results in segment fault when the vhost app uses the large value to traverse the mbuf ,if the address of the mbuf is largest in all mbuf,because the vhost app may access invalid memory .


Thanks.
Gaoxiang







---- Replied Message ----
| From | Xia, Chenbo<chenbo.xia@intel.com> |
| Date | 07/01/2022 21:05 |
| To | Gaoxiang Liu<gaoxiangliu0@163.com>,
maxime.coquelin@redhat.com<maxime.coquelin@redhat.com> |
| Cc | dev@dpdk.org<dev@dpdk.org>,
liugaoxiang@huawei.com<liugaoxiang@huawei.com>,
stable@dpdk.org<stable@dpdk.org> |
| Subject | RE: [PATCH v2] vhost: fix avail idx update error when desc copy failed |
> -----Original Message-----
> From: Gaoxiang Liu <gaoxiangliu0@163.com>
> Sent: Wednesday, June 22, 2022 9:20 AM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; liugaoxiang@huawei.com; Gaoxiang Liu
> <gaoxiangliu0@163.com>; stable@dpdk.org
> Subject: [PATCH v2] vhost: fix avail idx update error when desc copy
> failed
>
> When copy_desc_to_mbuf function failed, i added 1.

Function name now is desc_to_mbuf

> And last_avail_idx added i, other than i - 1.
> It may cause that the first mbuf in mbuf-list is dropped,
> the second mbuf in mbuf-list is received in the following
> rx procedure.
> And The pkt_len of the second mbuf is zero, resulting in
> segment fault when parsing the mbuf.

Could you help elaborate more? Do you mean first mbuf len is zero
as it's dropped? And where does the segfault happen? APP? Please
describe more to help understand the issue.

But I do notice one problem here is if vhost APP does not handle
the mbuf array correctly, some packets will be missed in the case
of pkts got dropped in the middle of a burst.

Thanks,
Chenbo

>
> Fixes: 0fd5608ef97f ("vhost: handle mbuf allocation failure")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gaoxiang Liu <liugaoxiang@huawei.com>
>
> ---
> v2:
> * Fixed other idx update errors.
> ---
>  lib/vhost/virtio_net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 68a26eb17d..eb254e1024 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2850,11 +2850,11 @@ virtio_dev_tx_split(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
>      if (dropped)
>          rte_pktmbuf_free_bulk(&pkts[i - 1], count - i + 1);
>
> -    vq->last_avail_idx += i;
> +    vq->last_avail_idx += i - dropped;
>
>      do_data_copy_dequeue(vq);
> -    if (unlikely(i < count))
> -        vq->shadow_used_idx = i;
> +    if (unlikely((i - dropped) < count))
> +        vq->shadow_used_idx = i - dropped;
>      if (likely(vq->shadow_used_idx)) {
>          flush_shadow_used_ring_split(dev, vq);
>          vhost_vring_call_split(dev, vq);
> --
> 2.32.0

[-- Attachment #2: Type: text/html, Size: 7001 bytes --]

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

* RE: [PATCH v2] vhost: fix avail idx update error when desc copy failed
  2022-07-02  3:00     ` Gaoxiang Liu
@ 2022-07-04  7:51       ` Xia, Chenbo
  0 siblings, 0 replies; 7+ messages in thread
From: Xia, Chenbo @ 2022-07-04  7:51 UTC (permalink / raw)
  To: Gaoxiang Liu; +Cc: dev, liugaoxiang, stable, maxime.coquelin

Hi Gaoxiang,

Please reply in plain text next time.

> From: Gaoxiang Liu <gaoxiangliu0@163.com> 
> Sent: Saturday, July 2, 2022 11:00 AM
> To: Xia, Chenbo <chenbo.xia@intel.com>
> Cc: maxime.coquelin@redhat.com; dev@dpdk.org; liugaoxiang@huawei.com; stable@dpdk.org
> Subject: Re: [PATCH v2] vhost: fix avail idx update error when desc copy failed
>
> Hi,Chenbo
>
> If vhost driver receives a mbuf list,the mbuf list has two mbuf,
> and the pkt_len of the first mbuf in the mbuf list is the sum of data_len of all mbuf,and the pkt_len of the second mbuf is 0.
> When desc_to_mbuf failed,i added 1 and last_avail_idx added i.
> It may cause the first mbuf to be dropped and the second mbuf to be received.
> It is abnormal,because the received mbuf is not
> complete due to lack of the first mbuf,and its pkt_len is 0.
> Because the sender sends a normal mbuf-list packet,the vhost app receives the mbuf and considers it should be a normal pkt.
> The pkt_len is used ,but is not checked,when the vhost app calculates the checksum of the pkt.
> The pkt_len minus the length of the UDP header is a large value because of the negative number reverse.
> It results in segment fault when the vhost app uses the large value to traverse the mbuf ,if the address of the mbuf is largest in all mbuf,because the vhost app may access invalid memory .

First, one element of the mbuf array is for one packet, and it could be
chained mbuf. So if you are talking about chained mbuf, desc_to_mbuf should
fail for the first packet (in your case, this pkt is chained), so return 0,
and APP should check the return value, and knows that no pkt is received.
So I can't understand, if the second mbuf you mean is the second mbuf of
the chained mbuf, why will APP check that? It should never check based
on return value. Anything I missed?

Thanks,
Chenbo

>
> Thanks.
> Gaoxiang



---- Replied Message ----
From
mailto:chenbo.xia@intel.com
Date
07/01/2022 21:05
To
mailto:gaoxiangliu0@163.com,
mailto:maxime.coquelin@redhat.com
Cc
mailto:dev@dpdk.org,
mailto:liugaoxiang@huawei.com,
mailto:stable@dpdk.org
Subject
RE: [PATCH v2] vhost: fix avail idx update error when desc copy failed
> -----Original Message----- 
> From: Gaoxiang Liu <mailto:gaoxiangliu0@163.com> 
> Sent: Wednesday, June 22, 2022 9:20 AM 
> To: mailto:maxime.coquelin@redhat.com; Xia, Chenbo <mailto:chenbo.xia@intel.com> 
> Cc: mailto:dev@dpdk.org; mailto:liugaoxiang@huawei.com; Gaoxiang Liu 
> <mailto:gaoxiangliu0@163.com>; mailto:stable@dpdk.org 
> Subject: [PATCH v2] vhost: fix avail idx update error when desc copy 
> failed 
> 
> When copy_desc_to_mbuf function failed, i added 1. 

Function name now is desc_to_mbuf 

> And last_avail_idx added i, other than i - 1. 
> It may cause that the first mbuf in mbuf-list is dropped, 
> the second mbuf in mbuf-list is received in the following 
> rx procedure. 
> And The pkt_len of the second mbuf is zero, resulting in 
> segment fault when parsing the mbuf. 

Could you help elaborate more? Do you mean first mbuf len is zero 
as it's dropped? And where does the segfault happen? APP? Please 
describe more to help understand the issue. 
 

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

end of thread, other threads:[~2022-07-04  7:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-22  0:56 [PATCH] vhost: fix avail idx update error when desc copy failed Gaoxiang Liu
2022-06-22  1:19 ` [PATCH v2] " Gaoxiang Liu
2022-06-23  3:07   ` Xia, Chenbo
2022-06-23  6:23     ` Gaoxiang Liu
2022-07-01 13:05   ` Xia, Chenbo
2022-07-02  3:00     ` Gaoxiang Liu
2022-07-04  7:51       ` Xia, Chenbo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.