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 70E21A0560; Mon, 17 Oct 2022 15:27:59 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 150C04021D; Mon, 17 Oct 2022 15:27:59 +0200 (CEST) Received: from mail-vs1-f47.google.com (mail-vs1-f47.google.com [209.85.217.47]) by mails.dpdk.org (Postfix) with ESMTP id 042D540143 for ; Mon, 17 Oct 2022 15:27:57 +0200 (CEST) Received: by mail-vs1-f47.google.com with SMTP id h4so11428640vsr.11 for ; Mon, 17 Oct 2022 06:27:57 -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=oJ8ShGs4rd75Oa91VN4ojRMbzSqREhSLpyAnU1Dp3mA=; b=C14c35VzguX6N8DnwwjqmE6sTVuY/MorXDHMBa4PymsIENNxestvd3NGhfKkZAzC9M mcaBzGNmqfnJZLEUIjb1EOHe9yEm26yi85+xCO663Kc+vn3DYvwNQnlPXBRqnrRnKqO6 sm+dx73fKivOz8Lv8PyQ9QPHJ7sZDtGzWnO24KfP3v6vFFO5RWdoPAF9t3R9mMVdej2Q BonTq9NP3e+r2jsnbrSTIos/pQFqRvjuJnA3HIOkuqI7mSm8BXemt3wBsjaEu/JUp3fr XDURsFUT76PPAEwKhhs5DFYhcplDW28X85W0/RMLSR4hvXsdye5A3s9/2me8tvqbeEb+ tvlw== 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=oJ8ShGs4rd75Oa91VN4ojRMbzSqREhSLpyAnU1Dp3mA=; b=fwkQ4Mv7pc6brfmLym+5Yx09KsFtzfYFf8ZEmSaAY0xZztPqtqtZ5fNbcRSebZMlER LQHKceIQgOQ8ntHIH4X98fxq+Cz9faoIbtVD+f7XIgxBlbprPPWHS9R5WPEU0CQw4JLi R6R1t8a3DwZk26P6hCOrXftcxa8kFrCVW6GpgbaB3FnTJBAKbZ8KgZe+XXKulI8n10Fi 17NX3VZTo7cGIIcXGfF4RJgwucSTQuXlZL/0KZxAGoPgksAjg7DFMGt6QvXf5s5XtZIv Bz/d6KRUV7TxRMefUucb+44Hzkclyg6GFueYci5+nXMWY3udH1XX3SW7WGPrs3rBBA1w +CTA== X-Gm-Message-State: ACrzQf2uT7Q9J/1OxTGotos4k9hm4IoM1ofXMFQuiXfttk6VojcxqjwI VT1Xd48KYmpa7HPxGwYAnAjvNordQgMP6V90ndI= X-Google-Smtp-Source: AMsMyM6Y3Jmzuk/KHgcdO8/h8d1kppooFaeXK1luzIMRH7u81tF5zNau/Xu9kBFxHYrQ3/8+vO8qjJ1G40Y8ZO8L+4w= X-Received: by 2002:a05:6102:34d9:b0:3a7:80b9:20b7 with SMTP id a25-20020a05610234d900b003a780b920b7mr4057751vst.46.1666013276530; Mon, 17 Oct 2022 06:27:56 -0700 (PDT) MIME-Version: 1.0 References: <20221010175109.44282-1-kumaraparmesh92@gmail.com> In-Reply-To: From: kumaraparameshwaran rathinavel Date: Mon, 17 Oct 2022 18:57:44 +0530 Message-ID: Subject: Re: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame To: "Hu, Jiayu" Cc: Jun Qiu , "dev@dpdk.org" , David Marchand Content-Type: multipart/alternative; boundary="00000000000089ed8b05eb3af185" 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 --00000000000089ed8b05eb3af185 Content-Type: text/plain; charset="UTF-8" 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 wrote: > Hi Kumara, > > > > Agree. We need to trim padding bytes first, then do the length check. > > > > Thanks, > > Jiayu > > > > *From:* Jun Qiu > *Sent:* Wednesday, October 12, 2022 9:35 AM > *To:* kumaraparameshwaran rathinavel ; > dev@dpdk.org; David Marchand ; 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 > *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 > > --00000000000089ed8b05eb3af185 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Jiayu,


=
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 ?

<= div>Thanks,
Kumara.

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

Hi Kumara,

=C2=A0

Agree. We need to trim padding bytes first, then do t= he length check.

=C2=A0

Thanks,

Jiayu

=C2=A0

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 March= and <davi= d.marchand@redhat.com>; Hu, Jiayu <jiayu.hu@intel.com>
Subject: RE: [PATCH] gro : fix pkt length when extra bytes are padde= d to ethernet frame

=C2=A0

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_cpu_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<= /u>
  4. =C2=A0=C2=A0=C2=A0=C2=A0rte_pktmbuf_trim(pkt,=C2= =A0pkt->pkt_len=C2=A0-=C2=A0ip_tlen=C2=A0-=C2=A0pkt->l2_len);=C2=A0= =C2=A0<= /u>
  5. =C2=A0=C2=A0
  6. /*=C2=A0
  7. =C2=A0*=C2=A0Don't=C2=A0process=C2=A0the=C2=A0packet=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. =
  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<= /span>=C2=A0-1;=C2=A0=C2=A0
    13. <= /ol>

From: kumaraparameshwaran rathinavel <= ;kumarapara= mesh92@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> wrote:

On M= on, 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

Thanks, David. I did lo= ok 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 t= he padded bytes, this check should be done before the follwing check 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

-- <= br> David Marchand

--00000000000089ed8b05eb3af185--