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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 26/08/2024 04:57, Geliang Tang wrote:
> On Wed, 2024-08-21 at 11:37 +0200, Matthieu Baerts wrote:
>> 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:

(...)

>>>>> +	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);

(Why do you need to cast "tmp" (struct mptcp_subflow_context *) in the
same type of pointer? I don't think it changes anything for the error
you got (see below), but it looks strange.)

>                 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.

I'm not an expert in this, but I guess it means you cannot use
'mptcp_subflow_active()', because it can modify the 'subflow' structure
that you got with bpf_core_cast() for a read-only usage.

I guess it means we cannot iterate through the list and modify items
from the list "directly" with BPF. Except if there is something else we
can use, and I don't know about (which is very likely), we might have to
continue extracting the subflows into an array of a fixed size.

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