Re: [PATCH bpf-next/net v3 4/5] selftests/bpf: Add mptcp_subflow bpf_iter subtest

[Matthieu Baerts <matttbe@kernel.org>]

Hi Martin,

On 21/05/2025 00:18, Martin KaFai Lau wrote:
> On 5/19/25 3:04 AM, Matthieu Baerts wrote:
>>>> +SEC("cgroup/getsockopt")
>>>> +int iters_subflow(struct bpf_sockopt *ctx)
>>>> +{
>>>> +    struct mptcp_subflow_context *subflow;
>>>> +    struct bpf_sock *sk = ctx->sk;
>>>> +    struct sock *ssk = NULL;
>>>> +    struct mptcp_sock *msk;
>>>> +    int local_ids = 0;
>>>> +
>>>> +    if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
>>>> +        return 1;
>>>> +
>>>> +    msk = bpf_core_cast(sk, struct mptcp_sock);
>>>> +    if (!msk || msk->pm.server_side || !msk->pm.subflows)
>>>> +        return 1;
>>>> +
>>>> +    bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) {
>>>> +        /* Here MPTCP-specific packet scheduler kfunc can be called:
>>>> +         * this test is not doing anything really useful, only to
>>>
>>> Lets fold the bpf_iter_mptcp_subflow addition into the future
>>> "mptcp_sched_ops" set (the github link that you mentioned in patch 2).
>>> Post them as one set to have a more practical example.
>>
>> Thank you for this suggestion. We can delay that if needed.
>>
>> Note that we have two struct_ops in preparation: mptcp_sched_ops and
>> mptcp_pm_ops. We don't know which one will be ready first. They are both
>> "blocked" by internal API modifications we would like to do to ease the
>> maintenance later before "exposing" such API's via BPF. That's why we
>> suggested to upstream this common part first as it is ready. But we can
>> of course wait if you prefer.
> 
> This set is useful for discussing the questions you raised in patch 2.
> 
> I still don't see it useful to upstream patch 2 alone. The existing
> selftests/bpf/progs/mptcp_subflow.c has already shown a way to do
> similar iteration in SEC("cgroup/getsockopt") without patch 2.
> 
> I would prefer to wait for a fuller picture on the main struct_ops use
> case first to ensure that we didn't overlook things. iiuc, improving the
> iteration in SEC("cgroup/getsockopt") is not the main objective.

I understand, that makes sense. When the rest will be ready, we will
upstream patches from this series, except this one ("useless" selftest),
and restricting bpf_iter_mptcp_subflow_* and other new kfuncs to
BPF_PROG_TYPE_STRUCT_OPS only. So not to BPF_PROG_TYPE_CGROUP_SOCKOPT
any more which was only needed for this new test. I don't think this
program type requires access to these new kfunc for useful use-cases.
This can be changed later if required anyway.

>>>> +         * verify the iteration works.
>>>> +         */
>>>> +
>>>> +        local_ids += subflow->subflow_id;
>>>> +
>>>> +        /* only to check the following helper works */
>>>> +        ssk = mptcp_subflow_tcp_sock(subflow);
>>>> +    }
>>>> +
>>>> +    if (!ssk)
>>>> +        goto out;
>>>> +
>>>> +    /* assert: if not OK, something wrong on the kernel side */
>>>> +    if (ssk->sk_dport != ((struct sock *)msk)->sk_dport)
>>>> +        goto out;
>>>> +
>>>> +    /* only to check the following kfunc works */
>>>> +    subflow = bpf_mptcp_subflow_ctx(ssk);
>>>
>>> bpf_core_cast should be as good instead of adding a new
>>> bpf_mptcp_subflow_ctx() kfunc, so patch 1 should not be needed.
>>
>> OK, indeed, in this series we don't need it. We will need it later to
>> modify some fields from the "subflow" structure directly. We can do the
> 
> The "ssk" here is not a trusted pointer. Note that in patch 1, the kfunc
> bpf_mptcp_subflow_ctx() does not specify KF_TRUSTED_ARGS. I suspect it
> should be KF_TRUSTED_ARGS based on what you described here.

Good point, I think this flag is indeed missing.

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