Currently, rte_ipv4_cksum() and rte_ipv4_udptcp_cksum() assume all IPv4 headers have sizeof(struct rte_ipv4_hdr) bytes. This is not true for those (rare) packets with IPv4 options. Thus, both IPv4 and TCP/UDP checksums are calculated wrong. This patch fixes the issue by using the actual IPv4 header length from the packet's IHL field. Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de> --- lib/librte_net/rte_ip.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index fcd1eb342..6c3ff2603 100644 --- a/lib/librte_net/rte_ip.h +++ b/lib/librte_net/rte_ip.h @@ -269,7 +269,7 @@ static inline uint16_t rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr) { uint16_t cksum; - cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr)); + cksum = rte_raw_cksum(ipv4_hdr, (ipv4_hdr->version_ihl & 0xf) * 4); return (uint16_t)~cksum; } @@ -311,7 +311,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags) } else { psd_hdr.len = rte_cpu_to_be_16( (uint16_t)(rte_be_to_cpu_16(ipv4_hdr->total_length) - - sizeof(struct rte_ipv4_hdr))); + - ((ipv4_hdr->version_ihl & 0xf) * 4))); } return rte_raw_cksum(&psd_hdr, sizeof(psd_hdr)); } @@ -319,8 +319,8 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags) /** * Process the IPv4 UDP or TCP checksum. * - * The IPv4 header should not contains options. The IP and layer 4 - * checksum must be set to 0 in the packet by the caller. + * 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. @@ -339,7 +339,7 @@ rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr) if (l3_len < sizeof(struct rte_ipv4_hdr)) return 0; - l4_len = l3_len - sizeof(struct rte_ipv4_hdr); + l4_len = l3_len - ((ipv4_hdr->version_ihl & 0xf) * 4); cksum = rte_raw_cksum(l4_hdr, l4_len); cksum += rte_ipv4_phdr_cksum(ipv4_hdr, 0); -- 2.28.0
On 8/21/20 2:32 PM, Michael Pfeiffer wrote: > Currently, rte_ipv4_cksum() and rte_ipv4_udptcp_cksum() assume all IPv4 > headers have sizeof(struct rte_ipv4_hdr) bytes. This is not true for > those (rare) packets with IPv4 options. Thus, both IPv4 and TCP/UDP > checksums are calculated wrong. > > This patch fixes the issue by using the actual IPv4 header length from > the packet's IHL field. > > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de> > --- > lib/librte_net/rte_ip.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h > index fcd1eb342..6c3ff2603 100644 > --- a/lib/librte_net/rte_ip.h > +++ b/lib/librte_net/rte_ip.h > @@ -269,7 +269,7 @@ static inline uint16_t > rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr) > { > uint16_t cksum; > - cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr)); > + cksum = rte_raw_cksum(ipv4_hdr, (ipv4_hdr->version_ihl & 0xf) * 4); > return (uint16_t)~cksum; > } > > @@ -311,7 +311,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags) > } else { > psd_hdr.len = rte_cpu_to_be_16( > (uint16_t)(rte_be_to_cpu_16(ipv4_hdr->total_length) > - - sizeof(struct rte_ipv4_hdr))); > + - ((ipv4_hdr->version_ihl & 0xf) * 4))); > } > return rte_raw_cksum(&psd_hdr, sizeof(psd_hdr)); > } > @@ -319,8 +319,8 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags) > /** > * Process the IPv4 UDP or TCP checksum. > * > - * The IPv4 header should not contains options. The IP and layer 4 > - * checksum must be set to 0 in the packet by the caller. > + * 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. > @@ -339,7 +339,7 @@ rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr) > if (l3_len < sizeof(struct rte_ipv4_hdr)) > return 0; > > - l4_len = l3_len - sizeof(struct rte_ipv4_hdr); > + l4_len = l3_len - ((ipv4_hdr->version_ihl & 0xf) * 4); Previous if condition guarantees that the result will be not negative (and make huge l4_len), so it looks like it should be updated as well. I think it makes sense to introduce additional variable with calculated IPv4 header length. > > cksum = rte_raw_cksum(l4_hdr, l4_len); > cksum += rte_ipv4_phdr_cksum(ipv4_hdr, 0); >
Currently, rte_ipv4_cksum() and rte_ipv4_udptcp_cksum() assume all IPv4 headers have sizeof(struct rte_ipv4_hdr) bytes. This is not true for those (rare) packets with IPv4 options. Thus, both IPv4 and TCP/UDP checksums are calculated wrong. This patch fixes the issue by using the actual IPv4 header length from the packet's IHL field. Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de> --- v2: * Use actual header length for sanity check as well. * Introduce ip_hdr_len variable to increase readability. lib/librte_net/rte_ip.h | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index fcd1eb342..bb55ebb6f 100644 --- a/lib/librte_net/rte_ip.h +++ b/lib/librte_net/rte_ip.h @@ -269,7 +269,7 @@ static inline uint16_t rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr) { uint16_t cksum; - cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr)); + cksum = rte_raw_cksum(ipv4_hdr, (ipv4_hdr->version_ihl & 0xf) * 4); return (uint16_t)~cksum; } @@ -302,6 +302,9 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags) uint16_t len; /* L4 length. */ } psd_hdr; + uint32_t l3_len; + uint8_t ip_hdr_len; + psd_hdr.src_addr = ipv4_hdr->src_addr; psd_hdr.dst_addr = ipv4_hdr->dst_addr; psd_hdr.zero = 0; @@ -309,9 +312,9 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags) if (ol_flags & PKT_TX_TCP_SEG) { psd_hdr.len = 0; } else { - psd_hdr.len = rte_cpu_to_be_16( - (uint16_t)(rte_be_to_cpu_16(ipv4_hdr->total_length) - - sizeof(struct rte_ipv4_hdr))); + l3_len = rte_be_to_cpu_16(ipv4_hdr->total_length); + ip_hdr_len = (ipv4_hdr->version_ihl & 0xf) * 4; + psd_hdr.len = rte_cpu_to_be_16((uint16_t)(l3_len - ip_hdr_len)); } return rte_raw_cksum(&psd_hdr, sizeof(psd_hdr)); } @@ -319,8 +322,8 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags) /** * Process the IPv4 UDP or TCP checksum. * - * The IPv4 header should not contains options. The IP and layer 4 - * checksum must be set to 0 in the packet by the caller. + * 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. @@ -334,12 +337,14 @@ rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr) { uint32_t cksum; uint32_t l3_len, l4_len; + uint8_t ip_hdr_len; + ip_hdr_len = (ipv4_hdr->version_ihl & 0xf) * 4; l3_len = rte_be_to_cpu_16(ipv4_hdr->total_length); - if (l3_len < sizeof(struct rte_ipv4_hdr)) + if (l3_len < ip_hdr_len) return 0; - l4_len = l3_len - sizeof(struct rte_ipv4_hdr); + l4_len = l3_len - ip_hdr_len; cksum = rte_raw_cksum(l4_hdr, l4_len); cksum += rte_ipv4_phdr_cksum(ipv4_hdr, 0); -- 2.28.0
On 9/1/20 12:47 PM, Michael Pfeiffer wrote:
> Currently, rte_ipv4_cksum() and rte_ipv4_udptcp_cksum() assume all IPv4
> headers have sizeof(struct rte_ipv4_hdr) bytes. This is not true for
> those (rare) packets with IPv4 options. Thus, both IPv4 and TCP/UDP
> checksums are calculated wrong.
>
> This patch fixes the issue by using the actual IPv4 header length from
> the packet's IHL field.
>
> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> - cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
> + cksum = rte_raw_cksum(ipv4_hdr, (ipv4_hdr->version_ihl & 0xf) * 4);
Truly naive questions:
- doesn't it deserve a static inline function rte_ipv4_hdr_len()?
- how generated code for "* 4" compares with "<< 2"?
On Tue, 06 Oct 2020 00:55:19 +0200 Thomas Monjalon <thomas@monjalon.net> wrote: > > - cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr)); > > + cksum = rte_raw_cksum(ipv4_hdr, (ipv4_hdr->version_ihl & 0xf) * 4); > > Truly naive questions: > - doesn't it deserve a static inline function rte_ipv4_hdr_len()? Makes sense to have that. > - how generated code for "* 4" compares with "<< 2"? The code is the same if value is unsigned.
On Mon, Oct 05, 2020 at 07:39:45PM -0700, Stephen Hemminger wrote: > On Tue, 06 Oct 2020 00:55:19 +0200 > Thomas Monjalon <thomas@monjalon.net> wrote: > > > > On 9/1/20 12:47 PM, Michael Pfeiffer wrote: > > > Currently, rte_ipv4_cksum() and rte_ipv4_udptcp_cksum() assume all IPv4 > > > headers have sizeof(struct rte_ipv4_hdr) bytes. This is not true for > > > those (rare) packets with IPv4 options. Thus, both IPv4 and TCP/UDP > > > checksums are calculated wrong. > > > > > > This patch fixes the issue by using the actual IPv4 header length from > > > the packet's IHL field. > > > > > > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de> Acked-by: Olivier Matz <olivier.matz@6wind.com> > > > - cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr)); > > > + cksum = rte_raw_cksum(ipv4_hdr, (ipv4_hdr->version_ihl & 0xf) * 4); > > > > Truly naive questions: > > - doesn't it deserve a static inline function rte_ipv4_hdr_len()? > > Makes sense to have that. +1 However it could be in another patch: there are ~15 places where it could be replaced in dpdk.
Hi,
On Tue, 2020-10-06 at 10:10 +0200, Olivier Matz wrote:
> On Mon, Oct 05, 2020 at 07:39:45PM -0700, Stephen Hemminger wrote:
> > On Tue, 06 Oct 2020 00:55:19 +0200
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > On 9/1/20 12:47 PM, Michael Pfeiffer wrote:
> > > > Currently, rte_ipv4_cksum() and rte_ipv4_udptcp_cksum() assume all IPv4
> > > > headers have sizeof(struct rte_ipv4_hdr) bytes. This is not true for
> > > > those (rare) packets with IPv4 options. Thus, both IPv4 and TCP/UDP
> > > > checksums are calculated wrong.
> > > >
> > > > This patch fixes the issue by using the actual IPv4 header length from
> > > > the packet's IHL field.
> > > >
> > > > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>
> > > > - cksum = rte_raw_cksum(ipv4_hdr, sizeof(struct rte_ipv4_hdr));
> > > > + cksum = rte_raw_cksum(ipv4_hdr, (ipv4_hdr->version_ihl & 0xf) *
> > > > 4);
> > >
> > > Truly naive questions:
> > > - doesn't it deserve a static inline function rte_ipv4_hdr_len()?
> >
> > Makes sense to have that.
>
> +1
>
> However it could be in another patch: there are ~15 places where it
> could be replaced in dpdk.
Thank you very much for your feedback. I will prepare a second patch for that
function.
06/10/2020 10:10, Olivier Matz: > On Mon, Oct 05, 2020 at 07:39:45PM -0700, Stephen Hemminger wrote: > > On Tue, 06 Oct 2020 00:55:19 +0200 > > Thomas Monjalon <thomas@monjalon.net> wrote: > > > > > > On 9/1/20 12:47 PM, Michael Pfeiffer wrote: > > > > Currently, rte_ipv4_cksum() and rte_ipv4_udptcp_cksum() assume all IPv4 > > > > headers have sizeof(struct rte_ipv4_hdr) bytes. This is not true for > > > > those (rare) packets with IPv4 options. Thus, both IPv4 and TCP/UDP > > > > checksums are calculated wrong. > > > > > > > > This patch fixes the issue by using the actual IPv4 header length from > > > > the packet's IHL field. > > > > > > > > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de> > > Acked-by: Olivier Matz <olivier.matz@6wind.com> Applied, thanks > > > - doesn't it deserve a static inline function rte_ipv4_hdr_len()? > > > > Makes sense to have that. > > +1 > > However it could be in another patch: there are ~15 places where it > could be replaced in dpdk. OK thanks for working on the improvement.