Re: [PATCH net v2 1/2] psp: fix transport mode encap with ipv6 extension headers

[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] The updated kernel-doc comment on the exported
  psp_dev_encapsulate() says "With IPv6, they go after any extension…

--

psp: fix transport mode encap with ipv6 extension headers

This patch changes psp_dev_encapsulate() to walk the IPv6 extension
header chain. It now rewrites the nexthdr of the last header in the chain
to UDP, instead of always rewriting the fixed header. Fragment, routing,
NEXTHDR_NONE and unknown header types are rejected, and psp_dev_rcv()
does the reverse on receive using ipv6_skip_exthdr().

> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index c9c1a8826b7f9..0c009d7b08c00 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c

[ ... ]

> +static int psp_ipv6_nexthdr_off(const u8 *hdr, unsigned int len)
> +{

[ ... ]

> +		if (!ipv6_ext_hdr(type) || type == NEXTHDR_NONE ||
> +		    type == NEXTHDR_FRAGMENT || type == NEXTHDR_ROUTING)
> +			return -1;

[ ... ]

> @@ -243,7 +280,7 @@ static void psp_write_headers(struct net *net, struct sk_buff *skb, __be32 spi,
>  }
>  
>  /* Encapsulate a TCP packet with PSP by adding the UDP+PSP headers and filling
> - * them in.
> + * them in. With IPv6, they go after any extension headers.
>   */
>  bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  			 u8 ver, __be16 sport)
> @@ -251,10 +288,17 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  	u32 network_len = skb_network_header_len(skb);
>  	u32 ethr_len = skb_mac_header_len(skb);
>  	u32 bufflen = ethr_len + network_len;
> -
> -	if (skb->protocol != htons(ETH_P_IP) &&
> -	    skb->protocol != htons(ETH_P_IPV6))
> +	int nh_off = 0;
> +
> +	if (skb->protocol == htons(ETH_P_IPV6)) {
> +		/* UDP + PSP after the extension headers, if any */
> +		nh_off = psp_ipv6_nexthdr_off(skb_network_header(skb),
> +					      network_len);
> +		if (nh_off < 0)
> +			return false;

[Severity: Low]
This isn't a bug, but do the comments on psp_dev_encapsulate() describe
more than the code accepts?

The kernel-doc says "With IPv6, they go after any extension headers", and
the inline comment says "UDP + PSP after the extension headers, if any".
However, psp_ipv6_nexthdr_off() rejects routing, fragment, NEXTHDR_NONE
and unknown header types. For those, psp_dev_encapsulate() returns false.

Callers such as mlx5e_psp_handle_tx_skb() and nsim_do_psp() drop the skb
in that case. In the selftest, a socket with IPV6_RTHDR set ends up with
its data stuck in the send queue.

On the RX side, psp_dev_rcv() already says that fragment and routing
headers are not supported in transport mode. Could the TX helper's
comment state the same restriction, so driver authors know that some
extension header layouts make encapsulation fail?

> +	} else if (skb->protocol != htons(ETH_P_IP)) {
>  		return false;
> +	}

[ ... ]

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