Re: [PATCH net-next v1 02/15] psp: take the psp nexthdr from the packet on encap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mlx5e: psp software GSO on tunneled skbs bypasses
  psp_dev_encap_ok()
--

commit 1d6df14f6316e027ee333bf0c262d7f9842ccd5c
Author: Todd Freed <toddfreed@meta.com>

psp: take the psp nexthdr from the packet on encap

This commit populates the PSP next header from the original IP header in
psp_dev_encapsulate() instead of hardcoding IPPROTO_TCP, rejects packets
that cannot be encapsulated (IPv6 extension headers, IPv4 fragments, and
already-encapsulated packets), and updates mlx5e_features_check() to fall
back to software GSO and checksumming for unsupported protocols.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index c3c2c5720c6f3..d091de1a5992b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

[ ... ]

> @@ -5140,6 +5157,22 @@ netdev_features_t mlx5e_features_check(struct sk_buff *skb,
>  
>  	features = vlan_features_check(skb, features);
>  
> +	/* The PSP tx path fixes up the TCP checksum of a GSO skb for the
> +	 * device, which is only valid for TCP, and the device only completes
> +	 * a TCP or UDP checksum behind the PSP header. Have the stack segment
> +	 * and checksum other skbs.
> +	 */
> +	if ((skb_is_gso(skb) || skb->ip_summed == CHECKSUM_PARTIAL) &&
> +	    mlx5e_psp_is_offload(skb, netdev)) {
> +		if (skb_is_gso(skb) &&
> +		    !(skb_shinfo(skb)->gso_type &
> +		      (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))
> +			features &= ~NETIF_F_GSO_MASK;
> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
> +		    !mlx5e_psp_l4_csum_ok(skb))
> +			features &= ~NETIF_F_CSUM_MASK;
> +	}

[Severity: Medium]
Can clearing NETIF_F_CSUM_MASK and NETIF_F_GSO_MASK on an already
encapsulated GSO skb (skb->encapsulation == 1) cause software tunnel GSO
to strip skb->encapsulation and bypass the check in psp_dev_encap_ok()?

For a PSP-offloaded GSO packet encapsulated in a tunnel such as GRE,
mlx5e_psp_l4_csum_ok() sees IPPROTO_GRE and clears NETIF_F_CSUM_MASK,
which also clears NETIF_F_GSO_MASK in harmonize_features().
validate_xmit_skb() then runs software tunnel GSO (gre_gso_segment()),
which completes the inner checksums in software, leaves seg->encapsulation
as 0, points seg->transport_header at the outer GRE header right after the
outer IP header, and for SKB_GSO_GRE_CSUM sets seg->ip_summed to
CHECKSUM_PARTIAL.

When those segments reach mlx5e_psp_handle_tx_skb() ->
psp_dev_encapsulate(), psp_dev_encap_ok() sees seg->encapsulation == 0 and
network_len == ip_hdrlen(seg), so it encapsulates the outer GRE packet
with nexthdr = IPPROTO_GRE instead of dropping it.  In the mlx5e TX path,
mlx5e_psp_set_swp() leaves eseg->swp_inner_l4_offset as 0 for IPPROTO_GRE
while mlx5e_psp_txwqe_build_eseg_csum() still sets
MLX5_ETH_WQE_L4_INNER_CSUM because seg->ip_summed == CHECKSUM_PARTIAL,
producing corrupted frames or hardware TX descriptor syndromes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111005533.6436-1-toddfreed@meta.com?part=2