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

[Geliang Tang <geliang@kernel.org>]

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