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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0364f8d2-9aa5-4dc0-b7f6-1c8572932814@linux.dev>
Date: Tue, 20 May 2025 15:18:52 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Matthieu Baerts <matttbe@...nel.org>
Cc: mptcp@...ts.linux.dev, Mat Martineau <martineau@...nel.org>,
 Geliang Tang <geliang@...nel.org>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Andrii Nakryiko <andrii@...nel.org>, Eduard Zingerman <eddyz87@...il.com>,
 Song Liu <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
 Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next/net v3 4/5] selftests/bpf: Add mptcp_subflow
 bpf_iter subtest

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.