[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

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.

[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

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.

[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

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.

[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

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

[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.

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

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v4:
 - drop sock_owned_by_user_nocheck and spin_is_locked. According to
   comments from Mat and Martin, in this set mptcp_subflow
   bpf_iter only used from a cg sockopt bpf prog, no need to add these
   check at this moment.

v3:
 - patch 3, continue to use sock_owned_by_user_nocheck() and spin_is_locked()
checks instead of using msk_owned_by_me().
 - patch 5, drop declaration of bpf_mptcp_subflow_tcp_sock. It's no longer
used.
 - patch 5, update the comment for mptcp_subflow_tcp_sock(), which is a BPF
helper, not a kfunc.

The commit log of "bpf: Register mptcp common kfunc set" doesn't match the
code, please update it as:

'''
bpf: Register mptcp common kfunc set

MPTCP helper mptcp_subflow_ctx() is used to convert struct sock to
struct mptcp_subflow_context. It will be used in MPTCP BPF programs.

This patch defines corresponding wrapper of this helper, and put it
into the newly defined mptcp common kfunc set and register this set
with the flag BPF_PROG_TYPE_CGROUP_SOCKOPT to let it accessible to
the 'cgroup/getsockopt' type of BPF programs.
'''

v2:
 - Drop bpf_skc_to_mptcp_sock
 - Check the owner before assigning the msk as Mat suggested.
 - Use bpf_core_cast() in mptcp_subflow bpf_iter subtest instead of
using bpf_skc_to_mptcp_sock().

Address Martin's suggestions for "Add mptcp_subflow bpf_iter support" v2.

Geliang Tang (5):
  Revert "bpf: Extend bpf_skc_to_mptcp_sock to MPTCP sock"
  Revert "bpf: Allow use of skc_to_mptcp_sock in cg_sockopt"
  Squash to "bpf: Add mptcp_subflow bpf_iter"
  Revert "bpf: Acquire and release mptcp socket"
  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                               | 41 ++++---------------
 .../testing/selftests/bpf/bpf_experimental.h  |  2 +-
 tools/testing/selftests/bpf/progs/mptcp_bpf.h |  5 ---
 .../selftests/bpf/progs/mptcp_bpf_iters.c     | 10 ++---
 7 files changed, 15 insertions(+), 51 deletions(-)

-- 
2.43.0

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

[Geliang Tang]

On Mon, 2025-02-24 at 11:37 +0800, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v4:
>  - drop sock_owned_by_user_nocheck and spin_is_locked. According to
>    comments from Mat and Martin, in this set mptcp_subflow
>    bpf_iter only used from a cg sockopt bpf prog, no need to add
> these
>    check at this moment.

The commit log of "bpf: Register mptcp common kfunc set" doesn't match
the code, please update it as:

'''
bpf: Register mptcp common kfunc set

MPTCP helper mptcp_subflow_ctx() is used to convert struct sock to
struct mptcp_subflow_context. It will be used in MPTCP BPF programs.

This patch defines corresponding wrapper of this helper, and put it
into the newly defined mptcp common kfunc set and register this set
with the flag BPF_PROG_TYPE_CGROUP_SOCKOPT to let it accessible to
the 'cgroup/getsockopt' type of BPF programs.
'''

Thanks,
-Geliang

> 
> v3:
>  - patch 3, continue to use sock_owned_by_user_nocheck() and
> spin_is_locked()
> checks instead of using msk_owned_by_me().
>  - patch 5, drop declaration of bpf_mptcp_subflow_tcp_sock. It's no
> longer
> used.
>  - patch 5, update the comment for mptcp_subflow_tcp_sock(), which is
> a BPF
> helper, not a kfunc.
> 
> The commit log of "bpf: Register mptcp common kfunc set" doesn't
> match the
> code, please update it as:
> 
> '''
> bpf: Register mptcp common kfunc set
> 
> MPTCP helper mptcp_subflow_ctx() is used to convert struct sock to
> struct mptcp_subflow_context. It will be used in MPTCP BPF programs.
> 
> This patch defines corresponding wrapper of this helper, and put it
> into the newly defined mptcp common kfunc set and register this set
> with the flag BPF_PROG_TYPE_CGROUP_SOCKOPT to let it accessible to
> the 'cgroup/getsockopt' type of BPF programs.
> '''
> 
> v2:
>  - Drop bpf_skc_to_mptcp_sock
>  - Check the owner before assigning the msk as Mat suggested.
>  - Use bpf_core_cast() in mptcp_subflow bpf_iter subtest instead of
> using bpf_skc_to_mptcp_sock().
> 
> Address Martin's suggestions for "Add mptcp_subflow bpf_iter support"
> v2.
> 
> Geliang Tang (5):
>   Revert "bpf: Extend bpf_skc_to_mptcp_sock to MPTCP sock"
>   Revert "bpf: Allow use of skc_to_mptcp_sock in cg_sockopt"
>   Squash to "bpf: Add mptcp_subflow bpf_iter"
>   Revert "bpf: Acquire and release mptcp socket"
>   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                               | 41 ++++-------------
> --
>  .../testing/selftests/bpf/bpf_experimental.h  |  2 +-
>  tools/testing/selftests/bpf/progs/mptcp_bpf.h |  5 ---
>  .../selftests/bpf/progs/mptcp_bpf_iters.c     | 10 ++---
>  7 files changed, 15 insertions(+), 51 deletions(-)
> 

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: Success! ✅
- 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/13490473539

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


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, Mat,

On 24/02/2025 04:37, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v4:
>  - drop sock_owned_by_user_nocheck and spin_is_locked. According to
>    comments from Mat and Martin, in this set mptcp_subflow
>    bpf_iter only used from a cg sockopt bpf prog, no need to add these
>    check at this moment.

The v4 looks OK to me, but I prefer to wait for Mat's ACK, if that's OK,
because he reviewed the original series.

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

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

[Mat Martineau]

On Mon, 24 Feb 2025, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> v4:
> - drop sock_owned_by_user_nocheck and spin_is_locked. According to
>   comments from Mat and Martin, in this set mptcp_subflow
>   bpf_iter only used from a cg sockopt bpf prog, no need to add these
>   check at this moment.

Hi Geliang -

Sorry to continue the churn on this. I looked at Martin's comment in 
https://lore.kernel.org/mptcp/fdf0ddbe-e007-4a5f-bbdf-9a144e8fbe35@linux.dev/ 
where he says:

"For the cg get/setsockopt hook here, the lock should have already been 
held earlier in the kernel."

For the getsockopt case, there is a file layer lock held that ensures the 
'struct socket' remains valid while this hook is run, but no lock is 
acquired that prevents changes to msk->conn_list. It's not safe to iterate 
over the conn_list without protection from lock_sock() / unlock_sock().

As Matthieu noted in 
https://lore.kernel.org/mptcp/3b5af48e-4155-4b98-b67b-b75d9fb6285e@kernel.org/, 
when used in a bpf scheduler the msk lock will be held. But the test code 
called via getsockopt is a case where it's not protected, and a change to 
the linked list during iteration could lead to undefined behavior.

I'll talk about it with Matthieu in the meeting this week too.

- Mat


>
> v3:
> - patch 3, continue to use sock_owned_by_user_nocheck() and spin_is_locked()
> checks instead of using msk_owned_by_me().
> - patch 5, drop declaration of bpf_mptcp_subflow_tcp_sock. It's no longer
> used.
> - patch 5, update the comment for mptcp_subflow_tcp_sock(), which is a BPF
> helper, not a kfunc.
>
> The commit log of "bpf: Register mptcp common kfunc set" doesn't match the
> code, please update it as:
>
> '''
> bpf: Register mptcp common kfunc set
>
> MPTCP helper mptcp_subflow_ctx() is used to convert struct sock to
> struct mptcp_subflow_context. It will be used in MPTCP BPF programs.
>
> This patch defines corresponding wrapper of this helper, and put it
> into the newly defined mptcp common kfunc set and register this set
> with the flag BPF_PROG_TYPE_CGROUP_SOCKOPT to let it accessible to
> the 'cgroup/getsockopt' type of BPF programs.
> '''
>
> v2:
> - Drop bpf_skc_to_mptcp_sock
> - Check the owner before assigning the msk as Mat suggested.
> - Use bpf_core_cast() in mptcp_subflow bpf_iter subtest instead of
> using bpf_skc_to_mptcp_sock().
>
> Address Martin's suggestions for "Add mptcp_subflow bpf_iter support" v2.
>
> Geliang Tang (5):
>  Revert "bpf: Extend bpf_skc_to_mptcp_sock to MPTCP sock"
>  Revert "bpf: Allow use of skc_to_mptcp_sock in cg_sockopt"
>  Squash to "bpf: Add mptcp_subflow bpf_iter"
>  Revert "bpf: Acquire and release mptcp socket"
>  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                               | 41 ++++---------------
> .../testing/selftests/bpf/bpf_experimental.h  |  2 +-
> tools/testing/selftests/bpf/progs/mptcp_bpf.h |  5 ---
> .../selftests/bpf/progs/mptcp_bpf_iters.c     | 10 ++---
> 7 files changed, 15 insertions(+), 51 deletions(-)
>
> -- 
> 2.43.0
>
>
>

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

[Matthieu Baerts]

Hi Geliang, Mat,

On 24/02/2025 04:37, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v4:
>  - drop sock_owned_by_user_nocheck and spin_is_locked. According to
>    comments from Mat and Martin, in this set mptcp_subflow
>    bpf_iter only used from a cg sockopt bpf prog, no need to add these
>    check at this moment.

Thank you for the patches and the reviews!

Now in our tree:

New patches for t/upstream:
- fe0c24467a00: "squashed" patch 1/5 in "bpf: Extend
bpf_skc_to_mptcp_sock to MPTCP sock"
- 83bb4052ab8a: "squashed" patch 2/5 in "bpf: Allow use of
skc_to_mptcp_sock in cg_sockopt"
- dd8797c750fe: "squashed" patch 3/5 in "bpf: Add mptcp_subflow bpf_iter"
- a5e732a840e2: "squashed" my patch in "bpf: Add mptcp_subflow bpf_iter"
- 23094de26cb5: "squashed" (with conflicts) patch 4/5 in "bpf: Acquire
and release mptcp socket"
- 5a1d91403efd: "squashed" patch 5/5 in "selftests/bpf: Add
mptcp_subflow bpf_iter subtest"
- 29cc864a2cc6: tg:msg: update after the recent squash-to patch
- da6054a670da: conflict in t/mptcp-add-sched_data-helpers-2
- 401780491d45: conflict in t/mptcp-add-bpf_mptcp_sched_ops
- Results: e1315f2b7271..bf7e42dbc2fa (export)

Tests are now in progress:

- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/0447c3073f646734cffeec271b1a6b03a53103ef/checks

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

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

[Geliang Tang]

Hi Matt,

On Mon, 2025-02-24 at 11:37 +0800, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v4:
>  - drop sock_owned_by_user_nocheck and spin_is_locked. According to
>    comments from Mat and Martin, in this set mptcp_subflow
>    bpf_iter only used from a cg sockopt bpf prog, no need to add
> these
>    check at this moment.
> 
> v3:
>  - patch 3, continue to use sock_owned_by_user_nocheck() and
> spin_is_locked()
> checks instead of using msk_owned_by_me().
>  - patch 5, drop declaration of bpf_mptcp_subflow_tcp_sock. It's no
> longer
> used.
>  - patch 5, update the comment for mptcp_subflow_tcp_sock(), which is
> a BPF
> helper, not a kfunc.
> 
> The commit log of "bpf: Register mptcp common kfunc set" doesn't
> match the
> code, please update it as:
> 
> '''
> bpf: Register mptcp common kfunc set
> 
> MPTCP helper mptcp_subflow_ctx() is used to convert struct sock to
> struct mptcp_subflow_context. It will be used in MPTCP BPF programs.
> 
> This patch defines corresponding wrapper of this helper, and put it
> into the newly defined mptcp common kfunc set and register this set
> with the flag BPF_PROG_TYPE_CGROUP_SOCKOPT to let it accessible to
> the 'cgroup/getsockopt' type of BPF programs.
> '''

Thanks for applying this set. Please update this commit log too.

-Geliang

> 
> v2:
>  - Drop bpf_skc_to_mptcp_sock
>  - Check the owner before assigning the msk as Mat suggested.
>  - Use bpf_core_cast() in mptcp_subflow bpf_iter subtest instead of
> using bpf_skc_to_mptcp_sock().
> 
> Address Martin's suggestions for "Add mptcp_subflow bpf_iter support"
> v2.
> 
> Geliang Tang (5):
>   Revert "bpf: Extend bpf_skc_to_mptcp_sock to MPTCP sock"
>   Revert "bpf: Allow use of skc_to_mptcp_sock in cg_sockopt"
>   Squash to "bpf: Add mptcp_subflow bpf_iter"
>   Revert "bpf: Acquire and release mptcp socket"
>   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                               | 41 ++++-------------
> --
>  .../testing/selftests/bpf/bpf_experimental.h  |  2 +-
>  tools/testing/selftests/bpf/progs/mptcp_bpf.h |  5 ---
>  .../selftests/bpf/progs/mptcp_bpf_iters.c     | 10 ++---
>  7 files changed, 15 insertions(+), 51 deletions(-)
> 

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

[Matthieu Baerts]

Hi Geliang,

On 06/03/2025 02:50, Geliang Tang wrote:
> Hi Matt,
> 
> On Mon, 2025-02-24 at 11:37 +0800, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> v4:
>>  - drop sock_owned_by_user_nocheck and spin_is_locked. According to
>>    comments from Mat and Martin, in this set mptcp_subflow
>>    bpf_iter only used from a cg sockopt bpf prog, no need to add
>> these
>>    check at this moment.
>>
>> v3:
>>  - patch 3, continue to use sock_owned_by_user_nocheck() and
>> spin_is_locked()
>> checks instead of using msk_owned_by_me().
>>  - patch 5, drop declaration of bpf_mptcp_subflow_tcp_sock. It's no
>> longer
>> used.
>>  - patch 5, update the comment for mptcp_subflow_tcp_sock(), which is
>> a BPF
>> helper, not a kfunc.
>>
>> The commit log of "bpf: Register mptcp common kfunc set" doesn't
>> match the
>> code, please update it as:
>>
>> '''
>> bpf: Register mptcp common kfunc set
>>
>> MPTCP helper mptcp_subflow_ctx() is used to convert struct sock to
>> struct mptcp_subflow_context. It will be used in MPTCP BPF programs.
>>
>> This patch defines corresponding wrapper of this helper, and put it
>> into the newly defined mptcp common kfunc set and register this set
>> with the flag BPF_PROG_TYPE_CGROUP_SOCKOPT to let it accessible to
>> the 'cgroup/getsockopt' type of BPF programs.
>> '''
> 
> Thanks for applying this set. Please update this commit log too.

Done. The modification will be visible soon:

New patches for t/upstream:
- 061566ebfca8: tg:msg: adapt 'bpf: Register mptcp common kfunc set'
- Results: 6c4b1c674680..ccf37aa90567 (export)

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