Re: [PATCH bpf-next v3 11/15] bpf: tcp: Support selected sock_ops callbacks as struct_ops

["Emil Tsalapatis" <emil@etsalapatis.com>]

On Mon Jul 6, 2026 at 1:19 PM EDT, Amery Hung wrote:
> In LSFMMBPF 2025, I have talked about moving the BPF_PROG_TYPE_SOCK_OPS
> to a struct_ops interface [1].
>
> The BPF_SOCK_OPS_*_CB enum interface has grown over time as new TCP
> callback points were added. A BPF_PROG_TYPE_SOCK_OPS program now
> commonly needs a large switch on sock_ops->op, and the shared
> bpf_sock_ops_kern context has become harder to extend because different
> callbacks have different locking, argument, skb, and helper
> requirements. The existing 'union { u32 args[4]; u32 replylong[4]; }' is
> also not reliable in passing args to bpf prog when there are multiple
> progs attached to a cgroup.
>
> The above has already been solved in struct_ops. Add a TCP-specific
> struct_ops type, bpf_tcp_ops, and support attaching it to cgroups.
> This allows each callback have its own func signature and allows
> the verifier to select kfuncs/helpers based on the specific
> struct_ops member being implemented.
>
> This patch wires up the following existing sock_ops callbacks:
> - BPF_SOCK_OPS_TIMEOUT_INIT
> - BPF_SOCK_OPS_RWND_INIT
> - BPF_SOCK_OPS_RTT_CB
> - BPF_SOCK_OPS_STATE_CB
> - BPF_SOCK_OPS_RETRANS_CB
> - BPF_SOCK_OPS_TCP_CONNECT_CB
> - BPF_SOCK_OPS_TCP_LISTEN_CB
> - BPF_SOCK_OPS_RTO_CB
> - BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB
> - BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB
>
> BASE_RTT is ignored as it is not particularly useful. NEEDS_ECN should
> be done in bpf-tcp-cc instead. The tstamp ones should be a separate
> struct_ops (e.g. "bpf_sock_ops") that can work in both TCP and UDP.
>
> timeout_init and rwnd_init could have a request_sock pointer. This patch
> tries a different API and direclty passes the request_sock pointer as
> an arg.
>
> Two other approaches were considered before settling on having
> bpf_get_retval() read the dispatcher's run_ctx via saved_run_ctx. The
> first was to inherit the retval in the trampoline itself: add a helper
> in the four __bpf_prog_enter*() paths that, for struct_ops programs,
> copies the chained value from the caller's run_ctx (now saved_run_ctx)
> into the program's own run_ctx. It works but puts a per-enter
> program-type check on the generic trampoline fast path, taxing all
> fentry/fexit/lsm callers for a cgroup-struct_ops-only feature. The
> second was to do that same inherit only for the int-returning members
> via a gen_prologue that emits a hidden kfunc at the start of
> timeout_init/rwnd_init; this keeps the cost off the generic path and
> scoped to bpf_tcp_ops, but needs a kfunc + BTF_ID + prologue-emission
> machinery. The chosen approach avoids both: it touches neither the
> trampoline nor the program, since saved_run_ctx already points at the
> dispatcher's run_ctx that carries the value.
>
> [1], page 13: https://drive.google.com/file/d/1wjKZth6T0llLJ_ONPAL_6Q_jbxbAjByp/view?usp=sharing
>
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> Signed-off-by: Amery Hung <ameryhung@gmail.com>

Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>

Two nits below.

> ---
>  include/linux/bpf.h    |   1 +
>  include/net/tcp.h      | 113 ++++++++++++++++++++++++-
>  net/ipv4/Makefile      |   1 +
>  net/ipv4/af_inet.c     |   1 +
>  net/ipv4/bpf_tcp_ops.c | 188 +++++++++++++++++++++++++++++++++++++++++
>  net/ipv4/tcp.c         |   1 +
>  net/ipv4/tcp_input.c   |   4 +
>  net/ipv4/tcp_output.c  |   2 +
>  net/ipv4/tcp_timer.c   |   1 +
>  9 files changed, 310 insertions(+), 2 deletions(-)
>  create mode 100644 net/ipv4/bpf_tcp_ops.c
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index df95ae690da5..91024d2da4ea 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2597,6 +2597,7 @@ struct bpf_trace_run_ctx {
>  struct bpf_tramp_run_ctx {
>  	struct bpf_run_ctx run_ctx;
>  	u64 bpf_cookie;
> +	int retval;
>  	struct bpf_run_ctx *saved_run_ctx;
>  };
>  

<SNIP>

The retval is in bpf_tramp_run_ctx is generic, so maybe we can make the
helper for retrieving it generic, too. Should we at least rename it to
make this obvious?

> +{
> +	struct bpf_tramp_run_ctx *ctx =
> +		container_of(current->bpf_ctx, struct bpf_tramp_run_ctx, run_ctx);
> +
> +	/* bpf_get_retval() is only exposed to timeout_init/rwnd_init, which
> +	 * always run via bpf_tcp_ops_call_int(). Its run_ctx carries the int
> +	 * return value chained across the bpf_tcp_ops attached to the cgroup
> +	 * and is this program's saved_run_ctx.
> +	 */
> +	if (WARN_ON_ONCE(!ctx->saved_run_ctx))
> +		return 0;
> +
> +	return container_of(ctx->saved_run_ctx, struct bpf_tramp_run_ctx,
> +			    run_ctx)->retval;
> +}
> +

<SNIP>