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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf_tcp_ops allows sleepable BPF programs which will trigger kernel panics when executed within the RCU read-side critical sections of the dispatcher macros.
- [Low] Multi-line comments in BPF subsystem files must have the opening `/*` on its own line.
--

commit c794084a252357df7f0bbf934f1e1707d2dd52fd
Author: Martin KaFai Lau <martin.lau@kernel.org>
Subject: bpf: tcp: Add bpf_tcp_ops and support attaching to cgroups

This commit adds a TCP-specific struct_ops type, bpf_tcp_ops, and supports
attaching it to cgroups. It wires up several existing sock_ops callbacks to
allow each callback to have its own function signature.

Link: https://drive.google.com/file/d/1wjKZth6T0llLJ_ONPAL_6Q_jbxbAjByp/view?usp=sharing

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 6d376ea4d1c0..2102f9f2afd6 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
[ ... ]
> @@ -2953,12 +2953,120 @@ static inline void tcp_clear_sock_ops_cb_flags(struct sock *sk)
>  
>  #endif
>  
> +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_CGROUP_BPF)
> +
> +struct bpf_tcp_ops {
> +	/* Should return the initial SYN (active open) or SYN-ACK (passive open)
> +	 * retransmission timeout. Return the timeout in jiffies, or <= 0 for
> +	 * the kernel default.
> +	 *
> +	 * @req: request_sock on the passive (synack) path; NULL otherwise.
> +	 */

[Severity: Low]
This isn't a bug, but could these multi-line comments be updated to place the
opening /* on its own line?

[ ... ]
> diff --git a/net/ipv4/bpf_tcp_ops.c b/net/ipv4/bpf_tcp_ops.c
> new file mode 100644
> index 000000000000..cf53c95a0dbc
> --- /dev/null
> +++ b/net/ipv4/bpf_tcp_ops.c
[ ... ]
> +BPF_CALL_0(bpf_tcp_ops_get_retval)
> +{
> +	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.
> +	 */

[Severity: Low]
This isn't a bug, but would it be better to adjust this multi-line comment so
that the opening /* rests on its own line?

[ ... ]
> +static struct bpf_struct_ops bpf_tcp_ops = {
> +	.verifier_ops = &bpf_tcp_ops_verifier,
> +	.init_member = bpf_tcp_ops_init_member,
> +	.init = bpf_tcp_ops_init,
> +	.validate = bpf_tcp_ops_validate,
> +	.name = "bpf_tcp_ops",
> +	.cgroup_atype = CGROUP_TCP_SOCK_OPS,
> +	.cfi_stubs = &__bpf_tcp_ops,
> +	.owner = THIS_MODULE,
> +};

[Severity: High]
Is it possible for sleepable BPF programs to be loaded for this struct_ops
type?

Without a .check_member callback to explicitly reject them, the verifier
might allow sleepable programs to attach.

If a sleepable program is attached, could it trigger a scheduling while
atomic kernel panic?

Looking at include/net/tcp.h, the callbacks are invoked inside an RCU
read-side critical section:

bpf_tcp_ops_call()
    rcu_read_lock_dont_migrate();
    bpf_cgroup_struct_ops_foreach(...) {
        if (tcp_ops->op)
            tcp_ops->op(...);
    }
    rcu_read_unlock_migrate();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623175006.3136053-1-ameryhung@gmail.com?part=11