Skip to contentSkip to conversation

gro : fix pkt length when extra bytes are padded to ethernet frame

PATCH In review Kumara Parameshwaran 6 participants 12 messages Oct 10, 2022 – Oct 31, 2023 1 message not in the archive

lore
Share & tools
lore

Kumara Parameshwaran

author
#0 lore raw

PATCH 1/1 gro : fix pkt length when extra bytes are padded to ethernet frame

From: Kumara Parameshwaran <[email protected]>

When GRO packets are merged the packet length is used while merging the adjacent packets. If the padded bytes are accounted we would end up acking unsent TCP segments.

Signed-off-by Kumara Parameshwaran <[email protected]>

v1:
If there is padding to the ethernet frame cases where timestamp is disabled the packet length should be validated with the total ip length as packet length is used in the GRO merging logic

---
 lib/gro/gro_tcp4.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
lib/gro/gro_tcp4.c +7 -1
index 7498c66141..ac6439bb4e 100644
@@ -198,7 +198,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	struct rte_tcp_hdr *tcp_hdr;
 	uint32_t sent_seq;
 	int32_t tcp_dl;
-	uint16_t ip_id, hdr_len, frag_off;
+	uint16_t ip_id, hdr_len, frag_off, ip_len;
 	uint8_t is_atomic;
 
 	struct tcp4_flow_key key;

@@ -219,6 +219,12 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	tcp_hdr = (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
 	hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len;
 
+	ip_len = rte_be_to_cpu_16(ipv4_hdr->total_length);
+	/* To handle padding cases in cases like where timestamps are disabled */
+	if (unlikely(pkt->pkt_len > (uint32_t)(ip_len + pkt->l2_len))) {
+		pkt->pkt_len = ip_len + pkt->l2_len;
+	}
+
 	/*
 	 * Don't process the packet which has FIN, SYN, RST, PSH, URG, ECE
 	 * or CWR set.
Signature · 1 line
2.25.1
#1 lore raw
Quoted 7 lines · Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>

When GRO packets are merged the packet length is used while merging the adjacent packets. If the padded bytes are accounted we would end up acking unsent TCP segments.

Quoted 5 lines
Signed-off-by Kumara Parameshwaran <[email protected]>

v1:
If there is padding to the ethernet frame cases where timestamp is disabled the packet length should be validated with the total ip length as packet length is used in the GRO merging logic

Please, can you test with current main branch?
We recently merged a fix:
https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403

Signature · 1 line
David Marchand

A message that is not in the archive (sent off-list or not yet received): CANxNyasJ7RWk6S9hArfKc=m39huCOgab92sV4DjxDh0VENLf7Q@mail.gmail.com

author
#3 lore raw

PATCH 1/1 gro : fix pkt length when extra bytes are padded to ethernet frame

Quoted 41 lines · David Marchand
Quoted 10 lines · Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>

When GRO packets are merged the packet length is used while merging the adjacent packets. If the padded bytes are accounted we would end up acking unsent TCP segments.

Signed-off-by Kumara Parameshwaran <[email protected]>

v1:
If there is padding to the ethernet frame cases where timestamp

is disabled

Quoted 1 line

the packet length should be validated with the total ip length

as packet length

Quoted 1 line

is used in the GRO merging logic

Please, can you test with current main branch?
We recently merged a fix:

https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403

Thanks, David. I did look at the patch. This would fix the problem, but

Quoted 6 lines

lets say for a case of an ACK packet where TCP data length would be zero and if the packet contains the padded bytes, this check should be done before the follwing check and not after this. @Hu, Jiayu <[email protected]> @Jun Qiu <[email protected]> please let me know your thoughts.

/*

Quoted 7 lines

* Don't process the packet whose payload length is less than or
* equal to 0.
*/
tcp_dl = pkt->pkt_len - hdr_len;
if (tcp_dl <= 0)
return -1;

--
David Marchand

#4 lore raw

Yes, It's better to do it before the tcp_dl check.

  1.  /* trim the tail padding bytes */
  2.  ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
  3.  if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
  4.      rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
  5.
  6.  /*
  7.   * Don't process the packet whose payload length is less than or
  8.   * equal to 0.
  9.   */

10. tcp_dl = pkt->pkt_len - hdr_len;
11. if (tcp_dl <= 0)
12. return -1;
From: kumaraparameshwaran rathinavel <[email protected]>
Sent: Tuesday, October 11, 2022 1:48 PM
To: [email protected]; David Marchand <[email protected]>; Hu, Jiayu <[email protected]>; Jun Qiu <[email protected]>
Subject: Fwd: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

On Tue, Oct 11, 2022 at 1:03 AM David Marchand <[email protected]<mailto:[email protected]>> wrote:

Quoted 7 lines · On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran…

From: Kumara Parameshwaran <[email protected]<mailto:[email protected]>>

When GRO packets are merged the packet length is used while merging the adjacent packets. If the padded bytes are accounted we would end up acking unsent TCP segments.

Quoted 5 lines
Signed-off-by Kumara Parameshwaran <[email protected]<mailto:[email protected]>>

v1:
If there is padding to the ethernet frame cases where timestamp is disabled the packet length should be validated with the total ip length as packet length is used in the GRO merging logic

Please, can you test with current main branch?
We recently merged a fix:
https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403
Thanks, David. I did look at the patch. This would fix the problem, but lets say for a case of an ACK packet where TCP data length would be zero and if the packet contains the padded bytes, this check should be done before the follwing check and not after this. @Hu, Jiayu<mailto:[email protected]> @Jun Qiu<mailto:[email protected]> please let me know your thoughts. /*
* Don't process the packet whose payload length is less than or
* equal to 0.
*/
tcp_dl = pkt->pkt_len - hdr_len;
if (tcp_dl <= 0)
return -1;
--
David Marchand

author
#5 lore raw

Shall I raise a PR with the change ?

Quoted 74 lines · Jun Qiu

Yes, It's better to do it before the tcp_dl check.

1. /* trim the tail padding bytes */
2. ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
3. *if* (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
4. rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
5.
6. /*

   7.  * Don't process the packet whose payload length is less than or
   8.  * equal to 0.
   9.  */

10. tcp_dl = pkt->pkt_len - hdr_len;
11. *if* (tcp_dl <= 0)
12. *return* -1;

*From:* kumaraparameshwaran rathinavel <[email protected]> *Sent:* Tuesday, October 11, 2022 1:48 PM
*To:* [email protected]; David Marchand <[email protected]>; Hu, Jiayu <[email protected]>; Jun Qiu <[email protected]> *Subject:* Fwd: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

On Tue, Oct 11, 2022 at 1:03 AM David Marchand <[email protected]> wrote:

Quoted 10 lines · Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>

When GRO packets are merged the packet length is used while merging the adjacent packets. If the padded bytes are accounted we would end up acking unsent TCP segments.

Signed-off-by Kumara Parameshwaran <[email protected]>

v1:
If there is padding to the ethernet frame cases where timestamp

is disabled

Quoted 1 line

the packet length should be validated with the total ip length

as packet length

Quoted 1 line

is used in the GRO merging logic

Please, can you test with current main branch?
We recently merged a fix:

https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403

Thanks, David. I did look at the patch. This would fix the problem, but lets say for a case of an ACK packet where TCP data length would be zero and if the packet contains the padded bytes, this check should be done before the follwing check and not after this. @Hu, Jiayu <[email protected]> @Jun Qiu <[email protected]> please let me know your thoughts.

/*
* Don't process the packet whose payload length is less than or
* equal to 0.
*/
tcp_dl = pkt->pkt_len - hdr_len;
if (tcp_dl <= 0)
return -1;

--
David Marchand

#6 lore raw
Quoted 2 lines · kumaraparameshwaran rathinavel

Shall I raise a PR with the change ?

Let's give some time to Jiayu and Jun to reply.
Thanks.

Signature · 1 line
David Marchand
#7 lore raw
In reply to Jun Qiu

Hi Kumara,

Agree. We need to trim padding bytes first, then do the length check.

Thanks,
Jiayu

From: Jun Qiu <[email protected]>
Sent: Wednesday, October 12, 2022 9:35 AM
To: kumaraparameshwaran rathinavel <[email protected]>; [email protected]; David Marchand <[email protected]>; Hu, Jiayu <[email protected]>
Subject: RE: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

Yes, It's better to do it before the tcp_dl check.

  1.  /* trim the tail padding bytes */
  2.  ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
  3.  if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
  4.      rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
  5.
  6.  /*
  7.   * Don't process the packet whose payload length is less than or
  8.   * equal to 0.
  9.   */

10. tcp_dl = pkt->pkt_len - hdr_len;
11. if (tcp_dl <= 0)
12. return -1;
From: kumaraparameshwaran rathinavel <[email protected]<mailto:[email protected]>>
Sent: Tuesday, October 11, 2022 1:48 PM
To: [email protected]<mailto:[email protected]>; David Marchand <[email protected]<mailto:[email protected]>>; Hu, Jiayu <[email protected]<mailto:[email protected]>>; Jun Qiu <[email protected]<mailto:[email protected]>>
Subject: Fwd: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

On Tue, Oct 11, 2022 at 1:03 AM David Marchand <[email protected]<mailto:[email protected]>> wrote:

Quoted 7 lines · On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran…

From: Kumara Parameshwaran <[email protected]<mailto:[email protected]>>

When GRO packets are merged the packet length is used while merging the adjacent packets. If the padded bytes are accounted we would end up acking unsent TCP segments.

Quoted 5 lines
Signed-off-by Kumara Parameshwaran <[email protected]<mailto:[email protected]>>

v1:
If there is padding to the ethernet frame cases where timestamp is disabled the packet length should be validated with the total ip length as packet length is used in the GRO merging logic

Please, can you test with current main branch?
We recently merged a fix:
https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403
Thanks, David. I did look at the patch. This would fix the problem, but lets say for a case of an ACK packet where TCP data length would be zero and if the packet contains the padded bytes, this check should be done before the follwing check and not after this. @Hu, Jiayu<mailto:[email protected]> @Jun Qiu<mailto:[email protected]> please let me know your thoughts. /*
* Don't process the packet whose payload length is less than or
* equal to 0.
*/
tcp_dl = pkt->pkt_len - hdr_len;
if (tcp_dl <= 0)
return -1;
--
David Marchand

author
#8 lore raw

Hi Jiayu,

Please find the patch
http://patches.dpdk.org/project/dpdk/patch/[email protected]/

I did reply with the above message ID but for some reason a new patch series was created. Not sure what was the mistake that I made. Should the commit message be the same for the series ?

Thanks,
Kumara.

Quoted 98 lines · Jiayu

Hi Kumara,

Agree. We need to trim padding bytes first, then do the length check.

Thanks,

Jiayu

*From:* Jun Qiu <[email protected]>
*Sent:* Wednesday, October 12, 2022 9:35 AM
*To:* kumaraparameshwaran rathinavel <[email protected]>; [email protected]; David Marchand <[email protected]>; Hu, Jiayu < [email protected]>
*Subject:* RE: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

Yes, It's better to do it before the tcp_dl check.

1. /* trim the tail padding bytes */
2. ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
3. *if* (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
4. rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
5.
6. /*

   7.  * Don't process the packet whose payload length is less than or
   8.  * equal to 0.
   9.  */

10. tcp_dl = pkt->pkt_len - hdr_len;
11. *if* (tcp_dl <= 0)
12. *return* -1;

*From:* kumaraparameshwaran rathinavel <[email protected]> *Sent:* Tuesday, October 11, 2022 1:48 PM
*To:* [email protected]; David Marchand <[email protected]>; Hu, Jiayu <[email protected]>; Jun Qiu <[email protected]> *Subject:* Fwd: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

On Tue, Oct 11, 2022 at 1:03 AM David Marchand <[email protected]> wrote:

Quoted 10 lines · Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>

When GRO packets are merged the packet length is used while merging the adjacent packets. If the padded bytes are accounted we would end up acking unsent TCP segments.

Signed-off-by Kumara Parameshwaran <[email protected]>

v1:
If there is padding to the ethernet frame cases where timestamp

is disabled

Quoted 1 line

the packet length should be validated with the total ip length

as packet length

Quoted 1 line

is used in the GRO merging logic

Please, can you test with current main branch?
We recently merged a fix:

https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403

Thanks, David. I did look at the patch. This would fix the problem, but lets say for a case of an ACK packet where TCP data length would be zero and if the packet contains the padded bytes, this check should be done before the follwing check and not after this. @Hu, Jiayu <[email protected]> @Jun Qiu <[email protected]> please let me know your thoughts.

/*
* Don't process the packet whose payload length is less than or
* equal to 0.
*/
tcp_dl = pkt->pkt_len - hdr_len;
if (tcp_dl <= 0)
return -1;

--
David Marchand

#9 lore raw
Quoted 11 lines · On Mon, 17 Oct 2022 18:57:44 +0530 kumaraparameshwaran rathinavel…

From: kumaraparameshwaran rathinavel <[email protected]>
To: "Hu, Jiayu" <[email protected]>
Cc: Jun Qiu <[email protected]>, "[email protected]" <[email protected]>, David Marchand <[email protected]>
Subject: Re: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame
Date: Mon, 17 Oct 2022 18:57:44 +0530

Hi Jiayu,

Please find the patch
http://patches.dpdk.org/project/dpdk/patch/[email protected]/

Quoted 6 lines

I did reply with the above message ID but for some reason a new patch series was created. Not sure what was the mistake that I made. Should the commit message be the same for the series ?

Thanks,
Kumara.

I don't see a conclusive reply in this mail thread.
And the patch doesn't apply to current main.
Therefore if the problem still exists, please rebase and resubmit.

author
#10 lore raw

PATCH v2 1/1 gro : check for payload length after the trim

In reply to Kumara Parameshwaran

From: Kumara Parameshwaran <[email protected]>

When packet is padded with extra bytes the the validation of the payload length should be done after the trim operation

Fixes b8a55871d5af ("gro: trim tail padding bytes")
Cc [email protected]
Signed-off-by Kumara Parameshwaran <[email protected]>

---
v1:
If there is padding to the ethernet frame cases where timestamp is disabled the packet length should be validated with the total ip length as packet length is used in the GRO merging logic

v2:
Trim the packet length and then check for the protocol payload validation

 lib/gro/gro_tcp4.c | 11 ++++++-----
 lib/gro/gro_udp4.c | 10 +++++-----
 2 files changed, 11 insertions(+), 10 deletions(-)
lib/gro/gro_tcp4.c +6 -5
index 8f5e800250..0014096e63 100644
@@ -225,6 +225,12 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	 */
 	if (tcp_hdr->tcp_flags != RTE_TCP_ACK_FLAG)
 		return -1;
+
+	/* trim the tail padding bytes */
+	ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
+	if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
+		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
+
 	/*
 	 * Don't process the packet whose payload length is less than or
 	 * equal to 0.

@@ -233,11 +239,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	if (tcp_dl <= 0)
 		return -1;
 
-	/* trim the tail padding bytes */
-	ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
-	if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
-		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
-
 	/*
 	 * Save IPv4 ID for the packet whose DF bit is 0. For the packet
 	 * whose DF bit is 1, IPv4 ID is ignored.
lib/gro/gro_udp4.c +5 -5
index 839f9748b7..42596d33b6 100644
@@ -220,6 +220,11 @@ gro_udp4_reassemble(struct rte_mbuf *pkt,
 	if (!is_ipv4_fragment(ipv4_hdr))
 		return -1;
 
+	ip_dl = rte_be_to_cpu_16(ipv4_hdr->total_length);
+	/* trim the tail padding bytes */
+	if (pkt->pkt_len > (uint32_t)(ip_dl + pkt->l2_len))
+		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_dl - pkt->l2_len);
+
 	/*
 	 * Don't process the packet whose payload length is less than or
 	 * equal to 0.

@@ -227,14 +232,9 @@ gro_udp4_reassemble(struct rte_mbuf *pkt,
 	if (pkt->pkt_len <= hdr_len)
 		return -1;
 
-	ip_dl = rte_be_to_cpu_16(ipv4_hdr->total_length);
 	if (ip_dl <= pkt->l3_len)
 		return -1;
 
-	/* trim the tail padding bytes */
-	if (pkt->pkt_len > (uint32_t)(ip_dl + pkt->l2_len))
-		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_dl - pkt->l2_len);
-
 	ip_dl -= pkt->l3_len;
 	ip_id = rte_be_to_cpu_16(ipv4_hdr->packet_id);
 	frag_offset = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
Signature · 1 line
2.25.1
Acked-by on v2 1/1
#11 lore raw

RE: [PATCH v2] gro : check for payload length after the trim

Quoted 94 lines

-----Original Message-----
From: Kumara Parameshwaran <[email protected]>
Sent: Sunday, October 16, 2022 10:43 PM
To: Hu, Jiayu <[email protected]>
Cc: [email protected]; Kumara Parameshwaran <[email protected]>; [email protected]
Subject: [PATCH v2] gro : check for payload length after the trim

From: Kumara Parameshwaran <[email protected]>

When packet is padded with extra bytes the the validation of the payload length should be done after the trim operation

Fixes b8a55871d5af ("gro: trim tail padding bytes")
Cc [email protected]
Signed-off-by Kumara Parameshwaran <[email protected]>

---
v1:
If there is padding to the ethernet frame cases where timestamp is
disabled
the packet length should be validated with the total ip length as
packet length
is used in the GRO merging logic

v2:
Trim the packet length and then check for the protocol payload
validation lib/gro/gro_tcp4.c | 11 ++++++----- lib/gro/gro_udp4.c | 10
+++++-----

 2 files changed, 11 insertions(+), 10 deletions(-)
lib/gro/gro_tcp4.c index renamed from lib/gro/gro_tcp4.c +0 -0

8f5e800250..0014096e63 100644

lib/gro/gro_tcp4.c +6 -5
@@ -225,6 +225,12 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	 */
 	if (tcp_hdr->tcp_flags != RTE_TCP_ACK_FLAG)
 		return -1;
+
+	/* trim the tail padding bytes */
+	ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
+	if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
+		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
+
 	/*
 	 * Don't process the packet whose payload length is less than or
 	 * equal to 0.

@@ -233,11 +239,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	if (tcp_dl <= 0)
 		return -1;
 
-	/* trim the tail padding bytes */
-	ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
-	if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
-		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
-
 	/*
 	 * Save IPv4 ID for the packet whose DF bit is 0. For the packet
 	 * whose DF bit is 1, IPv4 ID is ignored.
lib/gro/gro_udp4.c index renamed from lib/gro/gro_udp4.c +0 -0

839f9748b7..42596d33b6 100644

lib/gro/gro_udp4.c +5 -6
@@ -220,6 +220,11 @@ gro_udp4_reassemble(struct rte_mbuf *pkt,
 	if (!is_ipv4_fragment(ipv4_hdr))
 		return -1;
 
+	ip_dl = rte_be_to_cpu_16(ipv4_hdr->total_length);
+	/* trim the tail padding bytes */
+	if (pkt->pkt_len > (uint32_t)(ip_dl + pkt->l2_len))
+		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_dl - pkt->l2_len);
+
 	/*
 	 * Don't process the packet whose payload length is less than or
 	 * equal to 0.

@@ -227,14 +232,9 @@ gro_udp4_reassemble(struct rte_mbuf *pkt,
 	if (pkt->pkt_len <= hdr_len)
 		return -1;
 
-	ip_dl = rte_be_to_cpu_16(ipv4_hdr->total_length);
 	if (ip_dl <= pkt->l3_len)
 		return -1;
 
-	/* trim the tail padding bytes */
-	if (pkt->pkt_len > (uint32_t)(ip_dl + pkt->l2_len))
-		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_dl - pkt->l2_len);
-
 	ip_dl -= pkt->l3_len;
 	ip_id = rte_be_to_cpu_16(ipv4_hdr->packet_id);
 	frag_offset = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
--
Quoted 1 line

2.25.1

Acked-by Jiayu Hu <[email protected]>

Thanks,
Jiayu