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 7530CA0547; Wed, 12 Oct 2022 09:48:37 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 69E4042E00; Wed, 12 Oct 2022 09:48:37 +0200 (CEST) Received: from mail-vk1-f172.google.com (mail-vk1-f172.google.com [209.85.221.172]) by mails.dpdk.org (Postfix) with ESMTP id 33CC242BF0 for ; Wed, 12 Oct 2022 09:48:36 +0200 (CEST) Received: by mail-vk1-f172.google.com with SMTP id m128so7770059vka.12 for ; Wed, 12 Oct 2022 00:48:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=CmNVdHdJu7EJeDdHdwZxRZb7g8NhYfhSA6f3+aNGPMs=; b=P1pJ8/aUkmBofMLt8XB1EaQkK7VJzIVpdNGL2OCVBAxWcfCNM14SP+hymr2kDCEpnd sQjpeyGDUmLVLvoGpt3LpP/43AoDyJnDceg7pX+q/8EcEfPpKZp6iTaNveNSo4wi2uq/ LJCkmKQ9uXVWKb93UBOR9I2qIv4SD4nFMWAQe2SCPi07b0WGa4ArnM0dyAp7I6T3vI+h iUIY1HMSVs2CiHkjaxG9OI0lqySCXf/3YG1TZORHXfRrvOEJflWDaYDXZ/nixB/uwUa0 +AU+uSxmHthneTcVJFSrTg5KK4jUF9I8SymjK7jIcmWH2GJmhqZh7HQ/kjvwrnGUjgjW 1AZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=CmNVdHdJu7EJeDdHdwZxRZb7g8NhYfhSA6f3+aNGPMs=; b=SzqAEJkerKH7aCjGaxM2FSmiHVL0Pwq+k/cJt8bV2HtuHPCaqai/xKrT03VnXQu5uy aIonMwiydOwqajR54WpzXdC6ls5JVvpKD7Nu8xyU7zEzbrxUEJGGfHSaQ1SlJwvkxm95 LNBeT7muLivFEuF6ROjEw3CANUayxCBtvSaWmsgBx4bfRwz6nmmA1nXtESNKQRsbKhP5 tkflMFVe7NAOJI9YaZTgNWcsLcz65YlU7U5i2MZTdA+AJ84tzsqAQ4L+z1i4s8G9Hpo5 xltdFJqXRB/oNP5rul+bUzPuhNF8j73x0UXHUwX9ITNwnU2hWCkkdUbt6buoqKr+dcWC hj2w== X-Gm-Message-State: ACrzQf2v4vrpjFpvy2abUngtuXWjlF/NiWAD+gPngRYAGCaXdiBkmynX 0kTGEcb5z2OtC4f4Q/wkhzjISXsRJDQSRQTT60A= X-Google-Smtp-Source: AMsMyM4CUqky9FvjU+0EvINl6IvPBiyANFXG3k8fG2Kt8+7vgtHZuXCgS3wXTDm/iSz1ODTY6vYYfIYt+RtD4h4qiYE= X-Received: by 2002:a1f:adca:0:b0:3ab:b862:2042 with SMTP id w193-20020a1fadca000000b003abb8622042mr7678707vke.37.1665560915587; Wed, 12 Oct 2022 00:48:35 -0700 (PDT) MIME-Version: 1.0 References: <20221010175109.44282-1-kumaraparmesh92@gmail.com> In-Reply-To: From: kumaraparameshwaran rathinavel Date: Wed, 12 Oct 2022 13:18:23 +0530 Message-ID: Subject: Re: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame To: Jun Qiu Cc: "dev@dpdk.org" , David Marchand , "Hu, Jiayu" Content-Type: multipart/alternative; boundary="000000000000b9b5d505ead19ece" 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 --000000000000b9b5d505ead19ece Content-Type: text/plain; charset="UTF-8" Shall I raise a PR with the change ? On Wed, Oct 12, 2022 at 7:04 AM Jun Qiu 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 > *Sent:* Tuesday, October 11, 2022 1:48 PM > *To:* dev@dpdk.org; David Marchand ; Hu, Jiayu > ; Jun Qiu > *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 > wrote: > > On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran > wrote: > > > > From: Kumara Parameshwaran > > > > 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 > > 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 > @Jun Qiu 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 > > --000000000000b9b5d505ead19ece Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
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, =C2=A0It's better to do it before the tcp_dl check.

=C2=A0

  1. /*=C2=A0trim=C2=A0the=C2=A0tail=C2=A0padding=C2=A0bytes= =C2=A0*/=C2=A0=C2= =A0
  2. ip_tlen=C2=A0=3D=C2=A0rte_be_to_c= pu_16(ipv4_hdr->total_length);=C2=A0=C2=A0
  3. if=C2=A0(pkt->pkt_len=C2=A0>=C2=A0(uint32_t)(ip_tlen=C2=A0+= =C2=A0pkt->l2_len))=C2=A0=C2=A0
  4. =C2=A0=C2=A0=C2=A0=C2=A0rte_pktmb= uf_trim(pkt,=C2=A0pkt->pkt_len=C2=A0-=C2=A0ip_tlen=C2=A0-=C2=A0pkt->l= 2_len);=C2=A0=C2=A0
  5. =C2=A0=C2=A0
  6. /*=C2=A0
  7. =C2=A0*=C2=A0Don't=C2=A0process=C2=A0the=C2=A0packe= t=C2=A0whose=C2=A0payload=C2=A0length=C2=A0is=C2=A0less=C2=A0than=C2=A0or=C2=A0
  8. =C2=A0*=C2=A0equal=C2=A0to=C2=A00.=C2=A0
  9. =C2=A0*/=C2=A0=C2=A0
  10. tcp_dl=C2=A0=3D=C2=A0pkt->pkt_= len=C2=A0-=C2=A0hdr_len;=C2=A0=C2=A0
  11. if=C2=A0(tcp_dl=C2=A0<=3D=C2=A00)=C2=A0=C2=A0
  12. =C2=A0=C2=A0=C2=A0=C2=A0return=C2=A0-1;=C2=A0=C2=A0

From: kuma= raparameshwaran 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 Q= iu <jun.qiu= @jaguarmicro.com>
Subject: Fwd: [PATCH] gro : fix pkt length when extra bytes are padd= ed to ethernet frame

=C2=A0

=C2=A0

=C2=A0

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

On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran <kumarap= aramesh92@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:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0If there is padding to the ethernet f= rame cases where timestamp is disabled
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0the packet length should be validated= with the total ip length as packet length
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0is 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=3Db= 8a55871d5af6f5b8694b1cb5eacbc629734e403

Th= anks, 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 c= heck and not after this. @Hu, Jiayu=C2= =A0 @Jun Qiu=C2=A0 please let me know your thoughts.=C2=A0

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

--
David Marchand

--000000000000b9b5d505ead19ece--