Re: [PATCH net] psp: use sk->sk_hash in psp_write_headers()

[Daniel Zahka <daniel.zahka@gmail.com>]

On 2/18/26 9:13 AM, Eric Dumazet wrote:
> udp_flow_src_port() is indirectly using sk->sk_txhash as a base,
> because __tcp_transmit_skb() uses skb_set_hash_from_sk().
>
> This is problematic because this field can change over the
> lifetime of a TCP flow, thanks to calls to sk_rethink_txhash().
>
> Problem is that some NIC might (ab)use the PSP UDP source port in their
> RSS computation, and PSP packets for a given flow could jump
> from one queue to another.
>
> In order to avoid surprises, it is safer to let Protective Load
> Balancing (PLB) get its entropy from the IPv6 flowlabel,
> and change psp_write_headers() to use sk->sk_hash which
> does not change for the duration of the flow.
>
> We might add a sysctl to select the behavior, if there
> is a need for it.
>
> Fixes: fc724515741a ("psp: provide encapsulation helper for drivers")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> Cc: Raed Salem <raeds@nvidia.com>
> Cc: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> Cc: Cosmin Ratiu <cratiu@nvidia.com>
> Cc: Daniel Zahka <daniel.zahka@gmail.com>
> Cc: Willem de Bruijn <willemb@google.com>
> ---
>   net/psp/psp_main.c | 39 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index a8534124f62669cf8b3611d5352e3827982d871d..066222eb56c4af187791445f30a70443aef8a6d8 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c
> @@ -166,9 +166,46 @@ static void psp_write_headers(struct net *net, struct sk_buff *skb, __be32 spi,
>   {
>   	struct udphdr *uh = udp_hdr(skb);
>   	struct psphdr *psph = (struct psphdr *)(uh + 1);
> +	const struct sock *sk = skb->sk;
>   
>   	uh->dest = htons(PSP_DEFAULT_UDP_PORT);
> -	uh->source = udp_flow_src_port(net, skb, 0, 0, false);
> +
> +	/* A bit of theory: Selection of the source port.
> +	 *
> +	 * We need some entropy, so that multiple flows use different
> +	 * source ports for better RSS spreading at the receiver.
> +	 *
> +	 * We also need that all packets belonging to one TCP flow
> +	 * use the same source port through their duration,
> +	 * so that all these packets land in the same receive queue.
> +	 *
> +	 * udp_flow_src_port() is using sk_txhash, inherited from
> +	 * skb_set_hash_from_sk() call in __tcp_transmit_skb().
> +	 * This field is subject to reshuffling, thanks to
> +	 * sk_rethink_txhash() calls in various TCP functions.
> +	 *
> +	 * Instead, use sk->sk_hash which is constant through
> +	 * the whole flow duration.
> +	 */
> +	if (likely(sk)) {
> +		u32 hash = sk->sk_hash;
> +		int min, max;
> +
> +		/* These operations are cheap, no need to cache the result
> +		 * in another socket field.
> +		 */
> +		inet_get_local_port_range(net, &min, &max);
> +		/* Since this is being sent on the wire obfuscate hash a bit
> +		 * to minimize possibility that any useful information to an
> +		 * attacker is leaked. Only upper 16 bits are relevant in the
> +		 * computation for 16 bit port value because we use a
> +		 * reciprocal divide.
> +		 */
> +		hash ^= hash << 16;
> +		uh->source = htons((((u64)hash * (max - min)) >> 32) + min);
> +	} else {
> +		uh->source = udp_flow_src_port(net, skb, 0, 0, false);
> +	}
>   	uh->check = 0;
>   	uh->len = htons(udp_len);
>   

Thanks. The PSP spec suggests in multiple places the that udp sport be a 
hash derived from "inner header fields", so sk->sk_hash fits that 
description better than sk->sk_txhash regardless of RSS use/abuse 
categorization.

It occurs to me now that fc724515741a ("psp: provide encapsulation 
helper for drivers") also has a bug of unused udp sport parameter. I can 
send a fix for that later, unless you want to remove that as well.

Reviewed-By: Daniel Zahka <daniel.zahka@gmail.com>