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

On Fri, Jul 31, 2026 at 2:11 PM Emil Tsalapatis <[email protected]> wrote:
>
> 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 <[email protected]>
> > Signed-off-by: Amery Hung <[email protected]>
>
> Reviewed-by: Emil Tsalapatis <[email protected]>
>
> 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?

The helper cannot easily be made generic: the chaining is done in
bpf_tcp_ops_call_int(), which is bpf_tcp_ops specific, and the helper
reads the retval out of that dispatcher's run_ctx via saved_run_ctx. A
bpf_cgroup_struct_ops_ name would over-promise for now. I would rather
keep bpf_tcp_ops_get_retval() and rename it if and when a second user
shows up (it likely will follow the same convention).

The commit message describes two other approaches that would make it
generic: 1) inheriting the retval in __bpf_prog_enter*(), and 2) a
gen_prologue-emitted kfunc, and why neither was chosen.

>
> > +{
> > +     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>