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

[Geliang Tang <geliang@kernel.org>]

Hi Martin, Matt,

On Wed, 2024-08-21 at 11:37 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for your reply!
> 
> On 21/08/2024 10:00, Geliang Tang wrote:
> > On Tue, 2024-08-20 at 11:48 +0200, Matthieu Baerts wrote:
> > > On 20/08/2024 10:44, Geliang Tang wrote:
> 
> (...)
> 
> > > > 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.
> 
> I would not mind, even if 'ss | grep' is used in other BPF tests, but
> I
> understood from Martin he prefers not to depend on external tools as
> much as possible. It should indeed ease the maintenance.
> 
> > > > +	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'.
> 
> Good, thank you!
> 
> > > > 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.
> 
> OK! (but I don't think you defined list_for_each_entry_safe(). Or is
> it
> defined elsewhere?)
> 
> > > > +
> > > > +#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.
> 
> Mmh, I'm not sure if this is enough: the offset of 'token' in the msk
> structure could point to something unrelated but non-null in another
> socket structure, or out of bound. I guess BPF is doing extra checks
> not
> to crash here. But maybe that's OK because it is only reading stuff?
> 
> > > > +	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.
> 
> Thanks!
> 
> > > > +
> > > > +	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.
> 
> OK, because structures cannot be modified I suppose.

No, it seems the subflow cast by bpf_core_cast() can't be passed to a
kernel function, regardless of whether this function modifies the
subflow or not. An "arg#0 is untrusted_ptr_ expected ptr_" error
occurs.

For example, mptcp_subflow_active() is a kernel function, and pass the
subflow to it in progs/mptcp_bpf_first.c like this:

'''
... ...
extern bool mptcp_subflow_active(struct mptcp_subflow_context *subflow)
__ksym;

... ...
SEC("struct_ops")
int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk,
             struct mptcp_sched_data *data)
{
        struct mptcp_subflow_context *subflow, *tmp;

        mptcp_for_each_subflow(msk, tmp) {
                subflow = bpf_core_cast(tmp, struct
mptcp_subflow_context);
                if (!mptcp_subflow_active(subflow))
                        continue;
        }
        return 0;
}

'''

An "arg#0 is untrusted_ptr_ expected ptr_" error occurs:

'''
; mptcp_for_each_subflow(msk, tmp) { @ mptcp_bpf_first.c:27
21: (e5) may_goto pc+1
22: R1=2488 R2=2488 R6=trusted_ptr_mptcp_sock(off=2488) R7=0
R8=trusted_ptr_mptcp_sock(off=2488) R9=0 R10=fp0
; mptcp_for_each_subflow(msk, tmp) { @ mptcp_bpf_first.c:27
22: (05) goto pc-14
9: (79) r6 = *(u64 *)(r6 +0)          ; R6_w=ptr_list_head()
10: (1f) r6 -= r7                     ; R6_w=ptr_list_head() R7=0
11: (bf) r1 = r6                      ; R1_w=ptr_list_head()
R6_w=ptr_list_head()
12: (0f) r1 += r7                     ; R1=ptr_list_head() R7=0
13: (1d) if r1 == r8 goto pc+9        ; R1=ptr_list_head()
R8=trusted_ptr_mptcp_sock(off=2488)
; subflow = bpf_core_cast(tmp, struct mptcp_subflow_context); @
mptcp_bpf_first.c:28
14: (bf) r1 = r6                      ; R1_w=ptr_list_head()
R6=ptr_list_head()
15: (18) r2 = 0x6d14                  ; R2_w=27924
17: (85) call bpf_rdonly_cast#159867          ;
R0_w=untrusted_ptr_mptcp_subflow_context()
; if (!mptcp_subflow_active(subflow)) @ mptcp_bpf_first.c:29
18: (bf) r1 = r0                      ;
R0_w=untrusted_ptr_mptcp_subflow_context()
R1_w=untrusted_ptr_mptcp_subflow_context()
19: (85) call mptcp_subflow_active#111397
arg#0 is untrusted_ptr_ expected ptr_ or socket
processed 23 insns (limit 1000000) max_states_per_insn 0 total_states 2
peak_states 2 mark_read 2
-- END PROG LOAD LOG --
'''

How can I fix this? I need your advice.

Thanks,
-Geliang

> 
> > > 
> > > > +		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.
> 
> Note that you could also modify list_for_each_entry() to add this
> 'cond_break' (+bpf_experimental.h?), not to have to deal with that
> each
> time mptcp_for_each_subflow is used.
> 
> Cheers,
> Matt