[PATCH mptcp-next v7 0/4] fixes for "new MPTCP subflow subtest v4"

[PATCH mptcp-next v7 0/4] fixes for "new MPTCP subflow subtest v4"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v7:
 - drop helpers mptcp_for_each_subflow_safe and
   list_for_each_entry_safe.
 - update parameters of _check_getsockopt_subflow_mark/cc.
 - print more messages in the logs as Matt suggested.

v6:
 - address Matt's comments in v5.
 - use mptcp_for_each_subflow in _check_getsockopt_subflow_mark.
 - check msk->pm.subflows in _check_getsockopt_subflow_mark/cc.
 - add wait_for_new_subflows helper.

v5:
 - rename _check_getsockopt_subflows_mark in patch 1.
 - address Matt's comments in v4.

v4:
 - address Matt's comments in v3.

v3:
 - drop "ss_search" from subflow subtest
 - fix mptcp_for_each_subflow_safe
 - add cond_break in list_for_each_entry/_safe

Two fixes address Martin's comments in "new MPTCP subflow subtest" v4.
Patch 2 will conflict with "selftests/bpf: Add bpf_first scheduler & test".

Geliang Tang (4):
  Squash to "selftests/bpf: Add mptcp subflow example"
  selftests/bpf: Add getsockopt to inspect mptcp subflow
  Squash to "selftests/bpf: Add mptcp subflow subtest"
  Squash to "selftests/bpf: Add bpf scheduler test"

 .../testing/selftests/bpf/prog_tests/mptcp.c  | 65 ++++++++-----
 tools/testing/selftests/bpf/progs/mptcp_bpf.h | 27 ++++++
 .../selftests/bpf/progs/mptcp_subflow.c       | 92 ++++++++++++++++++-
 3 files changed, 162 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v7 1/4] Squash to "selftests/bpf: Add mptcp subflow example"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

For easier verification of cc, set cc of the second subflow as "reno",
instead of the first one.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/progs/mptcp_subflow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
index bc572e1d6df8..2e28f4a215b5 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
@@ -52,7 +52,7 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
 	err = bpf_setsockopt(skops, SOL_SOCKET, SO_MARK, &mark, sizeof(mark));
 	if (err < 0)
 		return 1;
-	if (mark == 1)
+	if (mark == 2)
 		err = bpf_setsockopt(skops, SOL_TCP, TCP_CONGESTION, cc, TCP_CA_NAME_MAX);
 
 	return 1;
-- 
2.43.0

[PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a "cgroup/getsockopt" way to inspect the subflows of a
mptcp socket.

mptcp_for_each_stubflow() and other helpers related to list_dentry are
added into progs/mptcp_bpf.h.

Add an extra "cgroup/getsockopt" prog to walk the msk->conn_list and use
bpf_core_cast to cast a pointer to tcp_sock for readonly. It will allow
to inspect all the fields in a tcp_sock.

Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/progs/mptcp_bpf.h | 27 ++++++
 .../selftests/bpf/progs/mptcp_subflow.c       | 90 +++++++++++++++++++
 2 files changed, 117 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
index 782f36ed027e..92d5deed0214 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
@@ -4,9 +4,36 @@
 
 #include <vmlinux.h>
 #include <bpf/bpf_core_read.h>
+#include "bpf_experimental.h"
 
 #define MPTCP_SUBFLOWS_MAX 8
 
+static inline int list_is_head(const struct list_head *list,
+			       const struct list_head *head)
+{
+	return list == head;
+}
+
+#define list_entry(ptr, type, member)					\
+	container_of(ptr, type, member)
+
+#define list_first_entry(ptr, type, member)				\
+	list_entry((ptr)->next, type, member)
+
+#define list_next_entry(pos, member)					\
+	list_entry((pos)->member.next, typeof(*(pos)), member)
+
+#define list_entry_is_head(pos, head, member)				\
+	list_is_head(&pos->member, (head))
+
+#define list_for_each_entry(pos, head, member)				\
+	for (pos = list_first_entry(head, typeof(*pos), member);	\
+	     cond_break, !list_entry_is_head(pos, head, member);	\
+	     pos = list_next_entry(pos, member))
+
+#define mptcp_for_each_subflow(__msk, __subflow)			\
+	list_for_each_entry(__subflow, &((__msk)->conn_list), node)
+
 extern void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 					bool scheduled) __ksym;
 
diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
index 2e28f4a215b5..1053a795eb43 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
@@ -4,6 +4,7 @@
 
 /* vmlinux.h, bpf_helpers.h and other 'define' */
 #include "bpf_tracing_net.h"
+#include "mptcp_bpf.h"
 
 char _license[] SEC("license") = "GPL";
 
@@ -57,3 +58,92 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
 
 	return 1;
 }
+
+static int _check_getsockopt_subflow_mark(struct bpf_sock *sk)
+{
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_sock *msk;
+	int i = 0;
+
+	if (sk->protocol != IPPROTO_MPTCP) {
+		bpf_printk("MPTCP Subflow: unexpected protocol %u", sk->protocol);
+		return -1;
+	}
+
+	msk = bpf_core_cast(sk, struct mptcp_sock);
+	if (msk->pm.subflows != 1) {
+		bpf_printk("MPTCP Subflow: expected 1 extra subflows, got %u",
+			   msk->pm.subflows);
+		return -1;
+	}
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk;
+
+		ssk = mptcp_subflow_tcp_sock(bpf_core_cast(subflow,
+							   struct mptcp_subflow_context));
+
+		if (ssk->sk_mark != ++i) {
+			bpf_printk("MPTCP Subflow: expected %d mark, got %d",
+				   i, ssk->sk_mark);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
+static int _check_getsockopt_subflow_cc(struct bpf_sock *sk)
+{
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_sock *msk;
+
+	if (sk->protocol != IPPROTO_MPTCP) {
+		bpf_printk("MPTCP Subflow: unexpected protocol %u", sk->protocol);
+		return -1;
+	}
+
+	msk = bpf_core_cast(sk, struct mptcp_sock);
+	if (msk->pm.subflows != 1) {
+		bpf_printk("MPTCP Subflow: expected 1 extra subflows, got %u",
+			   msk->pm.subflows);
+		return -1;
+	}
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct inet_connection_sock *icsk;
+		struct sock *ssk;
+
+		ssk = mptcp_subflow_tcp_sock(bpf_core_cast(subflow,
+							   struct mptcp_subflow_context));
+		icsk = bpf_core_cast(ssk, struct inet_connection_sock);
+
+		if (ssk->sk_mark == 2 &&
+		    __builtin_memcmp(icsk->icsk_ca_ops->name, cc, TCP_CA_NAME_MAX)) {
+			bpf_printk("MPTCP Subflow: expected %s cc, got %s",
+				   cc, icsk->icsk_ca_ops->name);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
+SEC("cgroup/getsockopt")
+int _getsockopt_subflow(struct bpf_sockopt *ctx)
+{
+	struct bpf_sock *sk = ctx->sk;
+
+	if (!sk) {
+		bpf_printk("MPTCP Subflow: unexpected socket, sk is NULL");
+		ctx->retval = -1;
+		return 1;
+	}
+
+	if (ctx->level == SOL_SOCKET && ctx->optname == SO_MARK)
+		ctx->retval = _check_getsockopt_subflow_mark(sk);
+	else if (ctx->level == SOL_TCP && ctx->optname == TCP_CONGESTION)
+		ctx->retval = _check_getsockopt_subflow_cc(sk);
+
+	return 1;
+}
-- 
2.43.0

Re: [PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Matthieu Baerts]

Hi Geliang,

Thank you for the new version.

On 03/09/2024 10:04, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a "cgroup/getsockopt" way to inspect the subflows of a
> mptcp socket.
> 
> mptcp_for_each_stubflow() and other helpers related to list_dentry are
> added into progs/mptcp_bpf.h.
> 
> Add an extra "cgroup/getsockopt" prog to walk the msk->conn_list and use
> bpf_core_cast to cast a pointer to tcp_sock for readonly. It will allow
> to inspect all the fields in a tcp_sock.

(...)

> diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> index 2e28f4a215b5..1053a795eb43 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> @@ -4,6 +4,7 @@
>  
>  /* vmlinux.h, bpf_helpers.h and other 'define' */
>  #include "bpf_tracing_net.h"
> +#include "mptcp_bpf.h"
>  
>  char _license[] SEC("license") = "GPL";
>  
> @@ -57,3 +58,92 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
>  
>  	return 1;
>  }
> +
> +static int _check_getsockopt_subflow_mark(struct bpf_sock *sk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct mptcp_sock *msk;
> +	int i = 0;
> +
> +	if (sk->protocol != IPPROTO_MPTCP) {
> +		bpf_printk("MPTCP Subflow: unexpected protocol %u", sk->protocol);
> +		return -1;
> +	}

Out of curiosity, why did you move this code here and in
_check_getsockopt_subflow_cc()?

I'm just surprised because recently, you tried to reduce the amount of
duplicated code :)

Or is it because this program is used in other tests, where it should
not be? Do you need to add a restriction per PID, like you did in
mptcpify.c?

(...)

> +SEC("cgroup/getsockopt")
> +int _getsockopt_subflow(struct bpf_sockopt *ctx)
> +{
> +	struct bpf_sock *sk = ctx->sk;
> +
> +	if (!sk) {
> +		bpf_printk("MPTCP Subflow: unexpected socket, sk is NULL");
> +		ctx->retval = -1;
> +		return 1;
> +	}
> +
> +	if (ctx->level == SOL_SOCKET && ctx->optname == SO_MARK)
> +		ctx->retval = _check_getsockopt_subflow_mark(sk);
> +	else if (ctx->level == SOL_TCP && ctx->optname == TCP_CONGESTION)
> +		ctx->retval = _check_getsockopt_subflow_cc(sk);

As I mentioned in my previous reply: can ctx->retval be < 0 here, before
calling _check_getsockopt_subflow_XXX()?
If yes or not sure, maybe safer to do:

  err = _check_getsockopt_subflow_XXX()
  (...)

  if (err)
      ctx->retval = -1;

> +
> +	return 1;
> +}

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

Re: [PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Matthieu Baerts]

On 03/09/2024 11:11, Matthieu Baerts wrote:
> On 03/09/2024 10:04, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> This patch adds a "cgroup/getsockopt" way to inspect the subflows of a
>> mptcp socket.
>>
>> mptcp_for_each_stubflow() and other helpers related to list_dentry are
>> added into progs/mptcp_bpf.h.
>>
>> Add an extra "cgroup/getsockopt" prog to walk the msk->conn_list and use
>> bpf_core_cast to cast a pointer to tcp_sock for readonly. It will allow
>> to inspect all the fields in a tcp_sock.
> 
> (...)
> 
>> diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>> index 2e28f4a215b5..1053a795eb43 100644
>> --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>> +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>> @@ -4,6 +4,7 @@
>>  
>>  /* vmlinux.h, bpf_helpers.h and other 'define' */
>>  #include "bpf_tracing_net.h"
>> +#include "mptcp_bpf.h"
>>  
>>  char _license[] SEC("license") = "GPL";
>>  
>> @@ -57,3 +58,92 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
>>  
>>  	return 1;
>>  }
>> +
>> +static int _check_getsockopt_subflow_mark(struct bpf_sock *sk)
>> +{
>> +	struct mptcp_subflow_context *subflow;
>> +	struct mptcp_sock *msk;
>> +	int i = 0;
>> +
>> +	if (sk->protocol != IPPROTO_MPTCP) {
>> +		bpf_printk("MPTCP Subflow: unexpected protocol %u", sk->protocol);
>> +		return -1;
>> +	}
> 
> Out of curiosity, why did you move this code here and in
> _check_getsockopt_subflow_cc()?
> 
> I'm just surprised because recently, you tried to reduce the amount of
> duplicated code :)

While at it: it sounds better to have unique error messages, to know
where was the error. Here, a few messages are the same, e.g. protocol,
number of subflows, sk is null. Do not hesitate to add something to
differentiate them: level & optname, a string prefix, the line number, etc.

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

Re: [PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Geliang Tang]

On Tue, 2024-09-03 at 11:16 +0200, Matthieu Baerts wrote:
> On 03/09/2024 11:11, Matthieu Baerts wrote:
> > On 03/09/2024 10:04, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > This patch adds a "cgroup/getsockopt" way to inspect the subflows
> > > of a
> > > mptcp socket.
> > > 
> > > mptcp_for_each_stubflow() and other helpers related to
> > > list_dentry are
> > > added into progs/mptcp_bpf.h.
> > > 
> > > Add an extra "cgroup/getsockopt" prog to walk the msk->conn_list
> > > and use
> > > bpf_core_cast to cast a pointer to tcp_sock for readonly. It will
> > > allow
> > > to inspect all the fields in a tcp_sock.
> > 
> > (...)
> > 
> > > diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > index 2e28f4a215b5..1053a795eb43 100644
> > > --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > @@ -4,6 +4,7 @@
> > >  
> > >  /* vmlinux.h, bpf_helpers.h and other 'define' */
> > >  #include "bpf_tracing_net.h"
> > > +#include "mptcp_bpf.h"
> > >  
> > >  char _license[] SEC("license") = "GPL";
> > >  
> > > @@ -57,3 +58,92 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
> > >  
> > >  	return 1;
> > >  }
> > > +
> > > +static int _check_getsockopt_subflow_mark(struct bpf_sock *sk)
> > > +{
> > > +	struct mptcp_subflow_context *subflow;
> > > +	struct mptcp_sock *msk;
> > > +	int i = 0;
> > > +
> > > +	if (sk->protocol != IPPROTO_MPTCP) {
> > > +		bpf_printk("MPTCP Subflow: unexpected protocol
> > > %u", sk->protocol);
> > > +		return -1;
> > > +	}
> > 
> > Out of curiosity, why did you move this code here and in
> > _check_getsockopt_subflow_cc()?
> > 
> > I'm just surprised because recently, you tried to reduce the amount
> > of
> > duplicated code :)
> 
> While at it: it sounds better to have unique error messages, to know
> where was the error. Here, a few messages are the same, e.g.
> protocol,
> number of subflows, sk is null. Do not hesitate to add something to
> differentiate them: level & optname, a string prefix, the line
> number, etc.

No, I think we should drop all these "bpf_printk" messages. I checked
all other BPF selftests programs, no one use these error messages to
debug like this. Just "return 1" is enough:

        if (!sk || sk->protocol != IPPROTO_MPTCP)
                return 1;

If a BPF program fail, its own load log will show:

libbpf: prog '_getsockopt_subflow': -- BEGIN PROG LOAD LOG --
0: R1=ctx() R10=fp0
; int _getsockopt_subflow(struct bpf_sockopt *ctx) @
mptcp_subflow.c:136
0: (bf) r9 = r1                       ; R1=ctx() R9_w=ctx()
; struct bpf_sock *sk = ctx->sk; @ mptcp_subflow.c:138
1: (79) r7 = *(u64 *)(r9 +0)          ; R7_w=sock() R9_w=ctx()
; if (bpf_get_current_pid_tgid() >> 32 != pid) @ mptcp_subflow.c:141
2: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()

No need to add any line number by ourselves.

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Matthieu Baerts]

Hi Geliang,

Thank you for your reply!

On 04/09/2024 11:55, Geliang Tang wrote:
> On Tue, 2024-09-03 at 11:16 +0200, Matthieu Baerts wrote:
>> On 03/09/2024 11:11, Matthieu Baerts wrote:
>>> On 03/09/2024 10:04, Geliang Tang wrote:
>>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>>
>>>> This patch adds a "cgroup/getsockopt" way to inspect the subflows
>>>> of a
>>>> mptcp socket.
>>>>
>>>> mptcp_for_each_stubflow() and other helpers related to
>>>> list_dentry are
>>>> added into progs/mptcp_bpf.h.
>>>>
>>>> Add an extra "cgroup/getsockopt" prog to walk the msk->conn_list
>>>> and use
>>>> bpf_core_cast to cast a pointer to tcp_sock for readonly. It will
>>>> allow
>>>> to inspect all the fields in a tcp_sock.
>>>
>>> (...)
>>>
>>>> diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>>>> b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>>>> index 2e28f4a215b5..1053a795eb43 100644
>>>> --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>>>> +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>>>> @@ -4,6 +4,7 @@
>>>>  
>>>>  /* vmlinux.h, bpf_helpers.h and other 'define' */
>>>>  #include "bpf_tracing_net.h"
>>>> +#include "mptcp_bpf.h"
>>>>  
>>>>  char _license[] SEC("license") = "GPL";
>>>>  
>>>> @@ -57,3 +58,92 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
>>>>  
>>>>  	return 1;
>>>>  }
>>>> +
>>>> +static int _check_getsockopt_subflow_mark(struct bpf_sock *sk)
>>>> +{
>>>> +	struct mptcp_subflow_context *subflow;
>>>> +	struct mptcp_sock *msk;
>>>> +	int i = 0;
>>>> +
>>>> +	if (sk->protocol != IPPROTO_MPTCP) {
>>>> +		bpf_printk("MPTCP Subflow: unexpected protocol
>>>> %u", sk->protocol);
>>>> +		return -1;
>>>> +	}
>>>
>>> Out of curiosity, why did you move this code here and in
>>> _check_getsockopt_subflow_cc()?
>>>
>>> I'm just surprised because recently, you tried to reduce the amount
>>> of
>>> duplicated code :)
>>
>> While at it: it sounds better to have unique error messages, to know
>> where was the error. Here, a few messages are the same, e.g.
>> protocol,
>> number of subflows, sk is null. Do not hesitate to add something to
>> differentiate them: level & optname, a string prefix, the line
>> number, etc.
> 
> No, I think we should drop all these "bpf_printk" messages. I checked
> all other BPF selftests programs, no one use these error messages to
> debug like this. Just "return 1" is enough:
> 
>         if (!sk || sk->protocol != IPPROTO_MPTCP)
>                 return 1;
> 
> If a BPF program fail, its own load log will show:
> 
> libbpf: prog '_getsockopt_subflow': -- BEGIN PROG LOAD LOG --
> 0: R1=ctx() R10=fp0
> ; int _getsockopt_subflow(struct bpf_sockopt *ctx) @
> mptcp_subflow.c:136
> 0: (bf) r9 = r1                       ; R1=ctx() R9_w=ctx()
> ; struct bpf_sock *sk = ctx->sk; @ mptcp_subflow.c:138
> 1: (79) r7 = *(u64 *)(r9 +0)          ; R7_w=sock() R9_w=ctx()
> ; if (bpf_get_current_pid_tgid() >> 32 != pid) @ mptcp_subflow.c:141
> 2: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()
> 
> No need to add any line number by ourselves.

Good, that looks OK.

Just to be sure it is readable: here the BPF program failed because the
PID was not the expected one, right? Or is it another error?

Do you mind sharing the output if the CC is not "foo" for example please?

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

Re: [PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Geliang Tang]

On Tue, 2024-09-03 at 11:11 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the new version.
> 
> On 03/09/2024 10:04, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch adds a "cgroup/getsockopt" way to inspect the subflows
> > of a
> > mptcp socket.
> > 
> > mptcp_for_each_stubflow() and other helpers related to list_dentry
> > are
> > added into progs/mptcp_bpf.h.
> > 
> > Add an extra "cgroup/getsockopt" prog to walk the msk->conn_list
> > and use
> > bpf_core_cast to cast a pointer to tcp_sock for readonly. It will
> > allow
> > to inspect all the fields in a tcp_sock.
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > index 2e28f4a215b5..1053a795eb43 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > @@ -4,6 +4,7 @@
> >  
> >  /* vmlinux.h, bpf_helpers.h and other 'define' */
> >  #include "bpf_tracing_net.h"
> > +#include "mptcp_bpf.h"
> >  
> >  char _license[] SEC("license") = "GPL";
> >  
> > @@ -57,3 +58,92 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
> >  
> >  	return 1;
> >  }
> > +
> > +static int _check_getsockopt_subflow_mark(struct bpf_sock *sk)
> > +{
> > +	struct mptcp_subflow_context *subflow;
> > +	struct mptcp_sock *msk;
> > +	int i = 0;
> > +
> > +	if (sk->protocol != IPPROTO_MPTCP) {
> > +		bpf_printk("MPTCP Subflow: unexpected protocol
> > %u", sk->protocol);
> > +		return -1;
> > +	}
> 
> Out of curiosity, why did you move this code here and in
> _check_getsockopt_subflow_cc()?
> 
> I'm just surprised because recently, you tried to reduce the amount
> of
> duplicated code :)
> 
> Or is it because this program is used in other tests, where it should
> not be? Do you need to add a restriction per PID, like you did in
> mptcpify.c?

I don't know, I'll add "per PID" in next version.

> 
> (...)
> 
> > +SEC("cgroup/getsockopt")
> > +int _getsockopt_subflow(struct bpf_sockopt *ctx)
> > +{
> > +	struct bpf_sock *sk = ctx->sk;
> > +
> > +	if (!sk) {
> > +		bpf_printk("MPTCP Subflow: unexpected socket, sk
> > is NULL");
> > +		ctx->retval = -1;
> > +		return 1;
> > +	}
> > +
> > +	if (ctx->level == SOL_SOCKET && ctx->optname == SO_MARK)
> > +		ctx->retval = _check_getsockopt_subflow_mark(sk);
> > +	else if (ctx->level == SOL_TCP && ctx->optname ==
> > TCP_CONGESTION)
> > +		ctx->retval = _check_getsockopt_subflow_cc(sk);
> 
> As I mentioned in my previous reply: can ctx->retval be < 0 here,
> before
> calling _check_getsockopt_subflow_XXX()?
> If yes or not sure, maybe safer to do:
> 
>   err = _check_getsockopt_subflow_XXX()
>   (...)
> 
>   if (err)
>       ctx->retval = -1;
    else
        ctx->retval = 0;

Otherwise:

86: (63) *(u32 *)(r6 +0) = r1
dereference of modified ctx ptr R6 off=36 disallowed
processed 169 insns (limit 1000000) max_states_per_insn 2 total_states
15 peak_states 15 mark_read 4
-- END PROG LOAD LOG --

> 
> > +
> > +	return 1;
> > +}
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Matthieu Baerts]

On 04/09/2024 12:09, Geliang Tang wrote:
> On Tue, 2024-09-03 at 11:11 +0200, Matthieu Baerts wrote:
>> On 03/09/2024 10:04, Geliang Tang wrote:

(...)

>>> +SEC("cgroup/getsockopt")
>>> +int _getsockopt_subflow(struct bpf_sockopt *ctx)
>>> +{
>>> +	struct bpf_sock *sk = ctx->sk;
>>> +
>>> +	if (!sk) {
>>> +		bpf_printk("MPTCP Subflow: unexpected socket, sk
>>> is NULL");
>>> +		ctx->retval = -1;
>>> +		return 1;
>>> +	}
>>> +
>>> +	if (ctx->level == SOL_SOCKET && ctx->optname == SO_MARK)
>>> +		ctx->retval = _check_getsockopt_subflow_mark(sk);
>>> +	else if (ctx->level == SOL_TCP && ctx->optname ==
>>> TCP_CONGESTION)
>>> +		ctx->retval = _check_getsockopt_subflow_cc(sk);
>>
>> As I mentioned in my previous reply: can ctx->retval be < 0 here,
>> before
>> calling _check_getsockopt_subflow_XXX()?
>> If yes or not sure, maybe safer to do:
>>
>>   err = _check_getsockopt_subflow_XXX()
>>   (...)
>>
>>   if (err)
>>       ctx->retval = -1;
>     else
>         ctx->retval = 0;
> 
> Otherwise:
> 
> 86: (63) *(u32 *)(r6 +0) = r1
> dereference of modified ctx ptr R6 off=36 disallowed
> processed 169 insns (limit 1000000) max_states_per_insn 2 total_states
> 15 peak_states 15 mark_read 4
> -- END PROG LOAD LOG --

Sorry, I'm not familiar with these errors: do you mean that 'retval'
always need to be set? But in a previous version you sent, 'retval' was
only set to -1 in case of error, no?

The error message doesn't seem to say that.
Are you sure it is not because 'err' was not initialised, or something
like that?

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

[PATCH mptcp-next v7 3/4] Squash to "selftests/bpf: Add mptcp subflow subtest"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Drop ss_search() from run_subflow().

Use the "cgroup/getsockopt" way to inspect the subflows.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 57 ++++++++++++-------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 73adc58cd776..6085aaf61b27 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -368,21 +368,29 @@ static int endpoint_init(char *flags)
 	return -1;
 }
 
-static int _ss_search(char *src, char *dst, char *port, char *keyword)
+static void wait_for_new_subflows(int fd)
 {
-	return SYS_NOFAIL("ip netns exec %s ss -enita src %s dst %s %s %d | grep -q '%s'",
-			  NS_TEST, src, dst, port, PORT_1, keyword);
-}
+	socklen_t len;
+	u8 subflows;
+	int err, i;
 
-static int ss_search(char *src, char *keyword)
-{
-	return _ss_search(src, ADDR_1, "dport", keyword);
+	len = sizeof(subflows);
+	/* Wait max 1 sec for new subflows to be created */
+	for (i = 0; i < 10; i++) {
+		err = getsockopt(fd, SOL_MPTCP, MPTCP_INFO, &subflows, &len);
+		if (!err && subflows > 0)
+			break;
+
+		sleep(0.1);
+	}
 }
 
-static void run_subflow(char *new)
+static void run_subflow(void)
 {
 	int server_fd, client_fd, err;
+	char new[TCP_CA_NAME_MAX];
 	char cc[TCP_CA_NAME_MAX];
+	unsigned int mark;
 	socklen_t len;
 
 	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
@@ -393,19 +401,21 @@ static void run_subflow(char *new)
 	if (!ASSERT_GE(client_fd, 0, "connect to fd"))
 		goto close_server;
 
-	len = sizeof(cc);
-	err = getsockopt(server_fd, SOL_TCP, TCP_CONGESTION, cc, &len);
-	if (!ASSERT_OK(err, "getsockopt(server_fd, TCP_CONGESTION)"))
-		goto close_client;
-
 	send_byte(client_fd);
+	wait_for_new_subflows(client_fd);
+
+	len = sizeof(mark);
+	err = getsockopt(client_fd, SOL_SOCKET, SO_MARK, &mark, &len);
+	if (ASSERT_OK(err, "getsockopt(client_fd, SO_MARK)"))
+		ASSERT_EQ(mark, 0, "mark");
+
+	len = sizeof(new);
+	err = getsockopt(client_fd, SOL_TCP, TCP_CONGESTION, new, &len);
+	if (ASSERT_OK(err, "getsockopt(client_fd, TCP_CONGESTION)")) {
+		get_msk_ca_name(cc);
+		ASSERT_STREQ(new, cc, "cc");
+	}
 
-	ASSERT_OK(ss_search(ADDR_1, "fwmark:0x1"), "ss_search fwmark:0x1");
-	ASSERT_OK(ss_search(ADDR_2, "fwmark:0x2"), "ss_search fwmark:0x2");
-	ASSERT_OK(ss_search(ADDR_1, new), "ss_search new cc");
-	ASSERT_OK(ss_search(ADDR_2, cc), "ss_search default cc");
-
-close_client:
 	close(client_fd);
 close_server:
 	close(server_fd);
@@ -416,6 +426,7 @@ static void test_subflow(void)
 	int cgroup_fd, prog_fd, err;
 	struct mptcp_subflow *skel;
 	struct nstoken *nstoken;
+	struct bpf_link *link;
 
 	cgroup_fd = test__join_cgroup("/mptcp_subflow");
 	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup: mptcp_subflow"))
@@ -441,8 +452,14 @@ static void test_subflow(void)
 	if (endpoint_init("subflow") < 0)
 		goto close_netns;
 
-	run_subflow(skel->data->cc);
+	link = bpf_program__attach_cgroup(skel->progs._getsockopt_subflow,
+					  cgroup_fd);
+	if (!ASSERT_OK_PTR(link, "getsockopt prog"))
+		goto close_netns;
+
+	run_subflow();
 
+	bpf_link__destroy(link);
 close_netns:
 	cleanup_netns(nstoken);
 skel_destroy:
-- 
2.43.0

[PATCH mptcp-next v7 4/4] Squash to "selftests/bpf: Add bpf scheduler test"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Now ss_search() are only used by bpf_sched tests. It will be dropped
in next step.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 6085aaf61b27..8f22416e00fb 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -487,9 +487,15 @@ static struct nstoken *sched_init(char *flags, char *sched)
 	return NULL;
 }
 
+static int ss_search(char *src, char *dst, char *port, char *keyword)
+{
+	return SYS_NOFAIL("ip netns exec %s ss -enita src %s dst %s %s %d | grep -q '%s'",
+			  NS_TEST, src, dst, port, PORT_1, keyword);
+}
+
 static int has_bytes_sent(char *dst)
 {
-	return _ss_search(ADDR_1, dst, "sport", "bytes_sent:");
+	return ss_search(ADDR_1, dst, "sport", "bytes_sent:");
 }
 
 static void send_data_and_verify(char *sched, bool addr1, bool addr2)
-- 
2.43.0

Re: [PATCH mptcp-next v7 0/4] fixes for "new MPTCP subflow subtest v4"

[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 (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10679196481

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


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)