From: Geliang Tang <tanggeliang@kylinos.cn>

Drop bpf_mptcp_sk() and bpf_mptcp_subflow_tcp_sock() definitions. Use
bpf_skc_to_mptcp_sock() and mptcp_subflow_tcp_sock() in mptcp_subflow
bpf_iter selftests instead.

Address Martin's comments in v1:

- add null-check for bpf_mptcp_subflow_ctx.
- add KF_RET_NULL flags for bpf_mptcp_subflow_ctx.
- register this kfunc set to BPF_PROG_TYPE_CGROUP_SOCKOPT only,
  not BPF_PROG_TYPE_UNSPEC.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/bpf.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index a0f49af85d57..f9ba0a46a9f1 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -218,21 +218,13 @@ struct bpf_iter_mptcp_subflow_kern {
 
 __bpf_kfunc_start_defs();
 
-__bpf_kfunc static struct mptcp_sock *bpf_mptcp_sk(struct sock *sk)
-{
-	return mptcp_sk(sk);
-}
-
 __bpf_kfunc static struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx(const struct sock *sk)
 {
-	return mptcp_subflow_ctx(sk);
-}
+	if (!sk)
+		return NULL;
 
-__bpf_kfunc static struct sock *
-bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
-{
-	return mptcp_subflow_tcp_sock(subflow);
+	return mptcp_subflow_ctx(sk);
 }
 
 __bpf_kfunc static int
@@ -301,9 +293,7 @@ __bpf_kfunc static bool bpf_mptcp_subflow_queues_empty(struct sock *sk)
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(bpf_mptcp_common_kfunc_ids)
-BTF_ID_FLAGS(func, bpf_mptcp_sk)
-BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx)
-BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock)
+BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx, KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY)
@@ -337,7 +327,7 @@ static int __init bpf_mptcp_kfunc_init(void)
 	int ret;
 
 	ret = register_btf_fmodret_id_set(&bpf_mptcp_fmodret_set);
-	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC,
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCKOPT,
 					       &bpf_mptcp_common_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS,
 					       &bpf_mptcp_sched_kfunc_set);
-- 
2.45.2

On 09/12/2024 09:47, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Drop bpf_mptcp_sk() and bpf_mptcp_subflow_tcp_sock() definitions. Use
> bpf_skc_to_mptcp_sock() and mptcp_subflow_tcp_sock() in mptcp_subflow
> bpf_iter selftests instead.
> 
> Address Martin's comments in v1:
> 
> - add null-check for bpf_mptcp_subflow_ctx.
> - add KF_RET_NULL flags for bpf_mptcp_subflow_ctx.
> - register this kfunc set to BPF_PROG_TYPE_CGROUP_SOCKOPT only,
>   not BPF_PROG_TYPE_UNSPEC.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/bpf.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index a0f49af85d57..f9ba0a46a9f1 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -218,21 +218,13 @@ struct bpf_iter_mptcp_subflow_kern {
>  
>  __bpf_kfunc_start_defs();
>  
> -__bpf_kfunc static struct mptcp_sock *bpf_mptcp_sk(struct sock *sk)
> -{
> -	return mptcp_sk(sk);
> -}
> -
>  __bpf_kfunc static struct mptcp_subflow_context *
>  bpf_mptcp_subflow_ctx(const struct sock *sk)
>  {
> -	return mptcp_subflow_ctx(sk);
> -}
> +	if (!sk)
> +		return NULL;

Is it enough for the restrictions?

I mean, from what I understood, this helper can be used from any CGROUP
SOCKOPT hooks, with any type of 'struct sock *': then it could be called
from a non MPTCP subflow socket, e.g. a socket with ULP data, no?

Then should we not check this?

  sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk)

(and even sk_fullsock(sk)?)

(...)

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