From: Ferruh Yigit <ferruh.yigit@linux.intel.com>
To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
Olivier Matz <olivier.matz@6wind.com>
Cc: "Ferruh Yigit" <ferruh.yigit@intel.com>,
dev@dpdk.org, "Keith Wiles" <keith.wiles@intel.com>,
"Hongzhi Guo" <guohongzhi1@huawei.com>,
"Morten Brørup" <mb@smartsharesystems.com>,
"Thomas Monjalon" <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums
Date: Fri, 25 Jun 2021 16:38:22 +0100 [thread overview]
Message-ID: <a99a44b5-4958-aa11-d6fa-43e93a4be547@linux.intel.com> (raw)
In-Reply-To: <875c88f5-e76c-867f-2fbb-16d5d4c662bc@oktetlabs.ru>
On 6/8/2021 1:39 PM, Andrew Rybchenko wrote:
> On 6/8/21 3:29 PM, Olivier Matz wrote:
>> Hi Ferruh, Andrew,
>>
>> On Tue, Jun 08, 2021 at 01:23:33PM +0300, Andrew Rybchenko wrote:
>>> On 4/30/21 6:42 PM, Ferruh Yigit wrote:
>>>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
>>>>> Since commit d5df2ae0428a ("net: fix unneeded replacement of TCP
>>>>> checksum 0"), the functions rte_ipv4_udptcp_cksum() and
>>>>> rte_ipv6_udptcp_cksum() can return either 0x0000 or 0xffff when used to
>>>>> verify a packet containing a valid checksum.
>>>>>
>>>>> Since these functions should be used to calculate the checksum to set in
>>>>> a packet, introduce 2 new helpers for checksum verification. They return
>>>>> 0 if the checksum is valid in the packet.
>>>>>
>>>>> Use this new helper in net/tap driver.
>>>>>
>>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>>> ---
>>>>> drivers/net/tap/rte_eth_tap.c | 7 +-
>>>>> lib/net/rte_ip.h | 124 +++++++++++++++++++++++++++-------
>>>>> 2 files changed, 104 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>>> index 71282e8065..b14d5a1d55 100644
>>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>>> @@ -365,11 +365,12 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>> return;
>>>>> }
>>>>> }
>>>>> - cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>>>> + cksum_ok = !rte_ipv4_udptcp_cksum_verify(l3_hdr,
>>>>> + l4_hdr);
>>>>> } else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>>>>> - cksum = rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>>>> + cksum_ok = !rte_ipv6_udptcp_cksum_verify(l3_hdr,
>>>>> + l4_hdr);
>>>>> }
>>>>> - cksum_ok = (cksum == 0) || (cksum == 0xffff);
>>>>> mbuf->ol_flags |= cksum_ok ?
>>>>> PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
>>>>> }
>>>>> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
>>>>> index 8c189009b0..ef84bcc5bf 100644
>>>>> --- a/lib/net/rte_ip.h
>>>>> +++ b/lib/net/rte_ip.h
>>>>> @@ -344,20 +344,10 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags)
>>>>> }
>>>>>
>>>>> /**
>>>>> - * Process the IPv4 UDP or TCP checksum.
>>>>> - *
>>>>> - * The IP and layer 4 checksum must be set to 0 in the packet by
>>>>> - * the caller.
>>>>> - *
>>>>> - * @param ipv4_hdr
>>>>> - * The pointer to the contiguous IPv4 header.
>>>>> - * @param l4_hdr
>>>>> - * The pointer to the beginning of the L4 header.
>>>>> - * @return
>>>>> - * The complemented checksum to set in the IP packet.
>>>>> + * @internal Calculate the non-complemented IPv4 L4 checksum
>>>>> */
>>>>> static inline uint16_t
>>>>> -rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
>>>>> +__rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
>>>>> {
>>>>> uint32_t cksum;
>>>>> uint32_t l3_len, l4_len;
>>>>> @@ -374,16 +364,62 @@ rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
>>>>> cksum += rte_ipv4_phdr_cksum(ipv4_hdr, 0);
>>>>>
>>>>> cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff);
>>>>> - cksum = (~cksum) & 0xffff;
>>>>> +
>>>>> + return (uint16_t)cksum;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * Process the IPv4 UDP or TCP checksum.
>>>>> + *
>>>>> + * The IP and layer 4 checksum must be set to 0 in the packet by
>>>>> + * the caller.
>>>>> + *
>>>>> + * @param ipv4_hdr
>>>>> + * The pointer to the contiguous IPv4 header.
>>>>> + * @param l4_hdr
>>>>> + * The pointer to the beginning of the L4 header.
>>>>> + * @return
>>>>> + * The complemented checksum to set in the IP packet.
>>>>> + */
>>>>> +static inline uint16_t
>>>>> +rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
>>>>> +{
>>>>> + uint16_t cksum = __rte_ipv4_udptcp_cksum(ipv4_hdr, l4_hdr);
>>>>> +
>>>>> + cksum = ~cksum;
>>>>> +
>>>>> /*
>>>>> - * Per RFC 768:If the computed checksum is zero for UDP,
>>>>> + * Per RFC 768: If the computed checksum is zero for UDP,
>>>>> * it is transmitted as all ones
>>>>> * (the equivalent in one's complement arithmetic).
>>>>> */
>>>>> if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
>>>>> cksum = 0xffff;
>>>>>
>>>>> - return (uint16_t)cksum;
>>>>> + return cksum;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * Validate the IPv4 UDP or TCP checksum.
>>>>> + *
>>>>> + * @param ipv4_hdr
>>>>> + * The pointer to the contiguous IPv4 header.
>>>>> + * @param l4_hdr
>>>>> + * The pointer to the beginning of the L4 header.
>>>>> + * @return
>>>>> + * Return 0 if the checksum is correct, else -1.
>>>>> + */
>>>>> +__rte_experimental
>>>>> +static inline int
>>>>> +rte_ipv4_udptcp_cksum_verify(const struct rte_ipv4_hdr *ipv4_hdr,
>>>>> + const void *l4_hdr)
>>>>> +{
>>>>> + uint16_t cksum = __rte_ipv4_udptcp_cksum(ipv4_hdr, l4_hdr);
>>>>> +
>>>>> + if (cksum != 0xffff)
>>>>> + return -1;
>>>>> +
>>>>> + return 0;
>>>>
>>>> There is behavior change in tap verify, I am asking just to verify if expected,
>>>>
>>>> Previously for UDP, if calculated checksum is '0', the 'rte_ipv4_udptcp_cksum()'
>>>> returns 0xFFFF.
>>>> And 0xFFFF is taken as good checksum by tap PMD.
>>
>> rte_ipv4_udptcp_cksum() cannot return 0xFFFF: this is only possible if
>> all data is 0. Before verifying a udp packet, the user must check that
>> it is not 0 (which means no checksum). In tcp, "Data offset" is never
>> 0. Moreover, port=0 is a reserved value for both udp and tcp.
>>
>>>> With new 'rte_ipv4_udptcp_cksum_verify()', if calculated checksum is '0' it will
>>>> be taken as bad checksum.
>>>>
>>>> I don't know if calculated checksum with valid checksum in place can return 0.
>>>>
>>>>
>>>> Also for TCP, 'rte_ipv4_udptcp_cksum_verify()' doesn't have inversion (cksum =
>>>> ~cksum;) seems changing pass/fail status of the checksum, unless I am not
>>>> missing anything here.
>>>
>>> Yes, it looks suspicious to me as well.
>>>
>>> Olivier, could you clarify, please.
>>
>> Before commit d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0"),
>> the behavior was:
>>
>> // rte_ipv4_udptcp_cksum() is 0xffff if checksum is valid
>> // so cksum is 0 if checksum is valid
>> cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>> // ol_flags is set to PKT_RX_L4_CKSUM_GOOD if checksum is valid
>> mbuf->ol_flags |= cksum ? PKT_RX_L4_CKSUM_BAD : PKT_RX_L4_CKSUM_GOOD;
>>
>> After commit d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0"),
>> it is broken:
>>
>> // rte_ipv4_udptcp_cksum() is 0 (tcp) or 0xffff (udp) if checksum is valid
>> // so cksum is 0xffff (tcp) or 0 (udp) if checksum is valid
>> cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>> // ol_flags is set to BAD (tcp) or GOOD (udp) if checksum is valid
>> mbuf->ol_flags |= cksum ? PKT_RX_L4_CKSUM_BAD : PKT_RX_L4_CKSUM_GOOD;
>>
>> After patch 2/4 ("net/tap: fix Rx cksum flags on TCP packets"), the
>> correct behavior is restored:
>>
>> // cksum is 0 (tcp) or 0xffff (udp) if checksum is valid
>> // note: 0xffff for tcp cannot happen (there is at least 1 bit set in the header)
>> // note: 0 for udp cannot happen (it is replaced by in rte_ipv4_udptcp_cksum())
>> cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>> // cksum_ok is 1 if checksum is valid
>> cksum_ok = (cksum == 0) || (cksum == 0xffff);
>> // ol_flags is set to GOOD if checksum is valid
>> mbuf->ol_flags |= cksum_ok ? PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
>>
>> After this patch [3/4] ("net: introduce functions to verify L4 checksums"),
>> it is simplified by using rte_ipv4_udptcp_cksum_verify():
>>
>> // cksum_ok is 1 if checksum is valid
>> cksum_ok = !rte_ipv4_udptcp_cksum_verify(l3_hdr, l4_hdr);
>> // ol_flags is set to GOOD if checksum is valid
>> mbuf->ol_flags |= cksum_ok ? PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
>>
>
> Many thanks for the detailed explanations. It replies to all my
> questions (even not asked, but kept in my head).
>
Thanks for clarification, after checking again with help of description above,
it looks good to me.
next prev parent reply other threads:[~2021-06-25 15:38 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-27 13:57 [dpdk-dev] [PATCH 0/4] net/tap: fix Rx cksum Olivier Matz
2021-04-27 13:57 ` [dpdk-dev] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets Olivier Matz
2021-04-30 14:48 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2021-06-08 10:13 ` Andrew Rybchenko
2021-06-08 12:29 ` Olivier Matz
2021-06-08 12:34 ` Andrew Rybchenko
2021-06-08 12:49 ` Olivier Matz
2021-06-08 13:57 ` Andrew Rybchenko
2021-06-08 14:30 ` Olivier Matz
2021-04-27 13:57 ` [dpdk-dev] [PATCH 2/4] net/tap: fix Rx cksum flags on TCP packets Olivier Matz
2021-06-08 10:18 ` Andrew Rybchenko
2021-04-27 13:57 ` [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums Olivier Matz
2021-04-27 15:02 ` Morten Brørup
2021-04-27 15:07 ` Morten Brørup
2021-04-28 12:21 ` Olivier Matz
2021-04-28 12:42 ` Morten Brørup
2021-04-30 15:42 ` Ferruh Yigit
2021-06-08 10:23 ` Andrew Rybchenko
2021-06-08 12:29 ` Olivier Matz
2021-06-08 12:39 ` Andrew Rybchenko
2021-06-25 15:38 ` Ferruh Yigit [this message]
2021-04-27 13:57 ` [dpdk-dev] [PATCH 4/4] test/cksum: new test for L3/L4 checksum API Olivier Matz
2021-06-30 13:51 ` [dpdk-dev] [PATCH v2 0/4] net/tap: fix Rx cksum Olivier Matz
2021-06-30 13:51 ` [dpdk-dev] [PATCH v2 1/4] net/tap: fix Rx cksum flags on IP options packets Olivier Matz
2021-06-30 13:51 ` [dpdk-dev] [PATCH v2 2/4] net/tap: fix Rx cksum flags on TCP packets Olivier Matz
2021-06-30 13:51 ` [dpdk-dev] [PATCH v2 3/4] net: introduce functions to verify L4 checksums Olivier Matz
2021-06-30 13:51 ` [dpdk-dev] [PATCH v2 4/4] test/cksum: new test for L3/L4 checksum API Olivier Matz
2021-07-01 9:28 ` [dpdk-dev] [PATCH v2 0/4] net/tap: fix Rx cksum Andrew Rybchenko
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=a99a44b5-4958-aa11-d6fa-43e93a4be547@linux.intel.com \
--to=ferruh.yigit@linux.intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=guohongzhi1@huawei.com \
--cc=keith.wiles@intel.com \
--cc=mb@smartsharesystems.com \
--cc=olivier.matz@6wind.com \
--cc=thomas@monjalon.net \
/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).