Re: [PATCH mptcp-next v6 4/4] selftests: bpf: add skc_to_mptcp_sock test

[Geliang Tang <geliang.tang@suse.com>]

Hi Matt,

On Mon, Mar 07, 2022 at 03:15:46PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/03/2022 02:01, Geliang Tang wrote:
> > This patch extended the MPTCP test base, to exercise bpf_skc_to_mptcp_sock()
> > from C test as Alexei suggested in v3.
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  tools/testing/selftests/bpf/bpf_tcp_helpers.h |  6 +++++
> >  .../testing/selftests/bpf/prog_tests/mptcp.c  | 27 +++++++++++++++----
> >  tools/testing/selftests/bpf/progs/mptcp.c     | 23 ++++++++++++++++
> >  3 files changed, 51 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> > index b1ede6f0b821..05f62f81cc4d 100644
> > --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> > +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> > @@ -83,6 +83,12 @@ struct tcp_sock {
> >  	__u64	tcp_mstamp;	/* most recent packet received/sent */
> >  } __attribute__((preserve_access_index));
> >  
> > +struct mptcp_sock {
> > +	struct inet_connection_sock	sk;
> > +
> > +	__u32	token;
> > +} __attribute__((preserve_access_index));
> 
> Is it really OK to do that when 'struct mptcp_sock' is not declared in
> the 'include' directory?
> I guess the compiler can find where 'struct mptcp_sock' is actually
> defined but I'm surprised it doesn't need more assistance here.

Without this struct mptcp_sock declaration, we'll get the following build
errors:

progs/mptcp.c:64:23: error: progs/mptcp.cincomplete definition of type 'struct mptcp_sock':
64:23: error: incomplete definition of type 'struct mptcp_sock'
                storage->token = msk->token;
                                 ~~~^
                storage->token = msk->token;
                                 ~~~^
/home/tgl/mptcp_net-next/tools/testing/selftests/bpf/tools/include/bpf/bpf_helper_defs.h:32:8: note: forward declaration of 'struct mptcp_sock'
/home/tgl/mptcp_net-next/tools/testing/selftests/bpf/tools/include/bpf/bpf_helper_defs.h:struct mptcp_sock;32
:8       ^:
 note: forward declaration of 'struct mptcp_sock'
struct mptcp_sock;
       ^
11 error error generated generated.
.
make: *** [Makefile:487: /home/tgl/mptcp_net-next/tools/testing/selftests/bpf/mptcp.o] Error 1
make: *** Waiting for unfinished jobs....
make: *** [Makefile:492: /home/tgl/mptcp_net-next/tools/testing/selftests/bpf/no_alu32/mptcp.o] Error 1

> 
> In your tests, did you check that the token seen in the kernel --
> outside the BPF part but in MPTCP code -- is the same as what the
> userspace program can see after a capture from BPF side? I mean: the
> current test only check the token is different from 0 but it could also
> be set at a wrong value (any random value except 0) and we would not
> notice the issue, no?

Yes, in my tests, the token in the userspace (both bpf and ip mptcp monitor)
is the same as it printed in the kernel:

> sudo dmesg | grep mptcp_sock
[ 3057.842339] bpf_mptcp_sock_from_subflow sk=000000004370e862 sk_fullsock=1 sk_protocol=1 sk_is_mptcp=1
[ 3057.842341] bpf_mptcp_sock_from_subflow subflow->conn=00000000f16c62fd msk=00000000f16c62fd token=3420716048

# cat /sys/kernel/debug/tracing/trace_pipe
      test_progs-22898   [007] d..21  3057.858031: bpf_trace_printk: is_mptcp=0 token=0(0x0)
      test_progs-22898   [007] d..21  3057.872544: bpf_trace_printk: is_mptcp=1 token=3420716048(0xcbe3fc10)

> sudo ip mptcp monitor
[       CREATED] token=cbe3fc10 remid=0 locid=0 saddr4=127.0.0.1 daddr4=127.0.0.1 sport=54646 dport=60123
[       CREATED] token=68f336ad remid=0 locid=0 saddr4=127.0.0.1 daddr4=127.0.0.1 sport=60123 dport=54646
[        CLOSED] token=cbe3fc10

> 
> > +
> >  static __always_inline struct inet_connection_sock *inet_csk(const struct sock *sk)
> >  {
> >  	return (struct inet_connection_sock *)sk;
> > diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > index 04aef0f147dc..ba856956f9c3 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > @@ -6,9 +6,11 @@
> >  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)
> > +static int verify_sk(int map_fd, int client_fd, const char *msg,
> > +		     __u32 is_mptcp, __u32 token)
> >  {
> >  	int err = 0, cfd = client_fd;
> >  	struct mptcp_storage val;
> > @@ -19,8 +21,23 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
> >  	 * does not trigger sockops events.
> >  	 * We silently pass this situation at the moment.
> >  	 */
> > -	if (is_mptcp == 1)
> > -		return 0;
> > +	if (is_mptcp == 1) {
> > +		if (token <= 0)
> 
> It looks like you no longer need 'token': it always has the same value
> as "is_mptcp", which makes sense: if it is an MPTCP connection, it
> should have a token. So maybe good not to add this new 'token' variable,
> WDYT?
> 
> (it can be added later if it is needed to manage different cases)

agree.

> 
> > +			return 0;
> > +
> > +		if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
> > +			perror("Failed to read socket storage");
> > +			return -1;
> > +		}
> > +
> > +		if (val.token <= 0) {
> > +			log_err("%s: unexpected bpf_mptcp_sock.token %d %d",
> > +				msg, val.token, token);
> > +			err++;
> > +		}
> > +
> > +		return err;
> > +	}
> >  
> >  	if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
> >  		perror("Failed to read socket storage");
> > @@ -76,8 +93,8 @@ static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
> >  		goto close_client_fd;
> >  	}
> >  
> > -	err += is_mptcp ? verify_sk(map_fd, client_fd, "MPTCP subflow socket", 1) :
> > -			  verify_sk(map_fd, client_fd, "plain TCP socket", 0);
> > +	err += is_mptcp ? verify_sk(map_fd, client_fd, "MPTCP subflow socket", 1, 1) :
> > +			  verify_sk(map_fd, client_fd, "plain TCP socket", 0, 0);
> >  
> >  close_client_fd:
> >  	close(client_fd);
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp.c b/tools/testing/selftests/bpf/progs/mptcp.c
> > index be5ee8dac2b3..212e9341b877 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp.c
> > +++ b/tools/testing/selftests/bpf/progs/mptcp.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <linux/bpf.h>
> >  #include <bpf/bpf_helpers.h>
> > +#include "bpf_tcp_helpers.h"
> >  
> >  char _license[] SEC("license") = "GPL";
> >  __u32 _version SEC("version") = 1;
> > @@ -8,6 +9,7 @@ __u32 _version SEC("version") = 1;
> >  struct mptcp_storage {
> >  	__u32 invoked;
> >  	__u32 is_mptcp;
> > +	__u32 token;
> >  };
> >  
> >  struct {
> > @@ -20,6 +22,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 +46,26 @@ 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 mptcp_sock *msk;
> > +
> > +		msk = bpf_skc_to_mptcp_sock(sk);
> > +		if (!msk)
> > +			return 1;
> 
> (detail) if you have to edit this commit, please add a new empty line
> here after the 'return'.

Agree.

> 
> > +		storage = bpf_sk_storage_get(&socket_storage_map, msk, 0,
> > +					     BPF_SK_STORAGE_GET_F_CREATE);
> > +		if (!storage)
> > +			return 1;
> > +
> > +		storage->invoked++;
> > +		storage->token = msk->token;
> 
> Linked to my previous comment about checking if 'token' had the right
> value: it is not enough to compare this 'msk->token' printed with
> 'bpf_trace_printk' here below with the value you have in
> 'prog_tests/mptcp.c' because it is supposed to be the same one if the
> BPF MAP storage does its job (I guess it does).
> 
> Instead, we should compare with either what you can see with 'ss' or 'ip
> mptcp monitor' or with the code in 'net/mptcp/' dir (pr_debug's print it
> at some points I guess). But doing that is not enough: that will not be
> part of the automated tests, just a manual check. But still good to do :)
> 
> My point is that the token is supposed to be random: if you read the
> wrong address in the memory, it will also appear as a random value
> because it is unlikely to read '0'.

I did these manual checks. The all token values are the same in both
user space and kernel space. I'll try to add the automated check in the
next version, using netlink (like 'ip mptcp monitor') to get the token
values from kernel space, and compare it in bpf.

> 
> Maybe it would be good to also check other values from 'mptcp_sock'. But
> I don't know which one can be known in advanced in this structure appart
> from booleans: fully_established, cork, etc. But I guess we should avoid
> using booleans: one bit, we could be lucky to have the good value.
> 
> Or maybe interesting to use 'struct sock *first' (or *last_snd or
> *subflow)? I mean it would be a good test case to keep a ref to a
> subflow. Then from the userspace, we could get info from the MSK and
> then get info about the subflow.
> But I don't know if it is possible: typically from the usespace, we get
> a sk from a fd. Can we get a sk from its pointer?
> I guess it will be needed to iterate over the different subflows from a msk.

I'll try to check other more 'mptcp_sock' members later.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> 
> 
> > +		storage->is_mptcp = 1;
> > +	}
> > +
> > +	bpf_trace_printk(fmt, sizeof(fmt),
> > +			 storage->invoked, storage->is_mptcp, storage->token);
> >  
> >  	return 1;
> >  }
> 
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>