[PATCH mptcp-next v4 0/5] Squash to "Add mptcp_subflow bpf_iter support"

[PATCH mptcp-next v4 0/5] Squash to "Add mptcp_subflow bpf_iter support"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v4:
 - CI reports the following BUILD_BUG_ON fails on i386:

 BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) !=
 	      sizeof(struct bpf_iter_mptcp_subflow))

 Just like in bpf_iter_task_new(), change this "!=" to ">".

v3:
 - check sock_owned_by_user_nocheck(sk)/spin_is_locked(&sk->sk_lock.slock),
   instead of lockdep_sock_is_held(sk).
 - add "sizeof" and "alignof" checks.
 - 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.

v2:
 - add CONFIG_LOCKDEP check in patch 2 to fix the build error reported
   by CI.

Address Martin's comments in v1.

Geliang Tang (5):
  mptcp: update bpf_mptcp_sock_from_subflow
  bpf: Allow use of skc_to_mptcp_sock in cg_sockopt
  Squash to "bpf: Register mptcp common kfunc set"
  Squash to "bpf: Add mptcp_subflow bpf_iter"
  Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest"

 include/net/mptcp.h                           |  4 +--
 kernel/bpf/cgroup.c                           |  2 ++
 net/core/filter.c                             |  2 +-
 net/mptcp/bpf.c                               | 34 +++++++++----------
 tools/testing/selftests/bpf/progs/mptcp_bpf.h |  1 -
 .../selftests/bpf/progs/mptcp_bpf_iters.c     | 11 +++---
 6 files changed, 27 insertions(+), 27 deletions(-)

-- 
2.45.2

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

[Geliang Tang]

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.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h | 4 ++--
 net/core/filter.c   | 2 +-
 net/mptcp/bpf.c     | 4 +++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 814b5f2e3ed5..94d5976f7b8d 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -322,9 +322,9 @@ static inline void mptcpv6_handle_mapped(struct sock *sk, bool mapped) { }
 #endif
 
 #if defined(CONFIG_MPTCP) && defined(CONFIG_BPF_SYSCALL)
-struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk);
+struct mptcp_sock *bpf_mptcp_sock_from_sock(struct sock *sk);
 #else
-static inline struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk) { return NULL; }
+static inline struct mptcp_sock *bpf_mptcp_sock_from_sock(struct sock *sk) { return NULL; }
 #endif
 
 #if !IS_ENABLED(CONFIG_MPTCP)
diff --git a/net/core/filter.c b/net/core/filter.c
index fac245065b0a..9ac048f0c5dd 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11836,7 +11836,7 @@ const struct bpf_func_proto bpf_skc_to_unix_sock_proto = {
 BPF_CALL_1(bpf_skc_to_mptcp_sock, struct sock *, sk)
 {
 	BTF_TYPE_EMIT(struct mptcp_sock);
-	return (unsigned long)bpf_mptcp_sock_from_subflow(sk);
+	return (unsigned long)bpf_mptcp_sock_from_sock(sk);
 }
 
 const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
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);
 
-- 
2.45.2

[PATCH mptcp-next v4 2/5] bpf: Allow use of skc_to_mptcp_sock in cg_sockopt

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Currently, bpf_skc_to_mptcp_sock() helper is not allowed to be used
in cg_sockopt. This patch adds this permission.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 kernel/bpf/cgroup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 46e5db65dbc8..1ca22e4842cf 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -2358,6 +2358,8 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 #ifdef CONFIG_INET
 	case BPF_FUNC_tcp_sock:
 		return &bpf_tcp_sock_proto;
+	case BPF_FUNC_skc_to_mptcp_sock:
+		return &bpf_skc_to_mptcp_sock_proto;
 #endif
 	case BPF_FUNC_perf_event_output:
 		return &bpf_event_output_data_proto;
-- 
2.45.2

[PATCH mptcp-next v4 3/5] Squash to "bpf: Register mptcp common kfunc set"

[Geliang Tang]

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

[PATCH mptcp-next v4 4/5] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add "sizeof" and "alignof" checks.

Address Martin's comments in v1:

- bpf_iter_mptcp_subflow_new returns -EINVAL when msk socket lock isn't
  held.

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

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index f9ba0a46a9f1..0563d3c6d9d3 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -232,12 +232,20 @@ bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
 			   struct mptcp_sock *msk)
 {
 	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
+	struct sock *sk = (struct sock *)msk;
+
+	BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) >
+		     sizeof(struct bpf_iter_mptcp_subflow));
+	BUILD_BUG_ON(__alignof__(struct bpf_iter_mptcp_subflow_kern) !=
+		     __alignof__(struct bpf_iter_mptcp_subflow));
 
 	kit->msk = msk;
 	if (!msk)
 		return -EINVAL;
 
-	msk_owned_by_me(msk);
+	if (!sock_owned_by_user_nocheck(sk) &&
+	    !spin_is_locked(&sk->sk_lock.slock))
+		return -EINVAL;
 
 	kit->pos = &msk->conn_list;
 	return 0;
-- 
2.45.2

[PATCH mptcp-next v4 5/5] Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Use bpf_skc_to_mptcp_sock() and mptcp_subflow_tcp_sock() instead of
bpf_mptcp_sk() and bpf_mptcp_subflow_tcp_sock().

IPPROTO_MPTCP is checked in bpf_skc_to_mptcp_sock(), no need to check
it in BPF program.

bpf_skc_to_mptcp_sock() and bpf_mptcp_subflow_ctx() may return NULL,
need to check the return values.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/progs/mptcp_bpf.h       |  1 -
 tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 11 +++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
index 3b20cfd44505..b1f6e1fb467e 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
@@ -46,7 +46,6 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
 extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk) __ksym;
 extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym;
 
-extern struct mptcp_sock *bpf_mptcp_sk(struct sock *sk) __ksym;
 extern struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
 extern struct sock *
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
index 1bede22a7e3d..fd5691a4073b 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
@@ -21,12 +21,11 @@ int iters_subflow(struct bpf_sockopt *ctx)
 	struct mptcp_sock *msk;
 	int local_ids = 0;
 
-	if (!sk || sk->protocol != IPPROTO_MPTCP ||
-	    ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
+	if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
 		return 1;
 
-	msk = bpf_mptcp_sk((struct sock *)sk);
-	if (msk->pm.server_side || !msk->pm.subflows)
+	msk = bpf_skc_to_mptcp_sock(sk);
+	if (!msk || msk->pm.server_side || !msk->pm.subflows)
 		return 1;
 
 	msk = bpf_mptcp_sock_acquire(msk);
@@ -41,7 +40,7 @@ int iters_subflow(struct bpf_sockopt *ctx)
 		local_ids += subflow->subflow_id;
 
 		/* only to check the following kfunc works */
-		ssk = bpf_mptcp_subflow_tcp_sock(subflow);
+		ssk = mptcp_subflow_tcp_sock(subflow);
 	}
 
 	if (!ssk)
@@ -53,7 +52,7 @@ int iters_subflow(struct bpf_sockopt *ctx)
 
 	/* only to check the following kfunc works */
 	subflow = bpf_mptcp_subflow_ctx(ssk);
-	if (subflow->token != msk->token)
+	if (!subflow || subflow->token != msk->token)
 		goto out;
 
 	ids = local_ids;
-- 
2.45.2

Re: [PATCH mptcp-next v4 0/5] Squash to "Add mptcp_subflow bpf_iter support"

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Unstable: 1 failed test(s): selftest_mptcp_connect 🔴
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/12232272946

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/d9e0118c5b4b
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=915828


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v4 0/5] Squash to "Add mptcp_subflow bpf_iter support"

[Matthieu Baerts]

Hi Geliang,

On 09/12/2024 09:47, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v4:
>  - CI reports the following BUILD_BUG_ON fails on i386:
> 
>  BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) !=
>  	      sizeof(struct bpf_iter_mptcp_subflow))
> 
>  Just like in bpf_iter_task_new(), change this "!=" to ">".
> 
> v3:
>  - check sock_owned_by_user_nocheck(sk)/spin_is_locked(&sk->sk_lock.slock),
>    instead of lockdep_sock_is_held(sk).
>  - add "sizeof" and "alignof" checks.
>  - 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.

Thank you for the new versions. I have some additional questions, see my
comment on the individual patches.

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

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

[Matthieu Baerts]

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.

Re: [PATCH mptcp-next v4 2/5] bpf: Allow use of skc_to_mptcp_sock in cg_sockopt

[Matthieu Baerts]

On 09/12/2024 09:47, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Currently, bpf_skc_to_mptcp_sock() helper is not allowed to be used
> in cg_sockopt. This patch adds this permission.

Clearer to add the reason: why is it useful?

Something like this?

Thanks to the previous patch allowing skc_to_mptcp_sock() to be used
with MPTCP sockets, this permission allows this helper to be use it in
CGroup BPF hooks, e.g. [gs]etsocktopt.

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

Re: [PATCH mptcp-next v4 3/5] Squash to "bpf: Register mptcp common kfunc set"

[Matthieu Baerts]

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.

Re: [PATCH mptcp-next v4 4/5] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Matthieu Baerts]

Hi Geliang,

On 09/12/2024 09:47, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Add "sizeof" and "alignof" checks.
> 
> Address Martin's comments in v1:
> 
> - bpf_iter_mptcp_subflow_new returns -EINVAL when msk socket lock isn't
>   held.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/bpf.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index f9ba0a46a9f1..0563d3c6d9d3 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -232,12 +232,20 @@ bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
>  			   struct mptcp_sock *msk)
>  {
>  	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> +	struct sock *sk = (struct sock *)msk;
> +
> +	BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) >
> +		     sizeof(struct bpf_iter_mptcp_subflow));
> +	BUILD_BUG_ON(__alignof__(struct bpf_iter_mptcp_subflow_kern) !=
> +		     __alignof__(struct bpf_iter_mptcp_subflow));
>  
>  	kit->msk = msk;
>  	if (!msk)
>  		return -EINVAL;
>  
> -	msk_owned_by_me(msk);
> +	if (!sock_owned_by_user_nocheck(sk) &&
> +	    !spin_is_locked(&sk->sk_lock.slock))
> +		return -EINVAL;

Just to be sure, did you check it was always the case with the WIP
scheduler and path-manager BPF programs you have?

We don't want to be too restrictive. (Or we need different helpers)

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

Re: [PATCH mptcp-next v4 4/5] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Geliang Tang]

Hi Matt,

On Tue, 2024-12-10 at 12:48 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 09/12/2024 09:47, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Add "sizeof" and "alignof" checks.
> > 
> > Address Martin's comments in v1:
> > 
> > - bpf_iter_mptcp_subflow_new returns -EINVAL when msk socket lock
> > isn't
> >   held.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/bpf.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> > index f9ba0a46a9f1..0563d3c6d9d3 100644
> > --- a/net/mptcp/bpf.c
> > +++ b/net/mptcp/bpf.c
> > @@ -232,12 +232,20 @@ bpf_iter_mptcp_subflow_new(struct
> > bpf_iter_mptcp_subflow *it,
> >  			   struct mptcp_sock *msk)
> >  {
> >  	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> > +	struct sock *sk = (struct sock *)msk;
> > +
> > +	BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) >
> > +		     sizeof(struct bpf_iter_mptcp_subflow));
> > +	BUILD_BUG_ON(__alignof__(struct
> > bpf_iter_mptcp_subflow_kern) !=
> > +		     __alignof__(struct bpf_iter_mptcp_subflow));
> >  
> >  	kit->msk = msk;
> >  	if (!msk)
> >  		return -EINVAL;
> >  
> > -	msk_owned_by_me(msk);
> > +	if (!sock_owned_by_user_nocheck(sk) &&
> > +	    !spin_is_locked(&sk->sk_lock.slock))
> > +		return -EINVAL;
> 
> Just to be sure, did you check it was always the case with the WIP
> scheduler and path-manager BPF programs you have?

Yes, I checked, BPF packet scheduler and BPF path manager are working
fine. Thanks for your other suggestions on this set, I have updated to
the new version v5.

-Geliang

> 
> We don't want to be too restrictive. (Or we need different helpers)
> 
> Cheers,
> Matt