Re: [PATCH net v3 1/4] net/mlx5e: psp: tso: replace only the length in checksum seed

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Commit message dependency.

--

net/mlx5e: psp: tso: replace only the length in checksum seed

This patch changes mlx5e_psp_handle_tx_skb() so that, for PSP TSO packets,
it adjusts only the length part of the TCP checksum seed instead of
recomputing the seed from the l3 header addresses. The goal is to avoid
bad checksums when the l3 header daddr is an intermediate hop.

> the length difference is expected by the device. however, the
> difference in addresses is incorrect in cases where the l3 header daddr
> is an intermediate hop (e.g. IPV6_RTHDR). this causes every segment of
> the tso packet to go out with a bad tcp checksum, in this case.

[Severity: Low]
The code change in mlx5e_psp_handle_tx_skb() looks correct. However, is
IPV6_RTHDR the right example for this patch on its own?

At this point in the series, psp_dev_encapsulate() still writes the
nexthdr of the base IPv6 header unconditionally:

net/psp/psp_main.c:psp_dev_encapsulate() {
    ...
	} else {
		ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;
		be16_add_cpu(&ipv6_hdr(skb)->payload_len, PSP_ENCAP_HLEN);
	}
    ...
}

With a routing header present, this overwrites NEXTHDR_ROUTING in the base
header, while the UDP+PSP headers go in after the routing header. That
means the packet is already malformed before the TCP checksum seed
matters. So this patch alone doesn't appear to fix the IPv6 RTHDR case.

The IPv6 case seems to be fixed later in the series by "psp: support ipv6
extension headers in psp_dev_encapsulate()", which has its own Fixes: tag.
The only mention of that dependency here is the testing note:

> tested on connectx-7 with the rest of this series applied, with a psp
> tcp socket whose segment routing header's first hop isn't the final
> destination, and the peer checking tcp checksums in software.

The case this patch fixes by itself looks like IPv4 source routing
(IP_OPTIONS with SRR/LSRR). There, ip_hdr(skb)->daddr is the first hop,
but tcp_v4_send_check() seeded the checksum with inet->inet_daddr.

Could the commit message mention the IPv4 SRR/LSRR case? Could it also
say that the IPv6 RTHDR case depends on the psp_dev_encapsulate() patch?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023234640.1459291-1-toddfreed%40meta.com