[dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

[dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

[Jiawei Zhu]

Fixed issue could occur when Mbuf starvation happens
in a middle of reception of a segmented packet.
In such a situation, after release the segments of that
packet, it does not align consumer index to the next stride.
This would cause receive a wrong segmented packet.

Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
Cc: stable@dpdk.org

Signed-off-by: Jiawei Zhu <17826875952@163.com>
---
 drivers/net/mlx5/mlx5_rxtx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 2e4b87c..e3ce9fd 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
 				rte_mbuf_raw_free(pkt);
 				pkt = rep;
 			}
+			rq_ci >>= sges_n;
+			++rq_ci;
+			rq_ci <<= sges_n;
 			break;
 		}
 		if (!pkt) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

[Slava Ovsiienko]

Hi, Jiawei

Thank you for the patch, but It seems I need some clarifications.
As far I understand the issue:

- we are in the midst of receiving the multi-segment packet
- we have some mbufs allocated and packet chain is partially built
- we fail on allocation replenishing mbuf for the segment
- we free all the mbuf of the built chain
- exit from the rx_burtst loop
- rq_ci is expected to be kept pointing to the beginning of the current
  stride - it is supposed on next rx_burst() invocation we'll continue
  Rx queue handling from the stride where we failed
- on loop exit we see the code:
   if (unlikely((i == 0) && ((rq_ci >> sges_n) == rxq->rq_ci)))
          return 0;
   /* Update the consumer index. */
   rxq->rq_ci = rq_ci >> sges_n;
hence, rq_ci is always shifted by sges_n, all increments happened
during failed packet processing are just discarded, it seems no fix is needed.

Did I miss something? 

With best regards,
Slava

> -----Original Message-----
> From: Jiawei Zhu <17826875952@163.com>
> Sent: Monday, February 15, 2021 12:15
> To: dev@dpdk.org
> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
> Jiawei Zhu <17826875952@163.com>; stable@dpdk.org
> Subject: [PATCH] net/mlx5: fix wrong segmented packet in Rx
> 
> Fixed issue could occur when Mbuf starvation happens in a middle of
> reception of a segmented packet.
> In such a situation, after release the segments of that packet, it does not align
> consumer index to the next stride.
> This would cause receive a wrong segmented packet.
> 
> Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiawei Zhu <17826875952@163.com>
> ---
>  drivers/net/mlx5/mlx5_rxtx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c index
> 2e4b87c..e3ce9fd 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
>  				rte_mbuf_raw_free(pkt);
>  				pkt = rep;
>  			}
> +			rq_ci >>= sges_n;
> +			++rq_ci;
> +			rq_ci <<= sges_n;
>  			break;
>  		}
>  		if (!pkt) {
> --
> 1.8.3.1
> 

Re: [dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

[Jiawei Zhu]

Hi, Slava

Thanks for reading my patch, my issue may not be clear.
Here I give a possible error.
- we assume segs_n is 4 and we are receiving 4 segments multi-segment 
packet.
- we fail to alloc mbuf when receive the 3th segment，so it will free the 
mbufs which packet chain we have built. Here are the 1st and 2nd segment.
- Rx queue in this stride, the 1st and the 2nd segment are fill the new 
mbuf and there data will be rand, but the 3th and 4th segment are still 
fill the last data. So next if still begin on this stride， it will 
reveice wrong multi-segment packet.

- So we should discarded this packets and pass this stride. After exit 
the loop, we should align the next consumer index.

What Do you thinking?

With best regards
Jiawei

On 2021/2/24 9:20 PM, Slava Ovsiienko wrote:
> Hi, Jiawei
> 
> Thank you for the patch, but It seems I need some clarifications.
> As far I understand the issue:
> 
> - we are in the midst of receiving the multi-segment packet
> - we have some mbufs allocated and packet chain is partially built
> - we fail on allocation replenishing mbuf for the segment
> - we free all the mbuf of the built chain
> - exit from the rx_burtst loop
> - rq_ci is expected to be kept pointing to the beginning of the current
>    stride - it is supposed on next rx_burst() invocation we'll continue
>    Rx queue handling from the stride where we failed
> - on loop exit we see the code:
>     if (unlikely((i == 0) && ((rq_ci >> sges_n) == rxq->rq_ci)))
>            return 0;
>     /* Update the consumer index. */
>     rxq->rq_ci = rq_ci >> sges_n;
> hence, rq_ci is always shifted by sges_n, all increments happened
> during failed packet processing are just discarded, it seems no fix is needed.
> 
> Did I miss something?
> 
> With best regards,
> Slava
> 
>> -----Original Message-----
>> From: Jiawei Zhu <17826875952@163.com>
>> Sent: Monday, February 15, 2021 12:15
>> To: dev@dpdk.org
>> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
>> Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
>> Jiawei Zhu <17826875952@163.com>; stable@dpdk.org
>> Subject: [PATCH] net/mlx5: fix wrong segmented packet in Rx
>>
>> Fixed issue could occur when Mbuf starvation happens in a middle of
>> reception of a segmented packet.
>> In such a situation, after release the segments of that packet, it does not align
>> consumer index to the next stride.
>> This would cause receive a wrong segmented packet.
>>
>> Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Jiawei Zhu <17826875952@163.com>
>> ---
>>   drivers/net/mlx5/mlx5_rxtx.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c index
>> 2e4b87c..e3ce9fd 100644
>> --- a/drivers/net/mlx5/mlx5_rxtx.c
>> +++ b/drivers/net/mlx5/mlx5_rxtx.c
>> @@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
>>   				rte_mbuf_raw_free(pkt);
>>   				pkt = rep;
>>   			}
>> +			rq_ci >>= sges_n;
>> +			++rq_ci;
>> +			rq_ci <<= sges_n;
>>   			break;
>>   		}
>>   		if (!pkt) {
>> --
>> 1.8.3.1
>>
> 

Re: [dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

[Slava Ovsiienko]

Hi, Jiawei

Thank you for the clarification. I missed the point that we have updated elts array 
with new allocated mbufs and are not able to retry packet building anymore.
Very good catch, thank you!  Could you, please, add this extra explanation
to the  commit message and send the v2 ?

With best regards, 
Slava

> -----Original Message-----
> From: Jiawei Zhu <17826875952@163.com>
> Sent: Friday, February 26, 2021 18:11
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; stable@dpdk.org
> Subject: Re: [PATCH] net/mlx5: fix wrong segmented packet in Rx
> 
> Hi, Slava
> 
> Thanks for reading my patch, my issue may not be clear.
> Here I give a possible error.
> - we assume segs_n is 4 and we are receiving 4 segments multi-segment
> packet.
> - we fail to alloc mbuf when receive the 3th segment，so it will free the
> mbufs which packet chain we have built. Here are the 1st and 2nd segment.
> - Rx queue in this stride, the 1st and the 2nd segment are fill the new mbuf
> and there data will be rand, but the 3th and 4th segment are still fill the last
> data. So next if still begin on this stride， it will reveice wrong multi-segment
> packet.
> 
> - So we should discarded this packets and pass this stride. After exit the loop,
> we should align the next consumer index.
> 
> What Do you thinking?
> 
> With best regards
> Jiawei
> 
> On 2021/2/24 9:20 PM, Slava Ovsiienko wrote:
> > Hi, Jiawei
> >
> > Thank you for the patch, but It seems I need some clarifications.
> > As far I understand the issue:
> >
> > - we are in the midst of receiving the multi-segment packet
> > - we have some mbufs allocated and packet chain is partially built
> > - we fail on allocation replenishing mbuf for the segment
> > - we free all the mbuf of the built chain
> > - exit from the rx_burtst loop
> > - rq_ci is expected to be kept pointing to the beginning of the current
> >    stride - it is supposed on next rx_burst() invocation we'll continue
> >    Rx queue handling from the stride where we failed
> > - on loop exit we see the code:
> >     if (unlikely((i == 0) && ((rq_ci >> sges_n) == rxq->rq_ci)))
> >            return 0;
> >     /* Update the consumer index. */
> >     rxq->rq_ci = rq_ci >> sges_n;
> > hence, rq_ci is always shifted by sges_n, all increments happened
> > during failed packet processing are just discarded, it seems no fix is needed.
> >
> > Did I miss something?
> >
> > With best regards,
> > Slava
> >
> >> -----Original Message-----
> >> From: Jiawei Zhu <17826875952@163.com>
> >> Sent: Monday, February 15, 2021 12:15
> >> To: dev@dpdk.org
> >> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
> >> Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> >> <viacheslavo@nvidia.com>; Jiawei Zhu <17826875952@163.com>;
> >> stable@dpdk.org
> >> Subject: [PATCH] net/mlx5: fix wrong segmented packet in Rx
> >>
> >> Fixed issue could occur when Mbuf starvation happens in a middle of
> >> reception of a segmented packet.
> >> In such a situation, after release the segments of that packet, it
> >> does not align consumer index to the next stride.
> >> This would cause receive a wrong segmented packet.
> >>
> >> Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx
> >> path")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Jiawei Zhu <17826875952@163.com>
> >> ---
> >>   drivers/net/mlx5/mlx5_rxtx.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/net/mlx5/mlx5_rxtx.c
> >> b/drivers/net/mlx5/mlx5_rxtx.c index 2e4b87c..e3ce9fd 100644
> >> --- a/drivers/net/mlx5/mlx5_rxtx.c
> >> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> >> @@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
> >>   				rte_mbuf_raw_free(pkt);
> >>   				pkt = rep;
> >>   			}
> >> +			rq_ci >>= sges_n;
> >> +			++rq_ci;
> >> +			rq_ci <<= sges_n;
> >>   			break;
> >>   		}
> >>   		if (!pkt) {
> >> --
> >> 1.8.3.1
> >>
> >

[dpdk-dev] [PATCH v2] net/mlx5: fix wrong segmented packet in Rx

[Jiawei Zhu]

Fixed issue could occur when Mbuf starvation happens
in a middle of reception of a segmented packet.
In such a situation, after release the segments of that
packet, it does not align consumer index to the next stride.
This would cause receive a wrong segmented packet.

Here is a possible error.
- we assume segs_n is 4 and we are receiving 4
  segments multi-segment packet.
- we fail to alloc mbuf when receive the 3th segment
  so it will free the mbufs which packet chain we have
  built. Here are the 1st and 2nd segment.
- Rx queue in this stride, the 1st and the 2nd segment
  are fill the new mbuf and there data will be rand,
  but the 3th and 4th segment are still fill the last data.
  So next if still begin on this stride, it will receive
  wrong multi-segment packet.
- So we should discarded this packets and pass this stride.
  Before exit the loop, we should align the next consumer index.

Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
Cc: stable@dpdk.org

Signed-off-by: Jiawei Zhu <17826875952@163.com>
---
v2:
* Added extra explanation in commit message.
---
 drivers/net/mlx5/mlx5_rxtx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 2e4b87c..e3ce9fd 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
 				rte_mbuf_raw_free(pkt);
 				pkt = rep;
 			}
+			rq_ci >>= sges_n;
+			++rq_ci;
+			rq_ci <<= sges_n;
 			break;
 		}
 		if (!pkt) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

[Jiawei Zhu]

Hi, Slava
Thank you for your agreement. Here is the v2 patch:

https://patches.dpdk.org/project/dpdk/patch/1614617885-2650-1-git-send-email-17826875952@163.com/

With best regards,
Jiawei

On 2021/3/1 5:13 PM, Slava Ovsiienko wrote:
> Hi, Jiawei
> 
> Thank you for the clarification. I missed the point that we have updated elts array
> with new allocated mbufs and are not able to retry packet building anymore.
> Very good catch, thank you!  Could you, please, add this extra explanation
> to the  commit message and send the v2 ?
> 
> With best regards,
> Slava
> 
>> -----Original Message-----
>> From: Jiawei Zhu <17826875952@163.com>
>> Sent: Friday, February 26, 2021 18:11
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
>> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
>> Shuler <shahafs@nvidia.com>; stable@dpdk.org
>> Subject: Re: [PATCH] net/mlx5: fix wrong segmented packet in Rx
>>
>> Hi, Slava
>>
>> Thanks for reading my patch, my issue may not be clear.
>> Here I give a possible error.
>> - we assume segs_n is 4 and we are receiving 4 segments multi-segment
>> packet.
>> - we fail to alloc mbuf when receive the 3th segment，so it will free the
>> mbufs which packet chain we have built. Here are the 1st and 2nd segment.
>> - Rx queue in this stride, the 1st and the 2nd segment are fill the new mbuf
>> and there data will be rand, but the 3th and 4th segment are still fill the last
>> data. So next if still begin on this stride， it will reveice wrong multi-segment
>> packet.
>>
>> - So we should discarded this packets and pass this stride. After exit the loop,
>> we should align the next consumer index.
>>
>> What Do you thinking?
>>
>> With best regards
>> Jiawei
>>
>> On 2021/2/24 9:20 PM, Slava Ovsiienko wrote:
>>> Hi, Jiawei
>>>
>>> Thank you for the patch, but It seems I need some clarifications.
>>> As far I understand the issue:
>>>
>>> - we are in the midst of receiving the multi-segment packet
>>> - we have some mbufs allocated and packet chain is partially built
>>> - we fail on allocation replenishing mbuf for the segment
>>> - we free all the mbuf of the built chain
>>> - exit from the rx_burtst loop
>>> - rq_ci is expected to be kept pointing to the beginning of the current
>>>     stride - it is supposed on next rx_burst() invocation we'll continue
>>>     Rx queue handling from the stride where we failed
>>> - on loop exit we see the code:
>>>      if (unlikely((i == 0) && ((rq_ci >> sges_n) == rxq->rq_ci)))
>>>             return 0;
>>>      /* Update the consumer index. */
>>>      rxq->rq_ci = rq_ci >> sges_n;
>>> hence, rq_ci is always shifted by sges_n, all increments happened
>>> during failed packet processing are just discarded, it seems no fix is needed.
>>>
>>> Did I miss something?
>>>
>>> With best regards,
>>> Slava
>>>
>>>> -----Original Message-----
>>>> From: Jiawei Zhu <17826875952@163.com>
>>>> Sent: Monday, February 15, 2021 12:15
>>>> To: dev@dpdk.org
>>>> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
>>>> Shuler <shahafs@nvidia.com>; Slava Ovsiienko
>>>> <viacheslavo@nvidia.com>; Jiawei Zhu <17826875952@163.com>;
>>>> stable@dpdk.org
>>>> Subject: [PATCH] net/mlx5: fix wrong segmented packet in Rx
>>>>
>>>> Fixed issue could occur when Mbuf starvation happens in a middle of
>>>> reception of a segmented packet.
>>>> In such a situation, after release the segments of that packet, it
>>>> does not align consumer index to the next stride.
>>>> This would cause receive a wrong segmented packet.
>>>>
>>>> Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx
>>>> path")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Jiawei Zhu <17826875952@163.com>
>>>> ---
>>>>    drivers/net/mlx5/mlx5_rxtx.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/net/mlx5/mlx5_rxtx.c
>>>> b/drivers/net/mlx5/mlx5_rxtx.c index 2e4b87c..e3ce9fd 100644
>>>> --- a/drivers/net/mlx5/mlx5_rxtx.c
>>>> +++ b/drivers/net/mlx5/mlx5_rxtx.c
>>>> @@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
>>>>    				rte_mbuf_raw_free(pkt);
>>>>    				pkt = rep;
>>>>    			}
>>>> +			rq_ci >>= sges_n;
>>>> +			++rq_ci;
>>>> +			rq_ci <<= sges_n;
>>>>    			break;
>>>>    		}
>>>>    		if (!pkt) {
>>>> --
>>>> 1.8.3.1
>>>>
>>>
> 

[dpdk-dev] [PATCH v3] net/mlx5: fix wrong segmented packet in Rx

[Jiawei Zhu]

The issue occurred if mbuf starvation happened
in the middle of segmented packet reception.
In such a situation, after release the segments of
packet being received, code did not advance the
consumer index to the next stride. This caused
the receiving of the wrong segmented packet data.

The possible error scenario:
- we assume segs_n is 4 and we are receiving 4
  segments of multi-segment packet.
- we fail to allocate mbuf while receiving the 3rd segment,
  and this frees the mbufs of the packet chain we have built.
  There are the 1st and 2nd segments in the chain.
- the 1st and the 2nd segments of this stride of Rx queue
  are filled up (in elts array) with the new allocated
  mbufs and their data are random (the 3rd and 4th
  segments still contain the valid data of the packet though).
- on the next iteration of stride processing we get
  the wrong two segments of the multi-segment packet.

Hence, we should skip these mbufs in the stride and
we should advance the consumer index on loop exit.

Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
Cc: stable@dpdk.org

Signed-off-by: Jiawei Zhu <17826875952@163.com>
---
v3:
* Reword the commit message a little bit.

v2:
* Added extra explanation in commit message.
---
 drivers/net/mlx5/mlx5_rxtx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 2e4b87c..e3ce9fd 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
 				rte_mbuf_raw_free(pkt);
 				pkt = rep;
 			}
+			rq_ci >>= sges_n;
+			++rq_ci;
+			rq_ci <<= sges_n;
 			break;
 		}
 		if (!pkt) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

[Slava Ovsiienko]

Hi, Jiawei

Thanks a lot for the update.
There are some common points for the commit messages of fixing patches:
- the bug/error/issue should be described in PAST tense (what we HAD before the fix)
- what fix is doing should be described in PRESENT tense (what we HAVE right now, after fix apply)

Also, can we fix some typos in the message and reword it a little bit?
What do you think about something like this:

The issue occurred if mbuf starvation happened
in the middle of segmented packet reception.
In such a situation, after release the segments of
packet being received, code did not advance the
consumer index to the next stride. This caused
the receiving of the wrong segmented packet data.

The possible error scenario:
- we assume segs_n is 4 and we are receiving 4
  segments of multi-segment packet.
- we fail to allocate mbuf while receiving the 3rd segment,
  and this frees the mbufs of the packet chain we have built.
  There are the 1st and 2nd segments in the chain.
- the 1st and the 2nd segments of this stride of Rx queue
  are filled up (in elts array) with the new allocated
  mbufs and their data are random (the 3rd and 4th
  segments still contain the valid data of the packet though).
- on the next iteration of stride processing we get
  the wrong two segments of the multi-segment packet.

Hence, we should skip these mbufs in the stride and
we should advance the consumer index on loop exit.

With best regards,
Slava

> -----Original Message-----
> From: Jiawei Zhu <17826875952@163.com>
> Sent: Monday, March 1, 2021 19:02
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; stable@dpdk.org
> Subject: Re: [PATCH] net/mlx5: fix wrong segmented packet in Rx
> 
> Hi, Slava
> Thank you for your agreement. Here is the v2 patch:
> 
> https://patches.dpdk.org/project/dpdk/patch/1614617885-2650-1-git-send-
> email-17826875952@163.com/
> 
> With best regards,
> Jiawei
> 
> On 2021/3/1 5:13 PM, Slava Ovsiienko wrote:
> > Hi, Jiawei
> >
> > Thank you for the clarification. I missed the point that we have
> > updated elts array with new allocated mbufs and are not able to retry
> packet building anymore.
> > Very good catch, thank you!  Could you, please, add this extra
> > explanation to the  commit message and send the v2 ?
> >
> > With best regards,
> > Slava
> >
> >> -----Original Message-----
> >> From: Jiawei Zhu <17826875952@163.com>
> >> Sent: Friday, February 26, 2021 18:11
> >> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
> >> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
> >> Shuler <shahafs@nvidia.com>; stable@dpdk.org
> >> Subject: Re: [PATCH] net/mlx5: fix wrong segmented packet in Rx
> >>
> >> Hi, Slava
> >>
> >> Thanks for reading my patch, my issue may not be clear.
> >> Here I give a possible error.
> >> - we assume segs_n is 4 and we are receiving 4 segments multi-segment
> >> packet.
> >> - we fail to alloc mbuf when receive the 3th segment，so it will free
> >> the mbufs which packet chain we have built. Here are the 1st and 2nd
> segment.
> >> - Rx queue in this stride, the 1st and the 2nd segment are fill the
> >> new mbuf and there data will be rand, but the 3th and 4th segment are
> >> still fill the last data. So next if still begin on this stride， it
> >> will reveice wrong multi-segment packet.
> >>
> >> - So we should discarded this packets and pass this stride. After
> >> exit the loop, we should align the next consumer index.
> >>
> >> What Do you thinking?
> >>
> >> With best regards
> >> Jiawei
> >>
> >> On 2021/2/24 9:20 PM, Slava Ovsiienko wrote:
> >>> Hi, Jiawei
> >>>
> >>> Thank you for the patch, but It seems I need some clarifications.
> >>> As far I understand the issue:
> >>>
> >>> - we are in the midst of receiving the multi-segment packet
> >>> - we have some mbufs allocated and packet chain is partially built
> >>> - we fail on allocation replenishing mbuf for the segment
> >>> - we free all the mbuf of the built chain
> >>> - exit from the rx_burtst loop
> >>> - rq_ci is expected to be kept pointing to the beginning of the current
> >>>     stride - it is supposed on next rx_burst() invocation we'll continue
> >>>     Rx queue handling from the stride where we failed
> >>> - on loop exit we see the code:
> >>>      if (unlikely((i == 0) && ((rq_ci >> sges_n) == rxq->rq_ci)))
> >>>             return 0;
> >>>      /* Update the consumer index. */
> >>>      rxq->rq_ci = rq_ci >> sges_n;
> >>> hence, rq_ci is always shifted by sges_n, all increments happened
> >>> during failed packet processing are just discarded, it seems no fix is
> needed.
> >>>
> >>> Did I miss something?
> >>>
> >>> With best regards,
> >>> Slava
> >>>
> >>>> -----Original Message-----
> >>>> From: Jiawei Zhu <17826875952@163.com>
> >>>> Sent: Monday, February 15, 2021 12:15
> >>>> To: dev@dpdk.org
> >>>> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>;
> Shahaf
> >>>> Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> >>>> <viacheslavo@nvidia.com>; Jiawei Zhu <17826875952@163.com>;
> >>>> stable@dpdk.org
> >>>> Subject: [PATCH] net/mlx5: fix wrong segmented packet in Rx
> >>>>
> >>>> Fixed issue could occur when Mbuf starvation happens in a middle of
> >>>> reception of a segmented packet.
> >>>> In such a situation, after release the segments of that packet, it
> >>>> does not align consumer index to the next stride.
> >>>> This would cause receive a wrong segmented packet.
> >>>>
> >>>> Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx
> >>>> path")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Jiawei Zhu <17826875952@163.com>
> >>>> ---
> >>>>    drivers/net/mlx5/mlx5_rxtx.c | 3 +++
> >>>>    1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/mlx5/mlx5_rxtx.c
> >>>> b/drivers/net/mlx5/mlx5_rxtx.c index 2e4b87c..e3ce9fd 100644
> >>>> --- a/drivers/net/mlx5/mlx5_rxtx.c
> >>>> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> >>>> @@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
> >>>>    				rte_mbuf_raw_free(pkt);
> >>>>    				pkt = rep;
> >>>>    			}
> >>>> +			rq_ci >>= sges_n;
> >>>> +			++rq_ci;
> >>>> +			rq_ci <<= sges_n;
> >>>>    			break;
> >>>>    		}
> >>>>    		if (!pkt) {
> >>>> --
> >>>> 1.8.3.1
> >>>>
> >>>
> >

Re: [dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

[Jiawei Zhu]

Hi, Slava

Thanks a lot for your correction and advice. I'm not very good at 
grammar 😁.
Here is new patch and used your words.😃

https://patches.dpdk.org/project/dpdk/patch/1614619190-3846-1-git-send-email-17826875952@163.com/

With best regards,
Jiawei

On 2021/3/2 4:10 PM, Slava Ovsiienko wrote:
> Hi, Jiawei
> 
> Thanks a lot for the update.
> There are some common points for the commit messages of fixing patches:
> - the bug/error/issue should be described in PAST tense (what we HAD before the fix)
> - what fix is doing should be described in PRESENT tense (what we HAVE right now, after fix apply)
> 
> Also, can we fix some typos in the message and reword it a little bit?
> What do you think about something like this:
> 
> The issue occurred if mbuf starvation happened
> in the middle of segmented packet reception.
> In such a situation, after release the segments of
> packet being received, code did not advance the
> consumer index to the next stride. This caused
> the receiving of the wrong segmented packet data.
> 
> The possible error scenario:
> - we assume segs_n is 4 and we are receiving 4
>    segments of multi-segment packet.
> - we fail to allocate mbuf while receiving the 3rd segment,
>    and this frees the mbufs of the packet chain we have built.
>    There are the 1st and 2nd segments in the chain.
> - the 1st and the 2nd segments of this stride of Rx queue
>    are filled up (in elts array) with the new allocated
>    mbufs and their data are random (the 3rd and 4th
>    segments still contain the valid data of the packet though).
> - on the next iteration of stride processing we get
>    the wrong two segments of the multi-segment packet.
> 
> Hence, we should skip these mbufs in the stride and
> we should advance the consumer index on loop exit.
> 
> With best regards,
> Slava
> 
>> -----Original Message-----
>> From: Jiawei Zhu <17826875952@163.com>
>> Sent: Monday, March 1, 2021 19:02
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
>> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
>> Shuler <shahafs@nvidia.com>; stable@dpdk.org
>> Subject: Re: [PATCH] net/mlx5: fix wrong segmented packet in Rx
>>
>> Hi, Slava
>> Thank you for your agreement. Here is the v2 patch:
>>
>> https://patches.dpdk.org/project/dpdk/patch/1614617885-2650-1-git-send-
>> email-17826875952@163.com/
>>
>> With best regards,
>> Jiawei
>>
>> On 2021/3/1 5:13 PM, Slava Ovsiienko wrote:
>>> Hi, Jiawei
>>>
>>> Thank you for the clarification. I missed the point that we have
>>> updated elts array with new allocated mbufs and are not able to retry
>> packet building anymore.
>>> Very good catch, thank you!  Could you, please, add this extra
>>> explanation to the  commit message and send the v2 ?
>>>
>>> With best regards,
>>> Slava
>>>
>>>> -----Original Message-----
>>>> From: Jiawei Zhu <17826875952@163.com>
>>>> Sent: Friday, February 26, 2021 18:11
>>>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
>>>> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
>>>> Shuler <shahafs@nvidia.com>; stable@dpdk.org
>>>> Subject: Re: [PATCH] net/mlx5: fix wrong segmented packet in Rx
>>>>
>>>> Hi, Slava
>>>>
>>>> Thanks for reading my patch, my issue may not be clear.
>>>> Here I give a possible error.
>>>> - we assume segs_n is 4 and we are receiving 4 segments multi-segment
>>>> packet.
>>>> - we fail to alloc mbuf when receive the 3th segment，so it will free
>>>> the mbufs which packet chain we have built. Here are the 1st and 2nd
>> segment.
>>>> - Rx queue in this stride, the 1st and the 2nd segment are fill the
>>>> new mbuf and there data will be rand, but the 3th and 4th segment are
>>>> still fill the last data. So next if still begin on this stride， it
>>>> will reveice wrong multi-segment packet.
>>>>
>>>> - So we should discarded this packets and pass this stride. After
>>>> exit the loop, we should align the next consumer index.
>>>>
>>>> What Do you thinking?
>>>>
>>>> With best regards
>>>> Jiawei
>>>>
>>>> On 2021/2/24 9:20 PM, Slava Ovsiienko wrote:
>>>>> Hi, Jiawei
>>>>>
>>>>> Thank you for the patch, but It seems I need some clarifications.
>>>>> As far I understand the issue:
>>>>>
>>>>> - we are in the midst of receiving the multi-segment packet
>>>>> - we have some mbufs allocated and packet chain is partially built
>>>>> - we fail on allocation replenishing mbuf for the segment
>>>>> - we free all the mbuf of the built chain
>>>>> - exit from the rx_burtst loop
>>>>> - rq_ci is expected to be kept pointing to the beginning of the current
>>>>>      stride - it is supposed on next rx_burst() invocation we'll continue
>>>>>      Rx queue handling from the stride where we failed
>>>>> - on loop exit we see the code:
>>>>>       if (unlikely((i == 0) && ((rq_ci >> sges_n) == rxq->rq_ci)))
>>>>>              return 0;
>>>>>       /* Update the consumer index. */
>>>>>       rxq->rq_ci = rq_ci >> sges_n;
>>>>> hence, rq_ci is always shifted by sges_n, all increments happened
>>>>> during failed packet processing are just discarded, it seems no fix is
>> needed.
>>>>>
>>>>> Did I miss something?
>>>>>
>>>>> With best regards,
>>>>> Slava
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jiawei Zhu <17826875952@163.com>
>>>>>> Sent: Monday, February 15, 2021 12:15
>>>>>> To: dev@dpdk.org
>>>>>> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>;
>> Shahaf
>>>>>> Shuler <shahafs@nvidia.com>; Slava Ovsiienko
>>>>>> <viacheslavo@nvidia.com>; Jiawei Zhu <17826875952@163.com>;
>>>>>> stable@dpdk.org
>>>>>> Subject: [PATCH] net/mlx5: fix wrong segmented packet in Rx
>>>>>>
>>>>>> Fixed issue could occur when Mbuf starvation happens in a middle of
>>>>>> reception of a segmented packet.
>>>>>> In such a situation, after release the segments of that packet, it
>>>>>> does not align consumer index to the next stride.
>>>>>> This would cause receive a wrong segmented packet.
>>>>>>
>>>>>> Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx
>>>>>> path")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Jiawei Zhu <17826875952@163.com>
>>>>>> ---
>>>>>>     drivers/net/mlx5/mlx5_rxtx.c | 3 +++
>>>>>>     1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/mlx5/mlx5_rxtx.c
>>>>>> b/drivers/net/mlx5/mlx5_rxtx.c index 2e4b87c..e3ce9fd 100644
>>>>>> --- a/drivers/net/mlx5/mlx5_rxtx.c
>>>>>> +++ b/drivers/net/mlx5/mlx5_rxtx.c
>>>>>> @@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
>>>>>>     				rte_mbuf_raw_free(pkt);
>>>>>>     				pkt = rep;
>>>>>>     			}
>>>>>> +			rq_ci >>= sges_n;
>>>>>> +			++rq_ci;
>>>>>> +			rq_ci <<= sges_n;
>>>>>>     			break;
>>>>>>     		}
>>>>>>     		if (!pkt) {
>>>>>> --
>>>>>> 1.8.3.1
>>>>>>
>>>>>
>>>
> 

Re: [dpdk-dev] [PATCH v3] net/mlx5: fix wrong segmented packet in Rx

[Slava Ovsiienko]

> -----Original Message-----
> From: Jiawei Zhu <17826875952@163.com>
> Sent: Monday, March 1, 2021 19:20
> To: dev@dpdk.org
> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
> Jiawei Zhu <17826875952@163.com>; stable@dpdk.org
> Subject: [PATCH v3] net/mlx5: fix wrong segmented packet in Rx
> 
> The issue occurred if mbuf starvation happened in the middle of segmented
> packet reception.
> In such a situation, after release the segments of packet being received, code
> did not advance the consumer index to the next stride. This caused the
> receiving of the wrong segmented packet data.
> 
> The possible error scenario:
> - we assume segs_n is 4 and we are receiving 4
>   segments of multi-segment packet.
> - we fail to allocate mbuf while receiving the 3rd segment,
>   and this frees the mbufs of the packet chain we have built.
>   There are the 1st and 2nd segments in the chain.
> - the 1st and the 2nd segments of this stride of Rx queue
>   are filled up (in elts array) with the new allocated
>   mbufs and their data are random (the 3rd and 4th
>   segments still contain the valid data of the packet though).
> - on the next iteration of stride processing we get
>   the wrong two segments of the multi-segment packet.
> 
> Hence, we should skip these mbufs in the stride and we should advance the
> consumer index on loop exit.
> 
> Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiawei Zhu <17826875952@163.com>
> ---
> v3:
> * Reword the commit message a little bit.
> 
> v2:
> * Added extra explanation in commit message.
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

Thank you for the nice catch and working on the patch update.

With best regards, Slava

Re: [dpdk-dev] [PATCH v3] net/mlx5: fix wrong segmented packet in Rx

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jiawei Zhu
> Sent: Monday, March 1, 2021 7:20 PM
> To: dev@dpdk.org
> Cc: zhujiawei12@huawei.com; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
> Jiawei Zhu <17826875952@163.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v3] net/mlx5: fix wrong segmented packet in Rx
> 
> The issue occurred if mbuf starvation happened
> in the middle of segmented packet reception.
> In such a situation, after release the segments of
> packet being received, code did not advance the
> consumer index to the next stride. This caused
> the receiving of the wrong segmented packet data.
> 
> The possible error scenario:
> - we assume segs_n is 4 and we are receiving 4
>   segments of multi-segment packet.
> - we fail to allocate mbuf while receiving the 3rd segment,
>   and this frees the mbufs of the packet chain we have built.
>   There are the 1st and 2nd segments in the chain.
> - the 1st and the 2nd segments of this stride of Rx queue
>   are filled up (in elts array) with the new allocated
>   mbufs and their data are random (the 3rd and 4th
>   segments still contain the valid data of the packet though).
> - on the next iteration of stride processing we get
>   the wrong two segments of the multi-segment packet.
> 
> Hence, we should skip these mbufs in the stride and
> we should advance the consumer index on loop exit.
> 
> Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiawei Zhu <17826875952@163.com>
> ---
> v3:
> * Reword the commit message a little bit.
> 
> v2:
> * Added extra explanation in commit message.
> ---
>  drivers/net/mlx5/mlx5_rxtx.c | 3 +++
>  1 file changed, 3 insertions(+)

Patch applied to next-net-mlx,
Kindest regards,
Raslan Darawsheh

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] net/mlx5: fix wrong segmented packet in Rx

[Thomas Monjalon]

> > Signed-off-by: Jiawei Zhu <17826875952@163.com>
> 
> Patch applied to next-net-mlx,

Any specific reason for using your email address @163.com?
Would your huawei address (Cc'ed) fit here?
If you are OK, I prefer recognizing your contributions with a single identity.