patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH v3] ip_frag: recalculate data length of fragment
@ 2020-12-03 12:54 Yicai Lu
  2020-12-03 16:13 ` [dpdk-stable] [dpdk-dev] " Aaron Conole
  2020-12-07 12:25 ` [dpdk-stable] " Ananyev, Konstantin
  0 siblings, 2 replies; 5+ messages in thread
From: Yicai Lu @ 2020-12-03 12:54 UTC (permalink / raw)
  To: dev
  Cc: konstantin.ananyev, zhoujingbin, chenchanghu, jerry.lilijun,
	haifeng.lin, guohongzhi1, wangyunjian, Yicai Lu, stable

In some situations, we would get several ip fragments, which total
data length is less than minimum frame(64) and padding with zeros.
Examples: Second Fragment "a0a1 a2a3 a4a5 a6a7 0000 0000 ..."
and Third Fragment "a8a9 aaab acad aeaf b0b1 b2b3 ...".
Finally, we would reassemble Second and Third Fragment like this
"a0a1 a2a3 a4a5 a6a7 0000 0000 ... a8a9 aaab acad aeaf b0b1 ...",
which is not correct!
So, we need recalculate data length of fragment to remove padings!

Fixes: 7f0983ee331c ("ip_frag: check fragment length of incoming packet")
Cc: stable@dpdk.org

Signed-off-by: Yicai Lu <luyicai@huawei.com>
---
v2 -> v3: Update the comments.
---
 lib/librte_ip_frag/rte_ipv4_reassembly.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
index 1dda8aca0..9a9fe3703 100644
--- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
@@ -117,6 +117,7 @@ rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
 
 	ip_ofs *= RTE_IPV4_HDR_OFFSET_UNITS;
 	ip_len = rte_be_to_cpu_16(ip_hdr->total_length) - mb->l3_len;
+	mb->data_len = ip_len + mb->l3_len + mb->l2_len;
 
 	IP_FRAG_LOG(DEBUG, "%s:%d:\n"
 		"mbuf: %p, tms: %" PRIu64
-- 
2.28.0.windows.1


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v3] ip_frag: recalculate data length of fragment
  2020-12-03 12:54 [dpdk-stable] [PATCH v3] ip_frag: recalculate data length of fragment Yicai Lu
@ 2020-12-03 16:13 ` Aaron Conole
  2020-12-07  7:48   ` [dpdk-stable] 答复: " luyicai
  2020-12-07 12:25 ` [dpdk-stable] " Ananyev, Konstantin
  1 sibling, 1 reply; 5+ messages in thread
From: Aaron Conole @ 2020-12-03 16:13 UTC (permalink / raw)
  To: Yicai Lu
  Cc: dev, konstantin.ananyev, zhoujingbin, chenchanghu, jerry.lilijun,
	haifeng.lin, guohongzhi1, wangyunjian, stable

Yicai Lu <luyicai@huawei.com> writes:

> In some situations, we would get several ip fragments, which total
> data length is less than minimum frame(64) and padding with zeros.
> Examples: Second Fragment "a0a1 a2a3 a4a5 a6a7 0000 0000 ..."
> and Third Fragment "a8a9 aaab acad aeaf b0b1 b2b3 ...".
> Finally, we would reassemble Second and Third Fragment like this
> "a0a1 a2a3 a4a5 a6a7 0000 0000 ... a8a9 aaab acad aeaf b0b1 ...",
> which is not correct!
> So, we need recalculate data length of fragment to remove padings!
>
> Fixes: 7f0983ee331c ("ip_frag: check fragment length of incoming packet")
> Cc: stable@dpdk.org
>
> Signed-off-by: Yicai Lu <luyicai@huawei.com>
> ---

Sorry for coming late to the party.

Are you saying that we have fragments which are less than min ip_len
for anything other than the final fragment?  Or the total data length
after all fragments are reassembled is < min_ip_len ?

like:

frag1, len = 48
frag2, len = 10

something like that?  Can you put some concrete examples in the commit
message (or even better, include a test case in the ipv4_frag test suite
that shows this)?

> v2 -> v3: Update the comments.
> ---
>  lib/librte_ip_frag/rte_ipv4_reassembly.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> index 1dda8aca0..9a9fe3703 100644
> --- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> @@ -117,6 +117,7 @@ rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
>  
>  	ip_ofs *= RTE_IPV4_HDR_OFFSET_UNITS;
>  	ip_len = rte_be_to_cpu_16(ip_hdr->total_length) - mb->l3_len;
> +	mb->data_len = ip_len + mb->l3_len + mb->l2_len;
>  
>  	IP_FRAG_LOG(DEBUG, "%s:%d:\n"
>  		"mbuf: %p, tms: %" PRIu64


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

* [dpdk-stable] 答复: [dpdk-dev] [PATCH v3] ip_frag: recalculate data length of fragment
  2020-12-03 16:13 ` [dpdk-stable] [dpdk-dev] " Aaron Conole
@ 2020-12-07  7:48   ` luyicai
  0 siblings, 0 replies; 5+ messages in thread
From: luyicai @ 2020-12-07  7:48 UTC (permalink / raw)
  To: Aaron Conole
  Cc: dev, konstantin.ananyev, Zhoujingbin (Robin, Russell Lab),
	chenchanghu, Lilijun (Jerry),
	Linhaifeng, Guohongzhi (Russell Lab),
	wangyunjian, stable

|----------------------------------------------------------------|
|                          HOST                                  |
|                                                                |
| |--------------------|     |-------------------------------|   |
| |         ns2        |     |       |---------------|       |   |
| |     |--------|     |     |   |--------|     |--------|   |   |
| |     |  tap1  |     |     |   |  tap2  | ns1 |  tap3  |   |   |
| |     |mtu=1510|     |     |   |mtu=1510|     |mtu=1500|   |   |
| |-----|1.1.1.1 |-----|     |---|1.1.1.2 |-----|2.1.1.1 |---|   |
|       |--------|               |--------|     |--------|       |
|            |                       |               |           |
|            |                       |               |           |
|            |-----------------------|               |           |
|                                                    |           |
|                                                |--------|      |
|                                                |  bond  |      |
|------------------------------------------------|mtu=1500|------|
                                                 |--------|        

In a complex OVS+DPDK scenario, this problem is found when fragmented packet aggregation is processed.
Therefore, we simulated intermediate fragments by modifying the MTU on the host.
To illustrate the problem, we simplify the packet format and ignore the impact of the packet header.
In namespace2, a packet whose data length is 1520 is sent.
When the packet passes tap2, the packet is divided into two fragments: fragment A and fragment B, similar to (1520 = 1510 + 10).
When the packet passes tap3, the larger fragment packet A is divided into two fragments A1 and A2, similar to (1510 = 1500 + 10).
Finally, the bond interface receives three fragments: A1, A2, and B (1520 = 1500 + 10 + 10).
One fragmented packet A2 is smaller than the minimum Ethernet frame length, so it needs to be padded.

A1:
0000   fa 16 3e 9f fb 82 fa 47 b2 57 dc 20 08 00 45 00
0010   05 dc b4 66 20 00 3f 01 9c b6 01 01 01 01 02 01
0020   01 02 08 00 2d f6 fa e0 00 0a 0e 3a b2 5f 00 00
0030   00 00 2f a9 06 00 00 00 00 00 10 11 12 13 14 15
0040   16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25
...    ...
05e0   b6 b7 b8 b9 ba bb bc bd be bf

A2:
0000   fa 16 3e 9f fb 82 fa 47 b2 57 dc 20 08 00 45 00
0010   00 1c b4 66 20 b9 3f 01 a1 bd 01 01 01 01 02 01
0020   01 02 c0 c1 c2 c3 c4 c5 c6 c7

B:
0000   fa 16 3e 9f fb 82 fa 47 b2 57 dc 20 08 00 45 00
0010   00 33 b4 66 00 ba 3f 01 c1 a5 01 01 01 01 02 01
0020   01 02 c8 c9 ca cb cc cd ce cf d0 d1 d2 d3 d4 d5
0030   d6 d7 d8 d9 da db dc dd de df e0 e1 e2 e3 e4 e5
0040   e6

When processing the preceding packets above, DPDK would aggregate fragmented packets A2 and B.
And error packets are generated, which padding(zero) is displayed in the middle of the packet.

A2 + B:
0000   fa 16 3e 9f fb 82 fa 47 b2 57 dc 20 08 00 45 00
0010   00 33 b4 66 00 ba 3f 01 c1 a5 01 01 01 01 02 01
0020   01 02 c0 c1 c2 c3 c4 c5 c6 c7 00 00 00 00 00 00
0030   00 00 00 00 00 00 00 00 00 00 00 00 c8 c9 ca cb
0040   cc cd ce cf d0 d1 d2 d3 d4 d5 d6 d7 d8 d9 da db
0050   dc dd de df e0 e1 e2 e3 e4 e5 e6


-----邮件原件-----
发件人: Aaron Conole [mailto:aconole@redhat.com] 
发送时间: 2020年12月4日 0:14
收件人: luyicai <luyicai@huawei.com>
抄送: dev@dpdk.org; konstantin.ananyev@intel.com; Zhoujingbin (Robin, Russell Lab) <zhoujingbin@huawei.com>; chenchanghu <chenchanghu@huawei.com>; Lilijun (Jerry) <jerry.lilijun@huawei.com>; Linhaifeng <haifeng.lin@huawei.com>; Guohongzhi (Russell Lab) <guohongzhi1@huawei.com>; wangyunjian <wangyunjian@huawei.com>; stable@dpdk.org
主题: Re: [dpdk-dev] [PATCH v3] ip_frag: recalculate data length of fragment

Yicai Lu <luyicai@huawei.com> writes:

> In some situations, we would get several ip fragments, which total 
> data length is less than minimum frame(64) and padding with zeros.
> Examples: Second Fragment "a0a1 a2a3 a4a5 a6a7 0000 0000 ..."
> and Third Fragment "a8a9 aaab acad aeaf b0b1 b2b3 ...".
> Finally, we would reassemble Second and Third Fragment like this
> "a0a1 a2a3 a4a5 a6a7 0000 0000 ... a8a9 aaab acad aeaf b0b1 ...", 
> which is not correct!
> So, we need recalculate data length of fragment to remove padings!
>
> Fixes: 7f0983ee331c ("ip_frag: check fragment length of incoming 
> packet")
> Cc: stable@dpdk.org
>
> Signed-off-by: Yicai Lu <luyicai@huawei.com>
> ---

Sorry for coming late to the party.

Are you saying that we have fragments which are less than min ip_len for anything other than the final fragment?  Or the total data length after all fragments are reassembled is < min_ip_len ?

like:

frag1, len = 48
frag2, len = 10

something like that?  Can you put some concrete examples in the commit message (or even better, include a test case in the ipv4_frag test suite that shows this)?

> v2 -> v3: Update the comments.
> ---
>  lib/librte_ip_frag/rte_ipv4_reassembly.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c 
> b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> index 1dda8aca0..9a9fe3703 100644
> --- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> @@ -117,6 +117,7 @@ rte_ipv4_frag_reassemble_packet(struct 
> rte_ip_frag_tbl *tbl,
>  
>  	ip_ofs *= RTE_IPV4_HDR_OFFSET_UNITS;
>  	ip_len = rte_be_to_cpu_16(ip_hdr->total_length) - mb->l3_len;
> +	mb->data_len = ip_len + mb->l3_len + mb->l2_len;
>  
>  	IP_FRAG_LOG(DEBUG, "%s:%d:\n"
>  		"mbuf: %p, tms: %" PRIu64


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

* Re: [dpdk-stable] [PATCH v3] ip_frag: recalculate data length of fragment
  2020-12-03 12:54 [dpdk-stable] [PATCH v3] ip_frag: recalculate data length of fragment Yicai Lu
  2020-12-03 16:13 ` [dpdk-stable] [dpdk-dev] " Aaron Conole
@ 2020-12-07 12:25 ` Ananyev, Konstantin
  1 sibling, 0 replies; 5+ messages in thread
From: Ananyev, Konstantin @ 2020-12-07 12:25 UTC (permalink / raw)
  To: Yicai Lu, dev
  Cc: zhoujingbin, chenchanghu, jerry.lilijun, haifeng.lin,
	guohongzhi1, wangyunjian, stable


Hi,
 
> In some situations, we would get several ip fragments, which total
> data length is less than minimum frame(64) and padding with zeros.
> Examples: Second Fragment "a0a1 a2a3 a4a5 a6a7 0000 0000 ..."
> and Third Fragment "a8a9 aaab acad aeaf b0b1 b2b3 ...".
> Finally, we would reassemble Second and Third Fragment like this
> "a0a1 a2a3 a4a5 a6a7 0000 0000 ... a8a9 aaab acad aeaf b0b1 ...",
> which is not correct!
> So, we need recalculate data length of fragment to remove padings!
> 
> Fixes: 7f0983ee331c ("ip_frag: check fragment length of incoming packet")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yicai Lu <luyicai@huawei.com>
> ---
> v2 -> v3: Update the comments.
> ---
>  lib/librte_ip_frag/rte_ipv4_reassembly.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> index 1dda8aca0..9a9fe3703 100644
> --- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> @@ -117,6 +117,7 @@ rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
> 
>  	ip_ofs *= RTE_IPV4_HDR_OFFSET_UNITS;
>  	ip_len = rte_be_to_cpu_16(ip_hdr->total_length) - mb->l3_len;
> +	mb->data_len = ip_len + mb->l3_len + mb->l2_len;

That doesn't look correct.
Even one fragment can consist of multiple segments.
Plus you don't update mb->pkt_len.
To do it properly, you'll need something like:
trim = mb->pkt_len  - ip_len + mb->l3_len + mb->l2_len;
rte_pktmbuf_trim(mb, trim);

Though my preference would be to leave it as responsibility of the caller
(As it has to parse packet anyway to fill l2_len/l3_len and usually strips
l2 headers, etc).  

> 
>  	IP_FRAG_LOG(DEBUG, "%s:%d:\n"
>  		"mbuf: %p, tms: %" PRIu64
> --
> 2.28.0.windows.1


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

* Re: [dpdk-stable] [PATCH v3] ip_frag: recalculate data length of fragment
@ 2020-12-12  9:53 luyicai
  0 siblings, 0 replies; 5+ messages in thread
From: luyicai @ 2020-12-12  9:53 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev
  Cc: Zhoujingbin (Robin, Russell Lab), chenchanghu, Lilijun (Jerry),
	Linhaifeng, Guohongzhi (Russell Lab),
	wangyunjian, stable

Hi,

Thank you so much for your review!
I absolutely agree with that, and
I'll resubmit a patch as soon as possible.

> -----Original Message-----
> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Monday, December 7, 2020 8:25 PM
> To: luyicai <luyicai@huawei.com>; dev@dpdk.org
> Cc: Zhoujingbin (Robin, Russell Lab) <zhoujingbin@huawei.com>; 
> chenchanghu <chenchanghu@huawei.com>; Lilijun (Jerry) 
> <jerry.lilijun@huawei.com>; Linhaifeng <haifeng.lin@huawei.com>; 
> Guohongzhi (Russell Lab) <guohongzhi1@huawei.com>; wangyunjian 
> <wangyunjian@huawei.com>; stable@dpdk.org
> Subject: RE: [PATCH v3] ip_frag: recalculate data length of fragment


> Hi,
 
> In some situations, we would get several ip fragments, which total 
> data length is less than minimum frame(64) and padding with zeros.
> Examples: Second Fragment "a0a1 a2a3 a4a5 a6a7 0000 0000 ..."
> and Third Fragment "a8a9 aaab acad aeaf b0b1 b2b3 ...".
> Finally, we would reassemble Second and Third Fragment like this
> "a0a1 a2a3 a4a5 a6a7 0000 0000 ... a8a9 aaab acad aeaf b0b1 ...", 
> which is not correct!
> So, we need recalculate data length of fragment to remove padings!
> 
> Fixes: 7f0983ee331c ("ip_frag: check fragment length of incoming
> packet")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yicai Lu <luyicai@huawei.com>
> ---
> v2 -> v3: Update the comments.
> ---
>  lib/librte_ip_frag/rte_ipv4_reassembly.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c
> b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> index 1dda8aca0..9a9fe3703 100644
> --- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> @@ -117,6 +117,7 @@ rte_ipv4_frag_reassemble_packet(struct
> rte_ip_frag_tbl *tbl,
> 
>  	ip_ofs *= RTE_IPV4_HDR_OFFSET_UNITS;
>  	ip_len = rte_be_to_cpu_16(ip_hdr->total_length) - mb->l3_len;
> +	mb->data_len = ip_len + mb->l3_len + mb->l2_len;

> That doesn't look correct.
> Even one fragment can consist of multiple segments.
> Plus you don't update mb->pkt_len.
> To do it properly, you'll need something like:
> trim = mb->pkt_len  - ip_len + mb->l3_len + mb->l2_len; 
> rte_pktmbuf_trim(mb, trim);

> Though my preference would be to leave it as responsibility of the 
> caller (As it has to parse packet anyway to fill l2_len/l3_len and 
> usually strips
> l2 headers, etc).  

> 
>  	IP_FRAG_LOG(DEBUG, "%s:%d:\n"
>  		"mbuf: %p, tms: %" PRIu64
> --
> 2.28.0.windows.1

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

end of thread, other threads:[~2020-12-12  9:53 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-03 12:54 [dpdk-stable] [PATCH v3] ip_frag: recalculate data length of fragment Yicai Lu
2020-12-03 16:13 ` [dpdk-stable] [dpdk-dev] " Aaron Conole
2020-12-07  7:48   ` [dpdk-stable] 答复: " luyicai
2020-12-07 12:25 ` [dpdk-stable] " Ananyev, Konstantin
2020-12-12  9:53 luyicai

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).