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

[Geliang Tang <geliang@kernel.org>]

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

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