Re: [PATCH v4 net-next 1/1] tcp: allow congestion controls to override TSO autosizing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Moving `min_tso_segs` into an anonymous union breaks the registration and map updates of BPF TCP congestion controls, rendering the BPF CA subsystem unusable.
- [High] Aliasing function pointers with different signatures inside a union creates severe Control Flow Integrity (CFI) hazards and breaks BPF stub generation.
- [High] Returning 0 from `ca_ops->tso_segs()` causes a permanent stall in the TCP write queue.
--

--- Patch [1]: [PATCH v4 net-next 1/1] tcp: allow congestion controls to override TSO autosizing ---
commit 986d64ed9a398f1ef7247d7b653292cb0b6d25f3
Author: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>

tcp: allow congestion controls to override TSO autosizing

This patch introduces the TCP_CONG_EXACT_TSO_SEGS flag and adds an alternative
tso_segs() callback to tcp_congestion_ops. It allows congestion control
algorithms to provide an exact TSO segment count instead of just a minimum.
This is implemented by placing the new callback in a union with the existing
min_tso_segs() callback.

Link: None

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 2c5b889530b55..82cd82a19ff91 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
[ ... ]
> @@ -1361,8 +1363,16 @@ struct tcp_congestion_ops {
>  	/* hook for packet ack accounting (optional) */
>  	void (*pkts_acked)(struct sock *sk, const struct ack_sample *sample);
>  
> -	/* override sysctl_tcp_min_tso_segs (optional) */
> -	u32 (*min_tso_segs)(struct sock *sk);
> +	union {
> +		/* Used when TCP_CONG_EXACT_TSO_SEGS is not set,
> +		 * override sysctl_tcp_min_tso_segs (optional)
> +		 */
> +		u32 (*min_tso_segs)(struct sock *sk);
> +		/* Used when TCP_CONG_EXACT_TSO_SEGS is set,
> +		 * override tcp_tso_autosize (optional)
> +		 */
> +		u32 (*tso_segs)(struct sock *sk, u32 mss_now);
> +	};

[Severity: High]
Does this anonymous union prevent the BPF TCP congestion control subsystem from
initializing?

Looking at bpf_struct_ops_desc_init() in kernel/bpf/bpf_struct_ops.c, the BPF
framework iterates over the members of the struct. If it encounters an
anonymous member, it explicitly rejects it with an "anon member in struct is
not supported" warning and aborts the registration:

kernel/bpf/bpf_struct_ops.c:bpf_struct_ops_desc_init() {
    ...
    if (!*mname) {
        pr_warn("anon member in struct %s is not supported\n",
            st_ops->name);
        err = -EOPNOTSUPP;
        goto errout;
    }
    ...
}

[Severity: High]
Can aliasing function pointers with different signatures inside a union create
Control Flow Integrity (CFI) hazards?

The union aliases min_tso_segs (1 argument) and tso_segs (2 arguments). If a
congestion control module misconfigures the TCP_CONG_EXACT_TSO_SEGS flag, the
kernel might execute an indirect call using a signature that does not match the
target function's compiled kCFI hash. Would this trigger a kernel panic?

Additionally, does this break BPF stub generation since the static stub
structure __bpf_ops_tcp_congestion_ops can only populate a single pointer for
this memory offset?

> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index d7c1444b5e304..0de27a87c39e7 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
[ ... ]
> @@ -2278,11 +2278,20 @@ static u32 tcp_tso_segs(struct sock *sk, unsigned int mss_now)
>  	const struct tcp_congestion_ops *ca_ops = inet_csk(sk)->icsk_ca_ops;
>  	u32 min_tso, tso_segs;
>  
> -	min_tso = ca_ops->min_tso_segs ?
> -			ca_ops->min_tso_segs(sk) :
> -			READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_min_tso_segs);
> +	min_tso = READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_min_tso_segs);
> +
> +	if (ca_ops->flags & TCP_CONG_EXACT_TSO_SEGS) {
> +		if (WARN_ON_ONCE(!ca_ops->tso_segs))
> +			tso_segs = tcp_tso_autosize(sk, mss_now, min_tso);
> +		else
> +			tso_segs = ca_ops->tso_segs(sk, mss_now);
> +	} else {
> +		if (ca_ops->min_tso_segs)
> +			min_tso = ca_ops->min_tso_segs(sk);
> +
> +		tso_segs = tcp_tso_autosize(sk, mss_now, min_tso);
> +	}
>  
> -	tso_segs = tcp_tso_autosize(sk, mss_now, min_tso);
>  	return min_t(u32, tso_segs, sk->sk_gso_max_segs);
>  }

[Severity: High]
Does this code allow ca_ops->tso_segs() to return 0 and stall the socket?

While tcp_tso_autosize() is guaranteed to return at least 1 (because
sysctl_tcp_min_tso_segs is strictly clamped to >= 1), the new callback has no
minimum bounds check.

If ca_ops->tso_segs() returns 0, tcp_tso_segs() returns 0. When processed in
tcp_write_xmit(), this forces max_segs = 0, bounding cwnd_quota to 0:

net/ipv4/tcp_output.c:tcp_write_xmit() {
    ...
    cwnd_quota = min(cwnd_quota, max_segs);
    ...
}

This causes tcp_mss_split_point() to receive max_segs = 0 and return a limit of
0. Then tso_fragment() shrinks the original head SKB to 0 bytes:

net/ipv4/tcp_output.c:tcp_write_xmit() {
    ...
    if (skb->len > limit &&
        unlikely(tso_fragment(sk, skb, limit, mss_now, gfp)))
        break;
    ...
}

This 0-length SKB appears to permanently trigger the break condition, skipping
dequeue and stalling the write queue on all subsequent calls:

net/ipv4/tcp_output.c:tcp_write_xmit() {
    ...
    if (TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq)
        break;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726193802.397321-1-chia-yu.chang@nokia-bell-labs.com?part=1