Re: [PATCH v11 1/2] tcp: rehash onto different local ECMP path on retransmit timeout

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing mp_hash initialization in tcp_v6_send_response() causes TCP control packets (RST, TIME_WAIT ACKs) to use a different ECMP path than data packets.
- [High] Unconditional txhash randomization in BPF syncookies breaks symmetric ECMP routing for IPv6.
- [Medium] IPv4-mapped IPv6 connections incorrectly apply IPv6-specific cookie txhash logic, biasing queue distribution.

Pre-existing issues:
- [Critical] bpf_sk_assign_tcp_reqsk() partially initializes tcp_request_sock, leading to uninitialized slab memory usage and wild pointer dereferences.
--

commit e48c642c65c933dd8659dee905bbd3b1d86dd5a3
Author: Neil Spring <ntspring@meta.com>

tcp: rehash onto different local ECMP path on retransmit timeout

This patch updates TCP to select a different local ECMP path upon
retransmit timeout, path MTU discovery, or spurious retransmissions by
propagating the socket's txhash into the IPv6 multipath hash logic. It
ensures the cached destination is invalidated so the next transmit triggers
a fresh route lookup using the new hash. Syncookies are also updated to
derive txhash from the cookie for IPv6 to maintain ECMP path consistency.

> diff --git a/net/core/filter.c b/net/core/filter.c
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12301,6 +12301,7 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
>  
>  	treq->req_usec_ts = !!attrs->usec_ts_ok;
>  	treq->ts_off = tsoff;
> +	treq->txhash = net_tx_rndhash();
>  

[Severity: Critical]
This is a pre-existing issue, but since inet_reqsk_alloc() allocates the
request socket via a slab cache without zeroing memory, do the other fields
of tcp_request_sock (such as used_tcp_ao and af_specific) remain
uninitialized garbage?

If so, when tcp_create_openreq_child() is called later and dereferences
treq->af_specific->md5_lookup, could this result in a kernel panic from
evaluating the uninitialized pointers?

[Severity: High]
Does initializing txhash to a random value here break symmetric ECMP
routing for IPv6 BPF syncookies?

Since standard IPv6 syncookies set txhash to the cookie to maintain path
symmetry with the SYN-ACK, would this random initialization cause data
packets to route over a completely different ECMP path than the SYN/SYN-ACK
exchange?

>  	skb_orphan(skb);

[ ... ]

> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -280,9 +280,18 @@ static int cookie_tcp_reqsk_init(struct sock *sk, struct sk_buff *skb,
>  	treq->snt_synack = 0;
>  	treq->snt_tsval_first = 0;
>  	treq->tfo_listener = false;
> -	treq->txhash = net_tx_rndhash();
>  	treq->rcv_isn = ntohl(th->seq) - 1;
>  	treq->snt_isn = ntohl(th->ack_seq) - 1;
> +	if (sk->sk_family == AF_INET6) {
> +		/* Use the cookie as txhash so the ECMP path matches
> +		 * the SYN-ACK, where txhash was also set to the
> +		 * cookie.  The original request socket (and its
> +		 * txhash) was freed after sending the SYN-ACK.
> +		 */
> +		treq->txhash = treq->snt_isn;
> +	} else {
> +		treq->txhash = net_tx_rndhash();
> +	}

[Severity: Medium]
Can dual-stack listeners (IPv4-mapped IPv6) be negatively affected by this
check?

Since sk->sk_family is AF_INET6 for these listeners even when accepting IPv4
connections, would IPv4 connections incorrectly execute
treq->txhash = treq->snt_isn?

As noted in the commit message, the IPv4 cookie contains mssind bits that
could bias queue distribution if used as a hash.

>  	treq->syn_tos = TCP_SKB_CB(skb)->ip_dsfield;

[ ... ]

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -275,6 +277,15 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
>  
>  	security_sk_classify_flow(sk, flowi6_to_flowi_common(fl6));
>  
> +	/* Non-zero mp_hash bypasses rt6_multipath_hash() in
> +	 * fib6_select_path(), letting txhash control ECMP path
> +	 * selection so that sk_rethink_txhash() rehashes onto a
> +	 * different path.  Policies 1-3 derive a deterministic
> +	 * hash from the flow keys and must not be overridden.
> +	 */
> +	if (ip6_multipath_hash_policy(net) == 0 && sk->sk_txhash)
> +		fl6->mp_hash = (sk->sk_txhash >> 1) ?: 1;
> +

[Severity: High]
Does the missing mp_hash initialization in tcp_v6_send_response() cause TCP
control packets (like RST and TIME_WAIT ACKs) to use a different ECMP path
than data packets?

If tcp_v6_send_response() doesn't set fl6.mp_hash, it seems
fib6_select_path() will fall back to rt6_multipath_hash(). If the data path
was rehashed to recover from a failure, wouldn't the RSTs stay on the
broken default path and be dropped?

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