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

[Geliang Tang <geliang@kernel.org>]

On Mon, 2024-08-26 at 11:49 +0200, Matthieu Baerts wrote:
> 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'?

Yes, the same error occurs:

'''
libbpf: prog 'bpf_first_get_subflow': -- BEGIN PROG LOAD LOG --
0: R1=ctx() R10=fp0
; int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk, @
mptcp_bpf_first.c:22
0: (b7) r7 = 0                        ; R7_w=0
1: (b7) r2 = 2488                     ; R2_w=2488
2: (79) r8 = *(u64 *)(r1 +0)
func 'get_subflow' arg0 has btf_id 27737 type STRUCT 'mptcp_sock'
3: R1=ctx() R8_w=trusted_ptr_mptcp_sock()
3: (bf) r6 = r8                       ; R6_w=trusted_ptr_mptcp_sock()
R8_w=trusted_ptr_mptcp_sock()
4: (0f) r6 += r2                      ; R2_w=2488
R6_w=trusted_ptr_mptcp_sock(off=2488)
5: (b7) r1 = 2488                     ; R1_w=2488
6: (0f) r8 += r1                      ; R1_w=2488
R8_w=trusted_ptr_mptcp_sock(off=2488)
7: (b7) r9 = 0                        ; R9_w=0
8: (05) goto pc+12
; 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 = 0x6c55                  ; R2_w=27733
17: (85) call bpf_rdonly_cast#122463          ;
R0_w=untrusted_ptr_mptcp_subflow_context()
; mptcp_subflow_get_scheduled(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_get_scheduled#127716
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 --
'''

net/mptcp/sched.c:

'''
bool mptcp_subflow_get_scheduled(const struct mptcp_subflow_context
*subflow)
{
       return subflow->scheduled;
}
'''

progs/mptcp_bpf_first.c:

'''
extern bool mptcp_subflow_get_scheduled(const 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);
                mptcp_subflow_get_scheduled(subflow);
        }
        return 0;
}
'''

Thanks,
-Geliang

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