Re: [PATCH mptcp-next v3 1/2] mptcp: add bpf_iter_task for mptcp_sock

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Mon, 2025-03-17 at 11:29 +0100, Matthieu Baerts wrote:
> Hi Geliang, Mat,
> 
> On 17/03/2025 10:41, Geliang Tang wrote:
> > On Mon, 2025-03-10 at 11:30 +0800, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > To make sure the mptcp_subflow bpf_iter is running in the
> > > MPTCP context. This patch adds a simplified version of tracking
> > > for it:
> > > 
> > > 1. Add a 'struct task_struct *bpf_iter_task' field to struct
> > > mptcp_sock.
> > > 
> > > 2. Do a WRITE_ONCE(msk->bpf_iter_task, current) before calling
> > > a MPTCP BPF hook, and WRITE_ONCE(msk->bpf_iter_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.
> > > 
> > > Also creates helpers for setting, clearing and checking that
> > > value.
> > > 
> > > Suggested-by: Mat Martineau <martineau@kernel.org>
> > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > ---
> > >  net/mptcp/bpf.c      |  2 ++
> > >  net/mptcp/protocol.c |  1 +
> > >  net/mptcp/protocol.h | 20 ++++++++++++++++++++
> > >  net/mptcp/sched.c    | 15 +++++++++++----
> > >  4 files changed, 34 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> > > index c0da9ac077e4..0a78604742c7 100644
> > > --- a/net/mptcp/bpf.c
> > > +++ b/net/mptcp/bpf.c
> > > @@ -261,6 +261,8 @@ bpf_iter_mptcp_subflow_new(struct
> > > bpf_iter_mptcp_subflow *it,
> > >  		return -EINVAL;
> > >  
> > >  	msk = mptcp_sk(sk);
> > > +	if (!mptcp_check_bpf_iter_task(msk))
> > > +		return -EINVAL;
> > >  
> > >  	msk_owned_by_me(msk);
> > >  
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index 01157ad2e2dc..d98e48ce8cd8 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -2729,6 +2729,7 @@ static void __mptcp_init_sock(struct sock
> > > *sk)
> > >  	inet_csk(sk)->icsk_sync_mss = mptcp_sync_mss;
> > >  	WRITE_ONCE(msk->csum_enabled,
> > > mptcp_is_checksum_enabled(sock_net(sk)));
> > >  	WRITE_ONCE(msk->allow_infinite_fallback, true);
> > > +	mptcp_clear_bpf_iter_task(msk);
> > >  	msk->recovery = false;
> > >  	msk->subflow_id = 1;
> > >  	msk->last_data_sent = tcp_jiffies32;
> > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > > index 3492b256ecba..1c6958d64291 100644
> > > --- a/net/mptcp/protocol.h
> > > +++ b/net/mptcp/protocol.h
> > > @@ -334,6 +334,7 @@ struct mptcp_sock {
> > >  				 */
> > >  	struct mptcp_pm_data	pm;
> > >  	struct mptcp_sched_ops	*sched;
> > > +	struct task_struct *bpf_iter_task;
> > >  	struct {
> > >  		u32	space;	/* bytes copied in last
> > > measurement
> > > window */
> > >  		u32	copied; /* bytes copied in this
> > > measurement
> > > window */
> > > @@ -1291,4 +1292,23 @@ mptcp_token_join_cookie_init_state(struct
> > > mptcp_subflow_request_sock *subflow_re
> > >  static inline void mptcp_join_cookie_init(void) {}
> > >  #endif
> > >  
> > > +static inline void mptcp_set_bpf_iter_task(struct mptcp_sock
> > > *msk)
> > > +{
> > > +	WRITE_ONCE(msk->bpf_iter_task, current);
> > > +}
> > > +
> > > +static inline void mptcp_clear_bpf_iter_task(struct mptcp_sock
> > > *msk)
> > > +{
> > > +	WRITE_ONCE(msk->bpf_iter_task, NULL);
> > > +}
> > > +
> > > +static inline bool mptcp_check_bpf_iter_task(struct mptcp_sock
> > > *msk)
> > > +{
> > > +	struct task_struct *task = READ_ONCE(msk-
> > > >bpf_iter_task);
> > > +
> > > +	if (task && task == current)
> > > +		return true;
> > > +	return false;
> > > +}
> > 
> > This v3 has a bug. When I was testing MPTCP BPF selftests in a
> > loop, I
> > found that the test would break in some cases. After debugging, I
> > found
> > that "task" and "current" were not equal:
> > 
> > [  520.209749][T11984] MPTCP: bpf_iter_mptcp_subflow_new
> > msk=00000000fc8f7370 in_interrupt=0 task=00000000ef28139f
> > current=0000000024db2987
> > 
> > I will try to fix it, but haven't found a solution yet.
> 
> (sorry for the delay, I need a bit of time to catch up)
> 
> I talked a bit to Alexei Starovoitov last week. He told me that with
> the
> BPF struct_ops, it is possible to tell the verifier that some locks
> are
> taken either by some struct_ops types, or even per callbacks of some
> specific struct_ops (WIP on sched_ext side). It is also possible to
> get
> some locks automatically (polymorphism), and there are examples on
> VFS side.

Thanks for your reminder, I will look at these BPF codes. Our goal is
to make mptcp_subflow bpf_iter only used by struct_ops defined by MPTCP
BPF (bpf_mptcp_sched_ops and bpf_mptcp_pm_ops), right? Other struct_ops
are not allowed to use mptcp_subflow bpf_iter.

> 
> In other words, it means we don't need to add this "bpf_iter_task",
> there are other techniques, but I don't have more details, and I
> didn't
> check in the code. If it is not clear for you and you don't find 
> other
> examples elsewhere (sched_ext? check WIP patches maybe?), then Alexei
> said we should not hesitate to ask questions on the BPF mailing list.

We can send "Add mptcp_subflow bpf_iter support" v3 to the BPF mailing
list and ask questions during the reviewing process.

Thanks,
-Geliang

> 
> Cheers,
> Matt