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

[Mat Martineau <martineau@kernel.org>]

On Tue, 4 Mar 2025, Matthieu Baerts wrote:

> Hi Mat,
>
> On 04/03/2025 02:42, Mat Martineau wrote:
>> 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.
>
> Thank you for your reply! But I admit I'm a bit lost: which changes are
> you talking about?
>
> In the "Add mptcp_subflow bpf_iter support" series we sent to the BPF
> maintainers, this support is only added to CG [gs]etsockopt where the sk
> lock is owned, so no need to have extra checks for the moment, no? Then
> the "Squash to "Add mptcp_subflow bpf_iter support"" v4 (not the last
> one you looked at, the v5) should be enough, no?
>

Hi Matthieu -

Thanks for clarifying, I misunderstood the scope of "all these squash-to 
patches" to include v5 :)

I see what you mean, and I do agree that squashing the v4 patches 
(https://patchwork.kernel.org/project/mptcp/cover/cover.1740368110.git.tanggeliang@kylinos.cn/) 
for another round of BPF maintainer review is ok with me since the sockopt 
calls have the necessary locking, so we can separately upstream the 
iterator series (with sockopt limitations) now.

> My understanding is that setting "msk->bpf_XXX" to "current" will only
> be needed when mptcp_subflow bpf_iter will be allowed to be used with
> struct_ops, no? If yes, better to keep this complexity for later, no?

Agree here as well, the task_struct checking is only needed once there are 
other locking situations to verify.


- Mat