Re: [PATCH mptcp-next v5 2/5] Squash to "bpf: Extend bpf_skc_to_mptcp_sock to MPTCP sock"

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 07/03/2025 04:54, Geliang Tang wrote:
> On Mon, 2025-03-03 at 17:37 -0800, Mat Martineau wrote:

(...)

>> The code above will set msk->bpf_iter_task even when the lock is not
>> held, 
>> and then it is never cleared.
>>
>> To set/clear as expected for get/setsockopt I think the task pointer
>> would 
>> have to be set in places where the locks are acquired and released,
>> like 
>> these:
>>
>> https://elixir.bootlin.com/linux/v6.14-rc5/source/kernel/bpf/cgroup.c#L1955
>> https://elixir.bootlin.com/linux/v6.14-rc5/source/kernel/bpf/cgroup.c#L1846
>>
> 
> The get/setsockopt scenario is a bit complicated. I think we can use
> the bpf_iter_task check only for bpf schedulers, keep the
> get/setsockopt scenario unchanged, and continue to use the
> msk_owned_by_me(msk) check. What do you think?

I think in any case, it doesn't hurt to keep msk_owned_by_me(msk) for
lockdep.

If we think it is difficult to have the bpf_iter for both CG
[gs]etsockopt and struct_ops, then we can also drop the patches for the
[gs]etsockopt.

Or, from a kfunc, is it easy to check if we are called from a struct_ops?

> In addition, I think it's better to add bpf_iter_task to struct
> mptcp_sched_data instead in struct mptcp_sock, since it's more related
> to mptcp scheduler. For specific usage, please see [1]. What do you
> think of this?

Please see my reply to your comment on this thread. In short, we can
always add new parameters later, when we need them, no?

https://lore.kernel.org/706a3051-8328-4321-8eea-71d4120ea996@kernel.org

> 
> Thanks,
> -Geliang
> 
> [1]
> https://patchwork.kernel.org/project/mptcp/cover/cover.1739788598.git.tanggeliang@kylinos.cn/
> 
>>
>> - Mat
>>
>>> +		return msk;
>>> +	}
>>>
>>> -	if (sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
>>> -		return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
>>> +	if (sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk)) {
>>> +		msk = mptcp_sk(mptcp_subflow_ctx(sk)->conn);
>>> +		mptcp_set_bpf_iter_task(msk);
>>> +		return msk;
>>> +	}
>>>
>>> 	return NULL;
>>> }
>>> -- 
>>> 2.43.0
>>>
>>>
>>>
> 
> 

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