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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang, Martin,

On 26/08/2024 11:24, Geliang Tang wrote:
> Hi Matt, Martin,
> 
> On Mon, 2024-08-26 at 10:44 +0200, Matthieu Baerts wrote:
>> 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.)
> 
> We must do the cast, otherwise, an "access beyond struct list_head"
> occurs:
> 
> ; token = subflow->token; @ mptcp_subflow.c:92
> 13: (61) r4 = *(u32 *)(r1 +524)
> access beyond struct list_head at off 524 size 4

OK!

> 
> See Martin's comment in [1].
> 
> [1]
> https://patchwork.kernel.org/project/netdevbpf/patch/20240805-upstream-bpf-next-20240506-mptcp-subflow-test-v4-2-2b4ca6994993@kernel.org/
> 
>>
>>>                 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.
> 
> A read-only function will get the same error.
> 
> I added a read-only function mptcp_subflow_get_scheduled() for testing:
> 
> bool mptcp_subflow_get_scheduled(struct mptcp_subflow_context *subflow)

Do you have the same issue if '*subflow' is marked as 'const'?

> {
>         return subflow->scheduled;
> }
> 
> And invoke it from BPF in mptcp_for_each_subflow() loop:
> 
> 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);
>                 mptcp_subflow_get_scheduled(subflow);
>         }
>         return 0;
> }
> 
> The same "arg#0 is untrusted_ptr_ expected ptr_ or socket" occurs.
> 
> Hope Martin can give us a solution for this issue.
> 
> Thanks,
> -Geliang
> 
>>
>> 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
> 

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