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

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

Thanks for the review.

On Tue, 2024-08-20 at 11:48 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for looking at that!
> 
> On 20/08/2024 10:44, 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_rdonly_cast to cast a pointer to tcp_sock for readonly. It will
> > allow
> 
> Do you mean bpf_core_cast() instead of bpf_rdonly_cast()? Or is it
> indirectly used?

Yes, it should be "bpf_core_cast". Updated in v2.

> 
> > 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>
> > ---
> >  .../testing/selftests/bpf/prog_tests/mptcp.c  | 11 +++++++
> >  tools/testing/selftests/bpf/progs/mptcp_bpf.h | 26 +++++++++++++++
> >  .../selftests/bpf/progs/mptcp_subflow.c       | 32
> > +++++++++++++++++++
> >  3 files changed, 69 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > index 69fdcb28249d..2178db94f764 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > @@ -383,6 +383,7 @@ static void run_subflow(char *new)
> >  {
> >  	int server_fd, client_fd, err;
> >  	char cc[TCP_CA_NAME_MAX];
> > +	unsigned int mark;
> >  	socklen_t len;
> >  
> >  	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1,
> > 0);
> > @@ -407,6 +408,10 @@ static void run_subflow(char *new)
> >  	ASSERT_OK(ss_search(ADDR_1, new), "ss_search new cc");
> >  	ASSERT_OK(ss_search(ADDR_2, cc), "ss_search default cc");
> 
> I guess the next steps is to remove these 'ss_search()', right?

We can keep it for double checks.

> 
> > +	len = sizeof(mark);
> > +	err = getsockopt(client_fd, SOL_SOCKET, SO_MARK, &mark,
> > &len);
> > +	ASSERT_OK(err, "getsockopt(client_fd, SO_MARK)");
> > +
> >  	close(client_fd);
> >  fail:
> >  	close(server_fd);
> > @@ -417,6 +422,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"))
> > @@ -442,6 +448,10 @@ static void test_subflow(void)
> >  	if (endpoint_init("subflow") < 0)
> >  		goto close_netns;
> >  
> > +	link = bpf_program__attach_cgroup(skel->progs._getsockopt,
> > cgroup_fd);
> > +	if (!ASSERT_OK_PTR(link, "getsockopt prog"))
> 
> I don't know how it works: is this going to fail if the getsockopt
> program set 'ctx->retval = -1'?

Yes. ASSERT_OK(err, "getsockopt(client_fd, SO_MARK)") will fail if the
getsockopt program set 'ctx->retval = -1'.
> 
> > +		goto close_netns;
> > +
> >  	run_subflow(skel->data->cc);
> >  
> >  close_netns:
> > @@ -450,6 +460,7 @@ static void test_subflow(void)
> >  	mptcp_subflow__destroy(skel);
> >  close_cgroup:
> >  	close(cgroup_fd);
> > +	bpf_link__destroy(link);
> >  }
> >  
> >  static struct nstoken *sched_init(char *flags, char *sched)
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > index 782f36ed027e..2086170e7379 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> > @@ -7,6 +7,32 @@
> >  
> >  #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);	\
> > +	     !list_entry_is_head(pos, head,
> > member);			\
> > +	     pos = list_next_entry(pos, member))
> 
> Out of curiosity, are these generic helpers not defined elsewhere? Is
> it
> OK not to use the '_safe' version where the deletion of elements is
> supported?

It's read only in this test, so list_for_each_entry is enough. I added
the '_safe' version in v2 too for future use.

> 
> > +
> > +#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 bc572e1d6df8..c41418ab8db4 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,34 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
> >  
> >  	return 1;
> >  }
> > +
> > +SEC("cgroup/getsockopt")
> > +int _getsockopt(struct bpf_sockopt *ctx)
> 
> This name seems too generic while what is done here is specific to
> the
> 'subflow' test. Maybe: _check_getsockopt_subflows()?

Changed it to _getsockopt_subflow in v2.

> 
> (or _check_getsockopt_subflows_mark() and
> _check_getsockopt_subflow_cc()
> see below)
> 
> > +{
> > +	struct mptcp_sock *msk = bpf_core_cast(ctx->sk, struct
> > mptcp_sock);
> 
> What happens if the 'sk' is not an 'msk'?
> Does it check that the sk is indeed an MPTCP one? (how?)

Also check msk->token in v2.

> 
> > +	struct mptcp_subflow_context *subflow;
> > +	int i = 0;
> > +
> > +	if (!msk || ctx->level != SOL_SOCKET || ctx->optname !=
> > SO_MARK)
> > +		return 1;
> 
> Would it not be clearer to split the two checks? One dedicated to the
> mark, when getsockopt(SO_MARK) is used, and one for the CC, when the
> getsockopt(TCP_CONGESTION) is used? By doing that, we would know
> which
> one had an issue, if any, not just "something wrong with
> getsockopt()"?

Done in v2.

> 
> > +
> > +	mptcp_for_each_subflow(msk, subflow) {
> 
> (might be better to use this helper in our WIP MPTCP BPF packets
> scheduler examples, instead of converting them to a fixed array)

Yes, but there are still some access permission issues that need to be
resolved.

> 
> > +		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 != i + 1)
> > +			ctx->retval = -1;
> > +		if (ssk->sk_mark == 1 &&
> > +		    __builtin_memcmp(icsk->icsk_ca_ops->name, cc,
> > TCP_CA_NAME_MAX))
> > +			ctx->retval = -1;
> > +
> > +		if (i++ >= MPTCP_SUBFLOWS_MAX)
> > +			break;
> 
> I guess this part is needed for the verifier, right?
> Somehow related to "cond_break" explained in this link?
> 
>   https://lwn.net/Articles/964381/
> 

Change this as "cond_break" in v2.

Regards,
-Geliang

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