This patch extended the MPTCP test base, to exercise bpf_mptcp_sock() from
C test as Alexei suggested in [1].

[1]
https://lore.kernel.org/netdev/20200922040830.3iis6xiavhvpfq3v@ast-mbp.dhcp.thefacebook.com/

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c |  1 +
 tools/testing/selftests/bpf/progs/mptcp.c      | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 04aef0f147dc..eba1b6d12a8c 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -6,6 +6,7 @@
 struct mptcp_storage {
 	__u32 invoked;
 	__u32 is_mptcp;
+	__u32 token;
 };
 
 static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
diff --git a/tools/testing/selftests/bpf/progs/mptcp.c b/tools/testing/selftests/bpf/progs/mptcp.c
index be5ee8dac2b3..ea614e02c8e1 100644
--- a/tools/testing/selftests/bpf/progs/mptcp.c
+++ b/tools/testing/selftests/bpf/progs/mptcp.c
@@ -8,6 +8,7 @@ __u32 _version SEC("version") = 1;
 struct mptcp_storage {
 	__u32 invoked;
 	__u32 is_mptcp;
+	__u32 token;
 };
 
 struct {
@@ -20,6 +21,7 @@ struct {
 SEC("sockops")
 int _sockops(struct bpf_sock_ops *ctx)
 {
+	char fmt[] = "invoked=%u is_mptcp=%u token=%u\n";
 	struct mptcp_storage *storage;
 	struct bpf_tcp_sock *tcp_sk;
 	int op = (int)ctx->op;
@@ -43,6 +45,20 @@ int _sockops(struct bpf_sock_ops *ctx)
 
 	storage->invoked++;
 	storage->is_mptcp = tcp_sk->is_mptcp;
+	storage->token = 0;
+
+	if (tcp_sk->is_mptcp) {
+		struct bpf_mptcp_sock *msk;
+
+		msk = bpf_mptcp_sock(sk);
+		if (!msk)
+			return 1;
+
+		storage->token = msk->token;
+	}
+
+	bpf_trace_printk(fmt, sizeof(fmt),
+			 storage->invoked, storage->is_mptcp, storage->token);
 
 	return 1;
 }
-- 
2.34.1

Hi Geliang,

On 24/02/2022 16:21, Geliang Tang wrote:
> This patch extended the MPTCP test base, to exercise bpf_mptcp_sock() from
> C test as Alexei suggested in [1].

Thank you for this patch!

> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 04aef0f147dc..eba1b6d12a8c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -6,6 +6,7 @@
>  struct mptcp_storage {
>  	__u32 invoked;
>  	__u32 is_mptcp;
> +	__u32 token;
>  };
>  
>  static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)

Do you think we could verify that the token has been properly read and
stored here? I know there were some limitations but probably we can work
around them, no? We could store items on the msk and not the ssk for
MPTCP connections, would that not help?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Hi Matt,

On Tue, Mar 01, 2022 at 12:56:08PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 24/02/2022 16:21, Geliang Tang wrote:
> > This patch extended the MPTCP test base, to exercise bpf_mptcp_sock() from
> > C test as Alexei suggested in [1].
> 
> Thank you for this patch!
> 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > index 04aef0f147dc..eba1b6d12a8c 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > @@ -6,6 +6,7 @@
> >  struct mptcp_storage {
> >  	__u32 invoked;
> >  	__u32 is_mptcp;
> > +	__u32 token;
> >  };
> >  
> >  static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
> 
> Do you think we could verify that the token has been properly read and
> stored here? I know there were some limitations but probably we can work
> around them, no? We could store items on the msk and not the ssk for
> MPTCP connections, would that not help?

Yes, we should verify the token. Could you please give me more details
about how to store items on the msk? I don't know how the items are stored
on the ssk either, please show me which code is used to implement this.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

Hi Geliang,

On 03/03/2022 08:31, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, Mar 01, 2022 at 12:56:08PM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 24/02/2022 16:21, Geliang Tang wrote:
>>> This patch extended the MPTCP test base, to exercise bpf_mptcp_sock() from
>>> C test as Alexei suggested in [1].
>>
>> Thank you for this patch!
>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
>>> index 04aef0f147dc..eba1b6d12a8c 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
>>> @@ -6,6 +6,7 @@
>>>  struct mptcp_storage {
>>>  	__u32 invoked;
>>>  	__u32 is_mptcp;
>>> +	__u32 token;
>>>  };
>>>  
>>>  static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
>>
>> Do you think we could verify that the token has been properly read and
>> stored here? I know there were some limitations but probably we can work
>> around them, no? We could store items on the msk and not the ssk for
>> MPTCP connections, would that not help?
> 
> Yes, we should verify the token. Could you please give me more details
> about how to store items on the msk? I don't know how the items are stored
> on the ssk either, please show me which code is used to implement this.

The difficulty with MPTCP is that many events on the kernel side will be
linked to a subflow. But the userspace only knows the MPTCP socket.

Because of that, it seems to make sense to store info per msk to be able
to find info from the userspace from a fd.

Regarding how to do that, probably easier to look at the code you modified:

> @@ -43,6 +45,20 @@ int _sockops(struct bpf_sock_ops *ctx)
>  
>  	storage->invoked++;
>  	storage->is_mptcp = tcp_sk->is_mptcp;
> +	storage->token = 0;
> +
> +	if (tcp_sk->is_mptcp) {
> +		struct bpf_mptcp_sock *msk;
> +
> +		msk = bpf_mptcp_sock(sk);

After the modification you did in 'Squash to "bpf: add 'bpf_mptcp_sock'
structure and helper"', 'bpf_mptcp_sock()' now returns a generic "struct
sock *" which is in fact a "struct mptcp_sock *" if I'm not mistaken.

So we should have:

    struct mptcp_sock *msk = bpf_mptcp_sock(sk);

To be able to use this structure, I guess we then need to move the
definition of "struct mptcp_sock" from net/mptcp/protocol.h to
include/net/mptcp.h.

Now that you have a "real" sock, I guess you can use
bpf_sk_storage_get() with the msk instead of the sk. You can store the
token in this map but what might be very interesting is to store an
array of data about the different subflows (or just a counter).

I don't know well the internals of BPF so maybe what I'm suggesting is
not possible but feel free to tell me what is not clear and what cannot
work.

BTW, you can remove "struct bpf_mptcp_sock" from
include/uapi/linux/bpf.h. It is no longer needed with the modifications
you did. The doc around bpf_mptcp_sock() also needs to be updated as the
returned pointer is for a different structure: "mptcp_sock" instead of
"bpf_mptcp_sock".

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net