Re: [PATCH bpf-next v3 12/15] bpf: tcp: Support parse/len/write header option hooks in bpf_tcp_ops

On Fri, Jul 31, 2026 at 3:19 PM Emil Tsalapatis <[email protected]> wrote:
>
> On Mon Jul 6, 2026 at 1:19 PM EDT, Amery Hung wrote:
> > Add the TCP header option callbacks to the bpf_tcp_ops struct_ops type:
> >
> >   parse_hdr     - parse the options of an incoming skb on an established
> >                   connection
> >   hdr_opt_len   - reserve space in the TCP header for bpf options
> >   write_hdr_opt - write the reserved bpf options
> >
> > These mirror the BPF_SOCK_OPS_PARSE_HDR_OPT_CB, _HDR_OPT_LEN_CB and
> > _WRITE_HDR_OPT_CB legacy sockops callbacks, but are exposed as struct_ops
> > members so a program can implement them with normal function signatures
> > and per-member helper sets.
> >
> > The reserved header window is shared between the legacy sockops and
> > bpf_tcp_ops paths. tcp_{syn,synack,established}_options() first run the
> > legacy BPF_SOCK_OPS_HDR_OPT_LEN_CB and then call hdr_opt_len, so both
> > sources accumulate into opts->bpf_opt_len; at write time the legacy
> > options are emitted first and bpf_tcp_ops writes after them.
> >
> > API design
> >
> > bpf_tcp_ops overloads the sock_ops header-option helpers rather than
> > introducing a new API: bpf_reserve_hdr_opt(), bpf_store_hdr_opt() and
> > bpf_load_hdr_opt() are exposed per-member (reserve for hdr_opt_len,
> > store/load for write_hdr_opt, load for parse_hdr) and share the existing
> > kernel option-walking core via _bpf_sock_ops{store,load}hdr_opt(), with
> > the bpf_tcp_ops wrappers synthesizing a temporary bpf_sock_ops_kern from
> > the program ctx. This keeps a port from the legacy
> > BPF_SOCK_OPS*_HDR_OPT_CB callbacks mechanical (same helper calls) and
> > adds no new UAPI helper/kfunc surface.
> >
> > An alternative considered was to drop the option helpers entirely: have
> > hdr_opt_len reserve space purely through its return value, and introduce
> > a dedicated TCP-header-option dynptr used for both reading and writing.
> > That is a cleaner, more self-contained interface, but it is a larger
> > change and does not reuse the legacy helpers, making a port from sockops
> > less mechanical. It can be pursued as a follow-up; the helper-based
> > interface here keeps this series focused on moving the hooks to
> > struct_ops.
> >
> > The hdr_opt_len fast path in tcp_established_options() is gated by
> > cgroup_bpf_enabled(CGROUP_TCP_SOCK_OPS). Note this is a global,
> > per-attach-type static branch: it is enabled whenever any bpf_tcp_ops is
> > attached, even one that does not implement hdr_opt_len or that is attached
> > to a different cgroup. In those cases the block still runs but
> > bpf_tcp_ops_hdr_opt_len() no-ops via the per-member check in the dispatch
> > macro. A per-member/per-cgroup gate could be added later if the extra
> > fast-path work proves measurable.
> >
> > Signed-off-by: Amery Hung <[email protected]>
>
> Reviewed-by: Emil Tsalapatis <[email protected]>
>
> One question I've had both for this and the previous patch: Why are we
> defining new functionality as helpers and not kfuncs? Is it that we can
> allowlist with helpers and not with kfuncs?

These are not new functionality. bpf_reserve_hdr_opt(),
bpf_store_hdr_opt() and bpf_load_hdr_opt() already exist for sock_ops;
the patch only exposes them to bpf_tcp_ops per member and shares the
same option-walking core. I think we have been overloading helpers in
a few places. Adding kfunc equivalents would create a second API for
the same thing and make porting from the legacy
BPF_SOCK_OPS*_HDR_OPT_CB callbacks less mechanical.

Note that allowlisting is not the reason. kfuncs can be filtered per
program too, via btf_kfunc_id_set.filter.