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

[Geliang Tang <geliang@kernel.org>]

Hi Mat,

Thanks for the review.

On Mon, 2025-03-03 at 17:37 -0800, Mat Martineau wrote:
> On Mon, 3 Mar 2025, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Set msk->bpf_iter_task in bpf_mptcp_sock_from_sock() to allow
> > mptcp_subflow bpt_iter can be used in cgroup/getsockopt,
> > otherwise, the selftest in this set fails.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > net/mptcp/bpf.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> > index be222fa5f308..aff92f458e7f 100644
> > --- a/net/mptcp/bpf.c
> > +++ b/net/mptcp/bpf.c
> > @@ -197,14 +197,22 @@ static struct bpf_struct_ops
> > bpf_mptcp_sched_ops = {
> > 
> > struct mptcp_sock *bpf_mptcp_sock_from_sock(struct sock *sk)
> > {
> > +	struct mptcp_sock *msk;
> > +
> > 	if (unlikely(!sk || !sk_fullsock(sk)))
> > 		return NULL;
> > 
> > -	if (sk->sk_protocol == IPPROTO_MPTCP)
> > -		return mptcp_sk(sk);
> > +	if (sk->sk_protocol == IPPROTO_MPTCP) {
> > +		msk = mptcp_sk(sk);
> > +		mptcp_set_bpf_iter_task(msk);
> 
> Hi Geliang -
> 
> The important part of the bpf_iter_task approach is to only set the 
> task_struct pointer when the msk lock is already held, and to clear
> it 
> before the msk lock is released. When used this way, the check
> ensures 
> that the iterator code is both *running with the socket locked* and
> *in 
> the same context where the lock is held*.

Got it, thanks for your explanation.

> 
> 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?

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?

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
> > 
> > 
> >