Re: [PATCH mptcp-next v3 3/5] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 24/02/2025 09:31, Geliang Tang wrote:
> Hi Mat,
> 
> Thanks for the review.
> 
> On Thu, 2025-02-20 at 19:25 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 17/02/2025 11:34, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> Drop the NULL check for 'msk' as Martin suggested, add more checks
>>> for 'sk'.
>>>
>>> Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as
>>> the
>>> argument in the bpf_iter_mptcp_subflow_new as Martin suggested.
>>>
>>> v3:
>>>  - continue to use sock_owned_by_user_nocheck and spin_is_locked
>>> checks instead of using msk_owned_by_me().
>>
>> Why should we continue to do so?
>>
>> From what I understood, with the current version, this new
>> bpf_iter_mptcp_subflow_new kfunc can only be called from a cg sockopt
>> bpf prog, which means these checks are not needed for the moment, no?
> 
> Thanks for the clarification. I re-read Martin's comments and he also
> suggested removing these checks. So I removed it in v4.

Thanks!

>> So maybe we should only add these checks later, when extending its
>> usage
>> to struct_ops? In the meantime, msk_owned_by_me() could then be used
>> instead?
> 
> Do I need to add a patch into the "use bpf_iter in bpf schedulers" set
> that is under review to do these checks? Do you mean that using
> msk_owned_by_me() is enough for struct_ops, and there is no need to use
> sock_owned_by_user_nocheck() and spin_is_locked() checks?

My understanding is that if a kfunc is set to used in 'struct_ops', it
can be used with any 'struct_ops', and not a specific one. Is this correct?

- If yes, it means we don't really know in which context a kfunc will be
used, thus we need additional checks. But... this feels wrong: if
kfunc's cannot be restricted to specific 'struct_ops', probably this
should be reported to the BPF maintainers, and see if it could be
possible to have such restriction not to add unneeded checks at run time.

- If the MPTCP kfunc can be restricted to MPTCP 'struct_ops' where we
know the msk lock will be taken, then no need to add these checks if
they are useless.

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