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

[Martin KaFai Lau <martin.lau@linux.dev>]

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.

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