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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

Thank you for your reply!

On 04/09/2024 11:55, Geliang Tang wrote:
> On Tue, 2024-09-03 at 11:16 +0200, Matthieu Baerts wrote:
>> On 03/09/2024 11:11, Matthieu Baerts wrote:
>>> 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 :)
>>
>> While at it: it sounds better to have unique error messages, to know
>> where was the error. Here, a few messages are the same, e.g.
>> protocol,
>> number of subflows, sk is null. Do not hesitate to add something to
>> differentiate them: level & optname, a string prefix, the line
>> number, etc.
> 
> No, I think we should drop all these "bpf_printk" messages. I checked
> all other BPF selftests programs, no one use these error messages to
> debug like this. Just "return 1" is enough:
> 
>         if (!sk || sk->protocol != IPPROTO_MPTCP)
>                 return 1;
> 
> If a BPF program fail, its own load log will show:
> 
> libbpf: prog '_getsockopt_subflow': -- BEGIN PROG LOAD LOG --
> 0: R1=ctx() R10=fp0
> ; int _getsockopt_subflow(struct bpf_sockopt *ctx) @
> mptcp_subflow.c:136
> 0: (bf) r9 = r1                       ; R1=ctx() R9_w=ctx()
> ; struct bpf_sock *sk = ctx->sk; @ mptcp_subflow.c:138
> 1: (79) r7 = *(u64 *)(r9 +0)          ; R7_w=sock() R9_w=ctx()
> ; if (bpf_get_current_pid_tgid() >> 32 != pid) @ mptcp_subflow.c:141
> 2: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()
> 
> No need to add any line number by ourselves.

Good, that looks OK.

Just to be sure it is readable: here the BPF program failed because the
PID was not the expected one, right? Or is it another error?

Do you mind sharing the output if the CC is not "foo" for example please?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.