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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch makes psp_dev_encapsulate() accept protocols other
  than TCP: psp->nexthdr and inner_ipproto now come from the packet, and…

--

psp: take the psp nexthdr from the packet on encap

This patch takes the PSP next header from the IP header of the original
packet instead of hardcoding IPPROTO_TCP, so that socket-less associations
can carry UDP. psp_dev_encapsulate() now refuses packets with IPv6
extension headers, IPv4 fragments and skbs that are already encapsulated.
mlx5e_features_check() also turns off GSO and checksum offload for PSP
skbs the device can't handle.

> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index e09f6fec948d7..1eb1617aaf2b5 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c

[ ... ]

> @@ -236,7 +237,7 @@ static void psp_write_headers(struct net *net, struct sk_buff *skb, __be32 spi,
>  	uh->check = 0;
>  	udp_set_len(uh, udp_len);
>  
> -	psph->nexthdr = IPPROTO_TCP;
> +	psph->nexthdr = nexthdr;

[Severity: Medium]

psp_dev_encapsulate() now accepts non-TCP packets. Is the outer UDP
source port selection in psp_write_headers() still correct when skb->sk
is a UDP socket?

The code just above this hunk still does:

	if (likely(sk)) {
		u32 hash = sk->sk_hash;
		...
		hash ^= hash << 16;
		uh->source = htons(reciprocal_scale(hash, max - min + 1) + min);
	}

The comment there assumes sk_hash is the TCP ehash, which stays constant
for the life of the flow. UDP sockets use the same storage for other
values:

include/linux/udp.h:
#define udp_port_hash		inet.sk.__sk_common.skc_u16hashes[0]
#define udp_portaddr_hash	inet.sk.__sk_common.skc_u16hashes[1]

udp_lib_get_port() then sets:

	udp_sk(sk)->udp_port_hash = snum;
	udp_sk(sk)->udp_portaddr_hash ^= snum;

At that point udp_portaddr_hash already holds
ipv4_portaddr_hash(net, rcv_saddr, 0).

After hash ^= hash << 16, the upper 16 bits are
udp_portaddr_hash ^ udp_port_hash. That cancels snum and leaves only the
hash of the netns and local address. reciprocal_scale() uses essentially
only those upper bits.

Wouldn't every UDP socket bound to the same local address then get the
same outer PSP source port? That includes all wildcard-bound sockets in a
netns, and it would undo the RSS spreading the comment describes.

This path becomes reachable later in the series. tcf_psp_act() in
net/sched/act_psp.c attaches the METADATA_PSP dst to egress skbs that
still carry their UDP skb->sk. As a result, mlx5 and netdevsim call
psp_dev_encapsulate()->psp_write_headers() with a UDP socket. The later
patches in the series leave psp_write_headers() unchanged. The sk == NULL
branch, which uses udp_flow_src_port(), is not affected.

Should the sk->sk_hash path be limited to TCP sockets, with other sockets
falling back to udp_flow_src_port()?

>  	psph->hdrlen = PSP_HDRLEN_NOOPT;
>  	psph->crypt_offset = 0;
>  	psph->verfl = FIELD_PREP(PSPHDR_VERFL_VERSION, ver) |

[ ... ]

> @@ -279,11 +313,11 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  		be16_add_cpu(&ipv6_hdr(skb)->payload_len, PSP_ENCAP_HLEN);
>  	}
>  
> -	skb_set_inner_ipproto(skb, IPPROTO_TCP);
> +	skb_set_inner_ipproto(skb, nexthdr);
>  	skb_set_inner_transport_header(skb, skb_transport_offset(skb) +
>  						    PSP_ENCAP_HLEN);
>  	skb->encapsulation = 1;
> -	psp_write_headers(net, skb, spi, ver,
> +	psp_write_headers(net, skb, spi, ver, nexthdr,
>  			  skb->len - skb_transport_offset(skb), sport);
>  
>  	return true;

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