Re: [PATCH mptcp-next v4 1/5] mptcp: update bpf_mptcp_sock_from_subflow

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 09/12/2024 09:47, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The input parameter of bpf_mptcp_sock_from_subflow() is an msk subsocket
> of type IPPROTO_TCP. This patch extends it to accept an IPPROTO_MPTCP
> socket. With this change, the function name bpf_mptcp_sock_from_subflow
> is no longer appropriate, and it is renamed to bpf_mptcp_sock_from_sock.

It took me a bit of time to find the reason: why do you need this, why
not adding a new helper, especially because you don't directly use it in
this series.

Please always clearly mention the reason, probably the most important
bit in a commit message, even if it is obvious to you. I then suggest
having a (hopefully) clearer commit message:

================================== 8< ==================================
bpf: extend bpf_skc_to_mptcp_sock to MPTCP sock

Currently, bpf_skc_to_mptcp_sock() can only be used with sockets that
are MPTCP subflows: TCP sockets with tp->is_mptcp, created by the kernel
from an MPTCP socket (IPPROTO_MPTCP). Typically used with BPF sock_ops
operators.

Here, this helper is extended to support MPTCP sockets, the ones created
by the userspace (IPPROTO_MPTCP). This is useful for BPF hooks involving
these sockets, e.g. [gs]etsocktopt.

bpf_skc_to_mptcp_sock() uses bpf_mptcp_sock_from_subflow(). The former
suggests any MPTCP type/subtype can be used, but the latter only accepts
subflow ones. So bpf_mptcp_sock_from_subflow is modified here to support
MPTCP socket, and renamed to avoid confusions.
================================== 8< ==================================

WDYT? Here the reason (bpf_skc_to_mptcp_sock supporting MPTCP sockets)
is explained, and the title is clearer ("update X" is too vague).

(...)

> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index e9db856972cb..a0f49af85d57 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -188,8 +188,10 @@ static struct bpf_struct_ops bpf_mptcp_sched_ops = {
>  };
>  #endif /* CONFIG_BPF_JIT */
>  
> -struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
> +struct mptcp_sock *bpf_mptcp_sock_from_sock(struct sock *sk)
>  {
> +	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_MPTCP)
> +		return mptcp_sk(sk);
>  	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
>  		return mptcp_sk(mptcp_subflow_ctx(sk)->conn);

Maybe we should avoid repeating some checks?

  if (unlikely(!sk || !sk_fullsock(sk)))
      return NULL;

  if (sk->sk_protocol == IPPROTO_MPTCP)
      return mptcp_sk(sk);

  if (sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
      return mptcp_sk(mptcp_subflow_ctx(sk)->conn);

  return NULL;



Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.