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

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

[Kumara Parameshwaran]

From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>

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 <kumaraparamesh92@gmail.com>
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(-)

diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
index 7498c66141..ac6439bb4e 100644
--- a/lib/gro/gro_tcp4.c
+++ b/lib/gro/gro_tcp4.c
@@ -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.
-- 
2.25.1

Re: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

[David Marchand]

On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
<kumaraparamesh92@gmail.com> wrote:
>
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
>
> 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 <kumaraparamesh92@gmail.com>
> 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


-- 
David Marchand

Fwd: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

[kumaraparameshwaran rathinavel]

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

On Tue, Oct 11, 2022 at 1:03 AM David Marchand <david.marchand@redhat.com>
wrote:

> On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
> <kumaraparamesh92@gmail.com> wrote:
> >
> > From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> >
> > 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 <kumaraparamesh92@gmail.com>
> > 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
>> <jiayu.hu@intel.com>  @Jun Qiu <jun.qiu@jaguarmicro.com>  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
>
>

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

RE: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

[Jun Qiu]

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

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 <kumaraparamesh92@gmail.com>
Sent: Tuesday, October 11, 2022 1:48 PM
To: dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu <jiayu.hu@intel.com>; Jun Qiu <jun.qiu@jaguarmicro.com>
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 <david.marchand@redhat.com<mailto:david.marchand@redhat.com>> wrote:
On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
<kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>> wrote:
>
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
>
> 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 <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
> 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:jiayu.hu@intel.com>  @Jun Qiu<mailto:jun.qiu@jaguarmicro.com>  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

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

Re: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

[kumaraparameshwaran rathinavel]

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

Shall I raise a PR with the change ?


On Wed, Oct 12, 2022 at 7:04 AM Jun Qiu <jun.qiu@jaguarmicro.com> wrote:

> 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 <kumaraparamesh92@gmail.com>
> *Sent:* Tuesday, October 11, 2022 1:48 PM
> *To:* dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Jun Qiu <jun.qiu@jaguarmicro.com>
> *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 <david.marchand@redhat.com>
> wrote:
>
> On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
> <kumaraparamesh92@gmail.com> wrote:
> >
> > From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> >
> > 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 <kumaraparamesh92@gmail.com>
> > 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
> <jiayu.hu@intel.com>  @Jun Qiu <jun.qiu@jaguarmicro.com>  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
>
>

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

Re: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

[David Marchand]

On Wed, Oct 12, 2022 at 9:48 AM kumaraparameshwaran rathinavel
<kumaraparamesh92@gmail.com> wrote:
>
> Shall I raise a PR with the change ?

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

-- 
David Marchand

RE: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

[Hu, Jiayu]

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

Hi Kumara,

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

Thanks,
Jiayu

From: Jun Qiu <jun.qiu@jaguarmicro.com>
Sent: Wednesday, October 12, 2022 9:35 AM
To: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>; dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu <jiayu.hu@intel.com>
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 <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
Sent: Tuesday, October 11, 2022 1:48 PM
To: dev@dpdk.org<mailto:dev@dpdk.org>; David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>>; Hu, Jiayu <jiayu.hu@intel.com<mailto:jiayu.hu@intel.com>>; Jun Qiu <jun.qiu@jaguarmicro.com<mailto:jun.qiu@jaguarmicro.com>>
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 <david.marchand@redhat.com<mailto:david.marchand@redhat.com>> wrote:
On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
<kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>> wrote:
>
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
>
> 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 <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
> 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:jiayu.hu@intel.com>  @Jun Qiu<mailto:jun.qiu@jaguarmicro.com>  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

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

Re: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

[kumaraparameshwaran rathinavel]

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

Hi Jiayu,

Please find the patch
http://patches.dpdk.org/project/dpdk/patch/20221016144305.19249-1-kumaraparmesh92@gmail.com/

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.

On Sun, Oct 16, 2022 at 12:31 PM Hu, Jiayu <jiayu.hu@intel.com> wrote:

> Hi Kumara,
>
>
>
> Agree. We need to trim padding bytes first, then do the length check.
>
>
>
> Thanks,
>
> Jiayu
>
>
>
> *From:* Jun Qiu <jun.qiu@jaguarmicro.com>
> *Sent:* Wednesday, October 12, 2022 9:35 AM
> *To:* kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>;
> dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu <
> jiayu.hu@intel.com>
> *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 <kumaraparamesh92@gmail.com>
> *Sent:* Tuesday, October 11, 2022 1:48 PM
> *To:* dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Jun Qiu <jun.qiu@jaguarmicro.com>
> *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 <david.marchand@redhat.com>
> wrote:
>
> On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
> <kumaraparamesh92@gmail.com> wrote:
> >
> > From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> >
> > 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 <kumaraparamesh92@gmail.com>
> > 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
> <jiayu.hu@intel.com>  @Jun Qiu <jun.qiu@jaguarmicro.com>  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
>
>

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

Re: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

[Stephen Hemminger]

On Mon, 17 Oct 2022 18:57:44 +0530
kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com> wrote:

> From: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
> To: "Hu, Jiayu" <jiayu.hu@intel.com>
> Cc: Jun Qiu <jun.qiu@jaguarmicro.com>, "dev@dpdk.org" <dev@dpdk.org>,   David Marchand <david.marchand@redhat.com>
> 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/20221016144305.19249-1-kumaraparmesh92@gmail.com/
> 
> 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.

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

[Kumara Parameshwaran]

From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>

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: stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
---
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(-)

diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
index 8f5e800250..0014096e63 100644
--- a/lib/gro/gro_tcp4.c
+++ b/lib/gro/gro_tcp4.c
@@ -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.
diff --git a/lib/gro/gro_udp4.c b/lib/gro/gro_udp4.c
index 839f9748b7..42596d33b6 100644
--- a/lib/gro/gro_udp4.c
+++ b/lib/gro/gro_udp4.c
@@ -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);
-- 
2.25.1

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

[Hu, Jiayu]

> -----Original Message-----
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> Sent: Sunday, October 16, 2022 10:43 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Kumara Parameshwaran
> <kumaraparamesh92@gmail.com>; stable@dpdk.org
> Subject: [PATCH v2] gro : check for payload length after the trim
> 
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> 
> 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: stable@dpdk.org
> 
> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> ---
> 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(-)
> 
> diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c index
> 8f5e800250..0014096e63 100644
> --- a/lib/gro/gro_tcp4.c
> +++ b/lib/gro/gro_tcp4.c
> @@ -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.
> diff --git a/lib/gro/gro_udp4.c b/lib/gro/gro_udp4.c index
> 839f9748b7..42596d33b6 100644
> --- a/lib/gro/gro_udp4.c
> +++ b/lib/gro/gro_udp4.c
> @@ -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);
> --
> 2.25.1

Acked-by: Jiayu Hu <Jiayu.hu@intel.com>

Thanks,
Jiayu

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

[Thomas Monjalon]

> > From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> > 
> > 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: stable@dpdk.org
> > 
> > Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> 
> Acked-by: Jiayu Hu <Jiayu.hu@intel.com>

Applied, thanks.