Re: [PATCH mptcp-next] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Mat Martineau <martineau@kernel.org>]

On Sat, 1 Mar 2025, Matthieu Baerts wrote:

> Hi Mat,
>
> On 01/03/2025 02:37, Mat Martineau wrote:
>> On Thu, 27 Feb 2025, Matthieu Baerts wrote:
>>
>>> Hi Geliang, Mat,
>>>
>>> On 27/02/2025 03:03, Geliang Tang wrote:
>>>> Hi Matt,
>>>>
>>>> On Wed, 2025-02-26 at 19:07 +0100, Matthieu Baerts (NGI0) wrote:
>>>>> From what we understood, when being used from a CG [gs]etsockopt
>>>>> hook,
>>>>> the socket lock will be held. It seems that the BPF infra will make
>>>>> sure
>>>>> in this case. Mat will try to get the confirmation.
>>>>>
>>>>> The idea is then to add this msk_owned_by_me() check to make sure the
>>>>> assumption is correct, and will continue to be: the new selftest
>>>>> should
>>>>> complain if not when used with a debug kconfig. In other words, if
>>>>> the
>>>>> tests continue to pass with this patch, the Squash-to series can
>>>>> probably be applied.
>>>>>
>>>>> Note that this is just a debug check, hoping that the selftest will
>>>>> cover all cases. We can then not use this check to return an error if
>>>>> it
>>>>> is not held.
>>>>>
>>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>>
>>>> Thanks for updating this for me.
>>>>
>>>> LGTM!
>>>>
>>>> Reviewed-by: Geliang Tang <geliang@kernel.org>
>>>
>>> Thank you for the review!
>>>
>>>>> ---
>>>>> Based-on: <cover.1740368110.git.tanggeliang@kylinos.cn>
>>>>
>>>> I changed the state of this set, Squash to "Add mptcp_subflow bpf_iter
>>>> support" v4, together with this squash-to patch, as "Queued".
>>>
>>> Thank you. If that's OK, I will wait for Mat's green light before
>>> applying the patches, because he wanted to look at the BPF code to make
>>> sure our assumption was correct.
>>>
>>
>> Ok, I did miss it before but in the cases of setsockopt/getsockopt, the
>> socket lock is acquired by the bpf wrapper code:
>>
>> https://elixir.bootlin.com/linux/v6.14-rc4/source/kernel/bpf/cgroup.c#L1955
>>
>> That's why the test passes. There's still the general issue that the
>> iterator could be used in any bpf code that can pass a valid msk
>> pointer. The existing lock checks in the export branch check that the
>> socket is locked, but it could be locked by some other owner.
>
> Thank you for having looked! Because these new kfuncs are currently only
> tied to BPF_PROG_TYPE_CGROUP_SOCKOPT, can we apply all these squash-to
> patches and send this to BPF maintainers?

Hi Matthieu -

I think once the squash-to patches are accepted into our repo (a few more 
changes are needed), it would be ok to upstream them to the BPF 
maintainers.

- Mat


>
>
> When bpf_iter will be used in the schedulers and the PM, more checks can
> be added later.
>
>> As we've already discussed, msk_owned_by_me() doesn't work everywhere
>> because we can't change behavior if the lock isn't owned. However, we
>> don't need to handle every locking case like lockdep does - for the
>> scheduler BPF case, we only care if the BPF code is running in the
>> scheduler context. So, I think we can do a simplified version of
>> tracking the lock context:
>>
>> 1. Add a 'struct task_struct *bpf_scheduler_task' field to struct
>> mptcp_sock.
>>
>> 2. Do a WRITE_ONCE(msk->bpf_scheduler_context, current) before calling a
>> packet scheduler hook, and WRITE_ONCE(msk->bpf_scheduler_task, NULL)
>> after the hook returns.
>>
>> 3. In bpf_iter_mptcp_subflow_new(), check "READ_ONCE(msk->
>> bpf_scheduler_task) == current" to confirm the correct task, return
>> -EINVAL if it doesn't match.
>
> That's a very good idea! Yes, we will need to add such checks before
> msk_owned_by_me() when the bpf_iter will be used elsewhere.
>
>> (It would help to create helper functions for setting and checking that
>> value)
>
> +1.
>
>> Do you think this iterator will be useful in other places, like bpf path
>> manager? The same approach would work there, but a better name than
>> "bpf_scheduler_task" would help.
> Yes, good point! "msk->bpf_struct_ops_task"?
>
> (that's something that could also be added after having applied "use
> bpf_iter in bpf schedulers") series)
>
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.
>
>