Re: [PATCH bpf-next 5/7] bpf: mptcp: Don't support BPF_SOCK_OPS_RCVQ_CB_FLAG.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: tcp_set_sock_ops_cb_flags breaks ALL_CB_FLAGS for MPTCP
--

commit 578335638acc4a540df534d791edf9c63d563e32
Author: Kuniyuki Iwashima <kuniyu@google.com>
Subject: bpf: mptcp: Don't support BPF_SOCK_OPS_RCVQ_CB_FLAG.

This patch prepares for a new kfunc by disabling BPF_SOCK_OPS_RCVQ_CB_FLAG
for MPTCP subflows. It returns -EOPNOTSUPP if this flag is used on an MPTCP
socket.

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 07426e8641b7e..d3cf655da9ecd 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2932,6 +2932,16 @@ static inline int tcp_call_bpf_3arg(struct sock *sk, int op, u32 arg1, u32 arg2,
>  	return tcp_call_bpf(sk, op, 3, args);
>  }
>  
> +static inline int tcp_set_sock_ops_cb_flags(struct sock *sk, int val)
> +{
> +	if (sk_is_mptcp(sk) &&
> +	    (val & BPF_SOCK_OPS_RCVQ_CB_FLAG))
> +		return -EOPNOTSUPP;
> +

[Severity: Medium]
Will this check unintentionally break existing BPF programs that use
BPF_SOCK_OPS_ALL_CB_FLAGS to enable all callbacks?

If a program passes BPF_SOCK_OPS_ALL_CB_FLAGS (which is expanded in this
patch series to include the new BPF_SOCK_OPS_RCVQ_CB_FLAG bit), this rigid
check evaluates to true for MPTCP subflows and rejects the entire multi-flag
request instead of ignoring the unsupported bit.

> +	tcp_sk(sk)->bpf_sock_ops_cb_flags = val;
> +	return 0;
> +}
> +

[ ... ]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 5feb998846825..f29c061bb0664 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -6178,8 +6177,9 @@ static const struct bpf_func_proto bpf_sock_ops_getsockopt_proto = {
>  BPF_CALL_2(bpf_sock_ops_cb_flags_set, struct bpf_sock_ops_kern *, bpf_sock,
>  	   int, argval)
>  {
> -	struct sock *sk = bpf_sock->sk;
>  	int val = argval & BPF_SOCK_OPS_ALL_CB_FLAGS;
> +	struct sock *sk = bpf_sock->sk;
> +	int err;
>  
>  	if (!is_locked_tcp_sock_ops(bpf_sock))
>  		return -EOPNOTSUPP;
> @@ -6187,7 +6187,9 @@ BPF_CALL_2(bpf_sock_ops_cb_flags_set, struct bpf_sock_ops_kern *, bpf_sock,
>  	if (!IS_ENABLED(CONFIG_INET) || !sk_fullsock(sk))
>  		return -EINVAL;
>  
> -	tcp_sk(sk)->bpf_sock_ops_cb_flags = val;
> +	err = tcp_set_sock_ops_cb_flags(sk, val);
> +	if (err)
> +		return err;

[Severity: Medium]
Does returning early here prevent any of the other valid callbacks requested
in val from being enabled?

Because tcp_set_sock_ops_cb_flags() returns -EOPNOTSUPP when the new flag is
present, the BPF helper propagates the error without applying any of the
previously supported flags. This effectively turns a wildcard request for
"all supported flags" into a total failure on MPTCP subflows.

>  
>  	return argval & (~BPF_SOCK_OPS_ALL_CB_FLAGS);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920195633.3033620-1-kuniyu@google.com?part=5