From: "Deng, KaiwenX" <kaiwenx.deng@intel.com>
To: "Ferruh Yigit" <ferruh.yigit@amd.com>,
"Stephen Hemminger" <stephen@networkplumber.org>,
"Morten Brørup" <mb@smartsharesystems.com>,
"Zhang, Qi Z" <qi.z.zhang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"stable@dpdk.org" <stable@dpdk.org>,
"Yang, Qiming" <qiming.yang@intel.com>,
"Zhou, YidingX" <yidingx.zhou@intel.com>,
"Singh, Aman Deep" <aman.deep.singh@intel.com>,
"Zhang, Yuying" <yuying.zhang@intel.com>,
"Matz, Olivier" <olivier.matz@6wind.com>,
"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>
Subject: RE: [PATCH] app/test-pmd: fix L4 checksum with padding data
Date: Wed, 22 Nov 2023 03:04:12 +0000 [thread overview]
Message-ID: <SA3PR11MB800386BFD8832098FC5F02AD8EBAA@SA3PR11MB8003.namprd11.prod.outlook.com> (raw)
In-Reply-To: <da4d7a66-2bd2-4a48-a192-377cc81ceb7e@amd.com>
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Monday, November 20, 2023 6:46 PM
> To: Deng, KaiwenX <kaiwenx.deng@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Morten Brørup
> <mb@smartsharesystems.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>;
> Zhou, YidingX <yidingx.zhou@intel.com>; Singh, Aman Deep
> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>;
> Matz, Olivier <olivier.matz@6wind.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Subject: Re: [PATCH] app/test-pmd: fix L4 checksum with padding data
>
> On 11/20/2023 9:21 AM, Deng, KaiwenX wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Friday, November 17, 2023 8:50 AM
> >> To: Stephen Hemminger <stephen@networkplumber.org>
> >> Cc: Deng, KaiwenX <kaiwenx.deng@intel.com>; dev@dpdk.org;
> >> stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> >> <yidingx.zhou@intel.com>; Singh, Aman Deep
> >> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>;
> >> Matz, Olivier <olivier.matz@6wind.com>; De Lara Guarch, Pablo
> >> <pablo.de.lara.guarch@intel.com>
> >> Subject: Re: [PATCH] app/test-pmd: fix L4 checksum with padding data
> >>
> >> On 11/16/2023 10:58 PM, Stephen Hemminger wrote:
> >>> On Thu, 2 Nov 2023 19:20:07 +0000
> >>> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>>
> >>>> On 8/4/2023 9:28 AM, Kaiwen Deng wrote:
> >>>>> IEEE 802 packets may have a minimum size limit. The data fields
> >>>>> should be padded when necessary. In some cases, the padding data
> >>>>> is not zero. Testpmd does not trim these IP packets to the true
> >>>>> length of the frame, so errors will occur when calculating TCP or
> >>>>> UDP checksum.
> >>>>>
> >>>>
> >>>> Hi Kaiwen,
> >>>>
> >>>> I am trying to understand the problem, what is the testcase that
> >>>> has checksum error?
> >>>>
> >>>> Are the received mbuf data_len & pkt_len wrong? Instead of trying
> >>>> to fix the mbuf during forwarding, can we fix where packet generated?
> >>>
> >>> The root cause is that get_udptcp_cksum_mbuf is using m->pkt_len
> >>> which maybe larger than the actual data. The real issue is there and
> >>> in rte_ip.h checksum code. The correct fix would be to use l3_len instead.
> >>>
> >>
> >> I see, you are right.
> >>
> >> In 'rte_ipv4_udptcp_cksum_mbuf()',
> >> as payload length "mbuf->pkt_len - l4_off" is used, which includes
> >> padding and if padding is not zero it will end up producing wrong
> checksum.
> >>
> >>
> >> I agree using 'l3_len' instead is correct fix.
> >>
> >> But this requires ABI/API change,
> >> plus do we have any reason to keep the padding, discarding it as this
> >> patch does is also simpler alternative.
> >>
> >>
> >> Other alternative can be to zero the padding bytes. I guess standard
> >> doesn't enforce them to be zero, but we can do this to remove its
> >> impact on checksum calculation.
> > I'm not sure if this is ok, it feels like it would reduce performance.
> > I can try this alternative if needed.
> >
>
> Yes impacts performance, so not a good alternative, please scratch it.
>
> As discussion with Stephen and Morten, consensus is to fix SW functions that
> calculates checksum.
> 'rte_ipv4_udptcp_cksum_mbuf()' & 'rte_ipv6_udptcp_cksum_mbuf()'.
>
> Instead of using packet_len, those functions can use packet length, which will
> make the checksum correct.
>
>
> Can you please send a patch to fix those functions? I think this can be done
> without changing function fingerprint, so without causing any ABI/API break.
>
>
> >>
> >>
> >> @Kaiwen, did you able to test this with HW offload, what is the
> >> behavior of the HW, does is remove padding bytes?
> >>
> > I tested the HW offload case and the same tcp/udp checksum error
> > occurs when padding is not 0, But if change pkt_len to the true length of the
> frame, the checksum is correct.
> >
>
> I was expecting HW not impacted, since padding is part of the spec, my
> assumption would be HW only take the actual payload size into account,
> instead of buffer size.
>
> Can you please double check? Which HW you are testing with, can you please
> add maintainer of that HW to this discussion?
I've tested hw offloads for udp and tcp with Intel E810. The hardware takes the size
of the buffer into account when calculating the udp/tcp checksum, not the size of the
actual payload.
Hi @Zhang, Qi Z,
Can you help confirm if this behavior is normal in hw?
Thanks!
>
> If HW requires padding to be removed, we may go with your solution.
>
>
> >>
> >>> It also looks like test-pmd is not validating the IP header.
> >>> Both parse_ipv4() and parse_ipv6() should check if packet was truncated.
> >>> Same for both UDP and TCP lengths.
> >>>
> >>
> >
next prev parent reply other threads:[~2023-11-22 3:04 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-04 8:28 Kaiwen Deng
2023-11-02 19:20 ` Ferruh Yigit
2023-11-03 2:49 ` Deng, KaiwenX
2023-11-03 4:03 ` Ferruh Yigit
2023-11-14 2:19 ` Deng, KaiwenX
2023-11-14 19:09 ` Ferruh Yigit
2023-11-16 7:02 ` Deng, KaiwenX
2023-11-16 22:58 ` Stephen Hemminger
2023-11-17 0:50 ` Ferruh Yigit
2023-11-17 3:28 ` Stephen Hemminger
2023-11-17 9:29 ` Ferruh Yigit
2023-11-17 12:11 ` Morten Brørup
2023-11-17 16:23 ` Stephen Hemminger
2023-11-17 16:22 ` Stephen Hemminger
2023-11-20 10:47 ` Ferruh Yigit
2023-11-20 9:21 ` Deng, KaiwenX
2023-11-20 10:46 ` Ferruh Yigit
2023-11-22 3:04 ` Deng, KaiwenX [this message]
2023-11-17 1:13 ` Ferruh Yigit
2023-11-20 9:52 ` Deng, KaiwenX
2023-12-07 8:53 ` [PATCH v2] app/test-pmd: fix tcp/udp cksum " Kaiwen Deng
2023-12-07 14:35 ` Ferruh Yigit
2023-12-12 2:16 ` [PATCH v3] lib/net: " Kaiwen Deng
2023-12-12 8:10 ` Morten Brørup
2023-12-13 4:37 ` [PATCH v4] " Kaiwen Deng
2023-12-13 7:36 ` Morten Brørup
2023-12-14 9:22 ` [PATCH v5] " Kaiwen Deng
2023-12-14 11:20 ` Morten Brørup
2024-02-19 1:10 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SA3PR11MB800386BFD8832098FC5F02AD8EBAA@SA3PR11MB8003.namprd11.prod.outlook.com \
--to=kaiwenx.deng@intel.com \
--cc=aman.deep.singh@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=mb@smartsharesystems.com \
--cc=olivier.matz@6wind.com \
--cc=pablo.de.lara.guarch@intel.com \
--cc=qi.z.zhang@intel.com \
--cc=qiming.yang@intel.com \
--cc=stable@dpdk.org \
--cc=stephen@networkplumber.org \
--cc=yidingx.zhou@intel.com \
--cc=yuying.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).