From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 506ACA034C; Mon, 18 Jul 2022 07:06:43 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2EC404069F; Mon, 18 Jul 2022 07:06:43 +0200 (CEST) Received: from mail-ua1-f45.google.com (mail-ua1-f45.google.com [209.85.222.45]) by mails.dpdk.org (Postfix) with ESMTP id AAB6740041 for ; Mon, 18 Jul 2022 07:06:41 +0200 (CEST) Received: by mail-ua1-f45.google.com with SMTP id s3so4846904uaq.2 for ; Sun, 17 Jul 2022 22:06:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vURc1zsAljvtP6Mjp/sG9+MbMPuTXqIjohlo7aTer6o=; b=g7AIGfX+Jiy0Ds6WnTwRcZYLG3a6ljvxujFcld4MaNIpl9DQwpzBgwpPdWHA82tEhP SFFLP/f7oRJegMDCm2UbDmv+l1uRo7t/1eEcHD6aPfzLSUGlqM+RutaJft6P4jKHJhYZ pGlPBrBDRUnK0d3tmSsa7yJI3wa/iQeu41eKEsYZY6QAE6J2yYSfEZtWkvYZCZH7GBNH iVgcuunz99DOYxGIiiYu0fVQUtAJsldybkP9oHJ/m5UgBABKiBnd8x7zUCqv40s1/9y8 RtRSRanTKL3lSLHk8OBMe4z+XMVGn5PzOaZQUzZkfNZNdKER+K+Tkd/1wsQ4hJ/0W1kO dMAg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vURc1zsAljvtP6Mjp/sG9+MbMPuTXqIjohlo7aTer6o=; b=0E/an8dBJEwJQQr5+HGrCTLnustdCr5+65gauiAYz/KrjYob6RCqyes2oLaI6GqOJI BnXcfPojEzp9OEoMfGatlWrjvgWFhqunGO5t9GhiTYBl9ORv5RY7N7mfiLcycghtxX7p ZW6VblnWprITeybNGtZUgxtLrorHWletKCyMET4omN33DBg/BHZBvSImDrp8bw6lRIiS CgfprRrqznlpIQvSaehRPyuSeLMU0IQHacQDNI074MszCLPTgdEGfXbn/zBcPecNdnQ4 dWRq7excuwlFvqM5Hf1cs/CYB9aZ6KKVhCKiWgAxAioIGT2Zp2WwDMTcSmcgel5rxwyW CE5w== X-Gm-Message-State: AJIora+VNnpSDZ7eBZUyVMFtzfP++XmAuNji7GJxqjdUrC4SXyQVW7cA lNhTg0pQP80d9E2cHz+rDzD6tdOPkeO+x1CxtkXBZ/bC X-Google-Smtp-Source: AGRyM1vVNJVWdoK/bx234Tk79vieLjoR0xpg+LO8rU6xKL7rp8fOAbcb70nn3V7HnXcVnyTsoPrHOWFTEmedRWgWZUY= X-Received: by 2002:a9f:2f07:0:b0:37f:163a:904a with SMTP id x7-20020a9f2f07000000b0037f163a904amr8564520uaj.82.1658120800983; Sun, 17 Jul 2022 22:06:40 -0700 (PDT) MIME-Version: 1.0 References: <20220403115031.59632-1-kumaraparamesh92@gmail.com> In-Reply-To: From: kumaraparameshwaran rathinavel Date: Mon, 18 Jul 2022 10:36:29 +0530 Message-ID: Subject: Re: [PATCH v1] gro: bug fix in identifying 0 length tcp packets To: "Hu, Jiayu" Cc: Thomas Monjalon , mb@smartsharesystems.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="00000000000056607b05e40d55b0" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --00000000000056607b05e40d55b0 Content-Type: text/plain; charset="UTF-8" Hi Jiayu, Can you please suggest if the patch can be used ? When timestamps are disabled we would unnecessarily indulge pure TCP ack packets in the GRO layer. Or can we have a fix where if the TCP timestamp option is not present in the packet, do not process the packet, return immediately ? Thanks, Param On Mon, Apr 25, 2022 at 11:36 PM kumaraparameshwaran rathinavel < kumaraparamesh92@gmail.com> wrote: > Hi, > > I would like you to review this patch and let me know what you think of > it. > > Thanks, > Kumara. > > ---------- Forwarded message --------- > From: Kumara Parameshwaran > Date: Sun, Apr 3, 2022 at 5:20 PM > Subject: [PATCH v1] gro: bug fix in identifying 0 length tcp packets > To: > Cc: , Kumara Parameshwaran , < > stable@dpdk.org>, Kumara Parameshwaran > > > As the minimum Ethernet frame size is 64 bytes, a 0 length > tcp payload without tcp options would be 54 bytes and hence > there would be padding. So it would be incorrect to use the > packet length to determine the tcp data length. > > Fixes: 1e4cf4d6d4fb ("gro: cleanup") > Cc: stable@dpdk.org > > Signed-off-by: Kumara Parameshwaran > --- > v1: > Do not use packet length to determine the tcp data length as > the packet length could have padded bytes. This would lead > to addition of 0 length tcp packets into the GRO layer when > there ethernet fram is padded. > lib/gro/gro_tcp4.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c > index 7498c66..45e3f48 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, frag_off; > uint8_t is_atomic; > > struct tcp4_flow_key key; > @@ -217,7 +217,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt, > eth_hdr = rte_pktmbuf_mtod(pkt, struct rte_ether_hdr *); > ipv4_hdr = (struct rte_ipv4_hdr *)((char *)eth_hdr + pkt->l2_len); > tcp_hdr = (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len); > - hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len; > > /* > * Don't process the packet which has FIN, SYN, RST, PSH, URG, ECE > @@ -229,7 +228,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt, > * Don't process the packet whose payload length is less than or > * equal to 0. > */ > - tcp_dl = pkt->pkt_len - hdr_len; > + tcp_dl = rte_be_to_cpu_16(ipv4_hdr->total_length) - (pkt->l3_len + > pkt->l4_len); > if (tcp_dl <= 0) > return -1; > > -- > 2.7.4 > > --00000000000056607b05e40d55b0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Jiayu,
Can you please=C2=A0 suggest if t= he patch can be used ? When timestamps are disabled we would unnecessarily = indulge pure TCP ack packets in the GRO layer. Or can we have a fix where i= f the TCP timestamp option is not present in the packet, do not process the= packet, return immediately ?

Thanks,
Param

On Mon, Apr 25, 2022 at 11:36 PM kumaraparameshwaran rath= inavel <kumaraparamesh92@g= mail.com> wrote:
Hi,

I would lik= e you to review this patch and let me know what you think of it.
=

Thanks,
Kumara.

---------- Forwarded= message ---------
From: Kuma= ra Parameshwaran <kumaraparamesh92@gmail.com><= br>Date: Sun, Apr 3, 2022 at 5:20 PM
Subject: [PATCH v1] gro: bug fix in= identifying 0 length tcp packets
To: <jiayu.hu@intel.com>
Cc: <dev@dpdk.org>, Kumara Par= ameshwaran <kumaraparamesh92@gmail.com>, <stable@dpdk.org>, Kumara Parameshwaran <kparameshwar@vmw= are.com>


As the minimum Ethernet frame size is 64 b= ytes, a 0 length
tcp payload without tcp options would be 54 bytes and hence
there would be padding. So it would be incorrect to use the
packet length to determine the tcp data length.

Fixes: 1e4cf4d6d4fb ("gro: cleanup")
Cc: stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
v1:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Do not use packet length to determine the tcp d= ata length as
=C2=A0 =C2=A0 =C2=A0 =C2=A0 the packet length could have padded bytes. This= would lead
=C2=A0 =C2=A0 =C2=A0 =C2=A0 to addition of 0 length tcp packets into the GR= O layer when
=C2=A0 =C2=A0 =C2=A0 =C2=A0 there ethernet fram is padded.
=C2=A0lib/gro/gro_tcp4.c | 5 ++---
=C2=A01 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
index 7498c66..45e3f48 100644
--- a/lib/gro/gro_tcp4.c
+++ b/lib/gro/gro_tcp4.c
@@ -198,7 +198,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct rte_tcp_hdr *tcp_hdr;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 uint32_t sent_seq;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 int32_t tcp_dl;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0uint16_t ip_id, hdr_len, frag_off;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0uint16_t ip_id, frag_off;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 uint8_t is_atomic;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct tcp4_flow_key key;
@@ -217,7 +217,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 eth_hdr =3D rte_pktmbuf_mtod(pkt, struct rte_et= her_hdr *);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 ipv4_hdr =3D (struct rte_ipv4_hdr *)((char *)et= h_hdr + pkt->l2_len);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 tcp_hdr =3D (struct rte_tcp_hdr *)((char *)ipv4= _hdr + pkt->l3_len);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0hdr_len =3D pkt->l2_len + pkt->l3_len + p= kt->l4_len;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Don't process the packet which has = FIN, SYN, RST, PSH, URG, ECE
@@ -229,7 +228,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Don't process the packet whose payl= oad length is less than or
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* equal to 0.
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
-=C2=A0 =C2=A0 =C2=A0 =C2=A0tcp_dl =3D pkt->pkt_len - hdr_len;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0tcp_dl =3D rte_be_to_cpu_16(ipv4_hdr->total_= length) - (pkt->l3_len + pkt->l4_len);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (tcp_dl <=3D 0)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -1;

--
2.7.4

--00000000000056607b05e40d55b0--