Re: [net-next,v9,1/2] tcp: Replace min_tso_segs() with tso_segs() CC callback

[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

- [Medium] The patch silently removes the `bbr_min_tso_segs` BPF kfunc
  without a deprecation window and without mentioning it in the commit…

--

tcp: Replace min_tso_segs() with tso_segs() CC callback

The min_tso_segs() congestion control callback is replaced by a new
tso_segs() callback that takes mss_now and returns the final TSO segment
count, bypassing tcp_tso_autosize().  tcp_tso_autosize() is exported so
modules can still do the autosizing themselves, and BBR is converted to
the new callback.

> diff --git a/include/net/tcp.h b/include/net/tcp.h

[ ... ]

> diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
> index 82378a2bfd1e1..ecf11be46f382 100644
> --- a/net/ipv4/tcp_bbr.c
> +++ b/net/ipv4/tcp_bbr.c
> @@ -297,11 +297,18 @@ static void bbr_set_pacing_rate(struct sock *sk, u32 bw, int gain)
>  }
>  
>  /* override sysctl_tcp_min_tso_segs */
> -__bpf_kfunc static u32 bbr_min_tso_segs(struct sock *sk)
> +static u32 bbr_min_tso_segs(struct sock *sk)
>  {
>  	return READ_ONCE(sk->sk_pacing_rate) < (bbr_min_tso_rate >> 3) ? 1 : 2;
>  }
>  
> +__bpf_kfunc static u32 bbr_tso_segs(struct sock *sk, u32 mss_now)
> +{
> +	if (unlikely(!mss_now))
> +		return bbr_min_tso_segs(sk);
> +	return tcp_tso_autosize(sk, mss_now, bbr_min_tso_segs(sk));
> +}
> +

[ ... ]

> @@ -1163,7 +1170,7 @@ BTF_ID_FLAGS(func, bbr_sndbuf_expand)
>  BTF_ID_FLAGS(func, bbr_undo_cwnd)
>  BTF_ID_FLAGS(func, bbr_cwnd_event_tx_start)
>  BTF_ID_FLAGS(func, bbr_ssthresh)
> -BTF_ID_FLAGS(func, bbr_min_tso_segs)
> +BTF_ID_FLAGS(func, bbr_tso_segs)
>  BTF_ID_FLAGS(func, bbr_set_state)
>  BTF_KFUNCS_END(tcp_bbr_check_kfunc_ids)

[Severity: Medium]
Dropping the __bpf_kfunc annotation from bbr_min_tso_segs() and replacing its
BTF_ID_FLAGS() entry with bbr_tso_segs() takes the bbr_min_tso_segs kfunc out
of the kernel BTF kfunc set in one step.  Should this go through a
deprecation release instead?

Any out-of-tree BPF congestion control containing:

    extern u32 bbr_min_tso_segs(struct sock *sk) __ksym;

now fails to load with "kernel function bbr_min_tso_segs not found in kernel
BTF", and that applies to programs calling it from any callback, not only
from the renamed one.

The helper is still present and still called internally, both from
bbr_tso_segs() and from bbr_tso_segs_goal():

net/ipv4/tcp_bbr.c:bbr_tso_segs_goal() {
    ...
	segs = max_t(u32, bytes / tp->mss_cache, bbr_min_tso_segs(sk));
    ...
}

so would keeping the BTF_ID_FLAGS(func, bbr_min_tso_segs, KF_DEPRECATED)
entry for a release work here?  Documentation/bpf/kfuncs.rst says "It's
expected that the common case will be that kfuncs will go through a
deprecation period rather than being changed or removed without warning".

The v8 changelog of this submission said "Deprecate bbr_min_tso_segs() kfunc
instead of removing it", and v9 said "Remove deprecated flag on
bbr_min_tso_segs()", so the deprecation flag appears to have been dropped
again between revisions.  Was that intentional?

The commit message lists the BPF struct_ops impacts as:

    - The callback is renamed from min_tso_segs() to tso_segs()
    - The signature gains an extra u32 mss_now argument
    - The return value semantics is changed from "floor value passed into
      tcp_tso_autosize()" to "final tso_segs value", bypassing autosizing

Could the disappearance of the bbr_min_tso_segs kfunc be added to that list?

On the replacement side, the follow-up commit in this series, "bpf: make
tcp_tso_autosize() available to BPF congestion controls", marks
tcp_tso_autosize() as __bpf_kfunc and adds it to bpf_tcp_ca_check_kfunc_ids,
so a BPF congestion control can reproduce the old "raise the floor, keep
autosizing" behaviour by calling tcp_tso_autosize(sk, mss_now, my_floor)
from there on.  At this commit alone that kfunc is not yet registered, so
bbr_tso_segs() with BBR's hardcoded 1-or-2 floor is the only substitute.

In-tree users were updated in
tools/testing/selftests/bpf/progs/tcp_ca_kfunc.c and no other in-tree
reference to the old kfunc remains.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831165230.2696893-1-chia-yu.chang%40nokia-bell-labs.com