netdev - 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] [day] [month] [year] [list]

Message-ID: <f9b1d13c-aa20-4680-849c-535ea7c476a6@kernel.org>
Date: Fri, 23 May 2025 13:07:10 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Martin KaFai Lau <martin.lau@...ux.dev>
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

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.