[PATCH mptcp-next v3 0/2] add bpf_iter_task

[PATCH mptcp-next v3 0/2] add bpf_iter_task

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v3:
 - add bpf_iter_task in mptcp_sock, no need to add back struct
   mptcp_sched_data then.
 - add sk_lock_sock() and sk_release_sock() for struct sock.
 - set and clear bpf_iter_task for MPTCP BPF Cgroup getsockopt
   and setsockopt.

v2:
 - Keep mptcp scheduler API unchanged.
 - Add back struct mptcp_sched_data.
 - add bpf_iter_task in mptcp_sched_data instead of mptcp_sock
 - Add wrapper bpf_iter mptcp_subflow_sched.
 - Use mptcp_subflow_sched iter instead of mptcp_subflow.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1741347233.git.tanggeliang@kylinos.cn/

v1:
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1740997925.git.tanggeliang@kylinos.cn/

Geliang Tang (2):
  mptcp: add bpf_iter_task for mptcp_sock
  bpf: Customize mptcp's own sock lock

 include/net/sock.h   |  2 ++
 kernel/bpf/cgroup.c  |  8 ++++----
 net/mptcp/bpf.c      |  2 ++
 net/mptcp/protocol.c | 16 ++++++++++++++++
 net/mptcp/protocol.h | 20 ++++++++++++++++++++
 net/mptcp/sched.c    | 15 +++++++++++----
 6 files changed, 55 insertions(+), 8 deletions(-)

-- 
2.43.0

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

[Geliang Tang]

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;
+}
+
 #endif /* __MPTCP_PROTOCOL_H */
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index f09f7eb1d63f..161398f8960c 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -155,6 +155,7 @@ void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 int mptcp_sched_get_send(struct mptcp_sock *msk)
 {
 	struct mptcp_subflow_context *subflow;
+	int ret;
 
 	msk_owned_by_me(msk);
 
@@ -176,12 +177,16 @@ int mptcp_sched_get_send(struct mptcp_sock *msk)
 
 	if (msk->sched == &mptcp_sched_default || !msk->sched)
 		return mptcp_sched_default_get_send(msk);
-	return msk->sched->get_send(msk);
+	mptcp_set_bpf_iter_task(msk);
+	ret = msk->sched->get_send(msk);
+	mptcp_clear_bpf_iter_task(msk);
+	return ret;
 }
 
 int mptcp_sched_get_retrans(struct mptcp_sock *msk)
 {
 	struct mptcp_subflow_context *subflow;
+	int ret;
 
 	msk_owned_by_me(msk);
 
@@ -196,7 +201,9 @@ int mptcp_sched_get_retrans(struct mptcp_sock *msk)
 
 	if (msk->sched == &mptcp_sched_default || !msk->sched)
 		return mptcp_sched_default_get_retrans(msk);
-	if (msk->sched->get_retrans)
-		return msk->sched->get_retrans(msk);
-	return msk->sched->get_send(msk);
+	mptcp_set_bpf_iter_task(msk);
+	ret = msk->sched->get_retrans ? msk->sched->get_retrans(msk) :
+					msk->sched->get_send(msk);
+	mptcp_clear_bpf_iter_task(msk);
+	return ret;
 }
-- 
2.43.0

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

[Geliang Tang]

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.

Thanks,
-Geliang

> +
>  #endif /* __MPTCP_PROTOCOL_H */
> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index f09f7eb1d63f..161398f8960c 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -155,6 +155,7 @@ void mptcp_subflow_set_scheduled(struct
> mptcp_subflow_context *subflow,
>  int mptcp_sched_get_send(struct mptcp_sock *msk)
>  {
>  	struct mptcp_subflow_context *subflow;
> +	int ret;
>  
>  	msk_owned_by_me(msk);
>  
> @@ -176,12 +177,16 @@ int mptcp_sched_get_send(struct mptcp_sock
> *msk)
>  
>  	if (msk->sched == &mptcp_sched_default || !msk->sched)
>  		return mptcp_sched_default_get_send(msk);
> -	return msk->sched->get_send(msk);
> +	mptcp_set_bpf_iter_task(msk);
> +	ret = msk->sched->get_send(msk);
> +	mptcp_clear_bpf_iter_task(msk);
> +	return ret;
>  }
>  
>  int mptcp_sched_get_retrans(struct mptcp_sock *msk)
>  {
>  	struct mptcp_subflow_context *subflow;
> +	int ret;
>  
>  	msk_owned_by_me(msk);
>  
> @@ -196,7 +201,9 @@ int mptcp_sched_get_retrans(struct mptcp_sock
> *msk)
>  
>  	if (msk->sched == &mptcp_sched_default || !msk->sched)
>  		return mptcp_sched_default_get_retrans(msk);
> -	if (msk->sched->get_retrans)
> -		return msk->sched->get_retrans(msk);
> -	return msk->sched->get_send(msk);
> +	mptcp_set_bpf_iter_task(msk);
> +	ret = msk->sched->get_retrans ? msk->sched->get_retrans(msk)
> :
> +					msk->sched->get_send(msk);
> +	mptcp_clear_bpf_iter_task(msk);
> +	return ret;
>  }

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

[Matthieu Baerts]

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.

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.

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

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

[Geliang Tang]

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

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

[Matthieu Baerts]

Hi Geliang,

On 17/03/2025 11:59, Geliang Tang wrote:
> 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.

(...)

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

Yes, that's correct. I didn't check, but **maybe** some mptcp_sched_ops
and mptcp_pm_ops callback might not be allowed to use bpf_iter. In this
case, it might be needed to allow only some of them to use bpf_iter.

Note that it sounds like all mptcp_{pm,sched}_ops callbacks should be
done while holding the msk lock, e.g. being called from the worker and
not from a subflow event, etc. but maybe there are some exceptions needed.

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

Indeed. I hope to be able to find a bit of time this week to send the v3.

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

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

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3978 bytes --]

On Mon, 17 Mar 2025, Matthieu Baerts wrote:

> Hi Geliang,
>
> On 17/03/2025 11:59, Geliang Tang wrote:
>> 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.
>
> (...)
>
>>>>> +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.
>
> Yes, that's correct. I didn't check, but **maybe** some mptcp_sched_ops
> and mptcp_pm_ops callback might not be allowed to use bpf_iter. In this
> case, it might be needed to allow only some of them to use bpf_iter.
>
> Note that it sounds like all mptcp_{pm,sched}_ops callbacks should be
> done while holding the msk lock, e.g. being called from the worker and
> not from a subflow event, etc. but maybe there are some exceptions needed.

Hi Matthieu -

Is this last paragraph also feedback from Alexei? Can you elaborate a 
little - is the lock needed for BPF-specific reasons or to avoid having 
the callback modify msk state?

I'll try to look around at the verifier and automatic locking techniques 
you mentioned above, it would be great to have a better way to handle 
socket locks w/ BPF.

- Mat

>
>>> 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.
>
> Indeed. I hope to be able to find a bit of time this week to send the v3.
>
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.
>
>

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

[Matthieu Baerts]

Hi Mat,

On 18/03/2025 02:25, Mat Martineau wrote:
> On Mon, 17 Mar 2025, Matthieu Baerts wrote:
> 
>> Hi Geliang,
>>
>> On 17/03/2025 11:59, Geliang Tang wrote:
>>> 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.
>>
>> (...)
>>
>>>>>> +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.
>>
>> Yes, that's correct. I didn't check, but **maybe** some mptcp_sched_ops
>> and mptcp_pm_ops callback might not be allowed to use bpf_iter. In this
>> case, it might be needed to allow only some of them to use bpf_iter.
>>
>> Note that it sounds like all mptcp_{pm,sched}_ops callbacks should be
>> done while holding the msk lock, e.g. being called from the worker and
>> not from a subflow event, etc. but maybe there are some exceptions
>> needed.
> 
> Hi Matthieu -
> 
> Is this last paragraph also feedback from Alexei? Can you elaborate a
> little - is the lock needed for BPF-specific reasons or to avoid having
> the callback modify msk state?

No, it is not from Alexei, but from me. Currently, some actions on a
subflow will call some code in pm.c that will simply schedule the worker
to be called from a different context to do other actions. To convert
this to an mptcp_pm_ops, I think we should not have two callbacks -- one
to schedule the worker, and one to react from the worker context -- but
only one called from the worker context if such callback is set. This
should also simplify the BPF PMs if all/most callbacks are done from the
same context while owning the msk lock, no?

> I'll try to look around at the verifier and automatic locking techniques
> you mentioned above, it would be great to have a better way to handle
> socket locks w/ BPF.

Alexei suggested to look at the VFS code. If I understood correctly, the
idea is to have some locks automatically taken for some actions, but
without having to dedicate new kfunc, checking if they are already
taken, or requiring the BPF code to handle them (auto-releasing them
then). But I don't have more details about that.

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

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

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 4676 bytes --]

On Tue, 18 Mar 2025, Matthieu Baerts wrote:

> Hi Mat,
>
> On 18/03/2025 02:25, Mat Martineau wrote:
>> On Mon, 17 Mar 2025, Matthieu Baerts wrote:
>>
>>> Hi Geliang,
>>>
>>> On 17/03/2025 11:59, Geliang Tang wrote:
>>>> 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.
>>>
>>> (...)
>>>
>>>>>>> +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.
>>>
>>> Yes, that's correct. I didn't check, but **maybe** some mptcp_sched_ops
>>> and mptcp_pm_ops callback might not be allowed to use bpf_iter. In this
>>> case, it might be needed to allow only some of them to use bpf_iter.
>>>
>>> Note that it sounds like all mptcp_{pm,sched}_ops callbacks should be
>>> done while holding the msk lock, e.g. being called from the worker and
>>> not from a subflow event, etc. but maybe there are some exceptions
>>> needed.
>>
>> Hi Matthieu -
>>
>> Is this last paragraph also feedback from Alexei? Can you elaborate a
>> little - is the lock needed for BPF-specific reasons or to avoid having
>> the callback modify msk state?
>
> No, it is not from Alexei, but from me. Currently, some actions on a
> subflow will call some code in pm.c that will simply schedule the worker
> to be called from a different context to do other actions. To convert
> this to an mptcp_pm_ops, I think we should not have two callbacks -- one
> to schedule the worker, and one to react from the worker context -- but
> only one called from the worker context if such callback is set. This
> should also simplify the BPF PMs if all/most callbacks are done from the
> same context while owning the msk lock, no?
>

Ok, thanks for the details. I agree, for the PM callbacks it makes sense 
to only have a worker-context callback.


- Mat


>> I'll try to look around at the verifier and automatic locking techniques
>> you mentioned above, it would be great to have a better way to handle
>> socket locks w/ BPF.
>
> Alexei suggested to look at the VFS code. If I understood correctly, the
> idea is to have some locks automatically taken for some actions, but
> without having to dedicate new kfunc, checking if they are already
> taken, or requiring the BPF code to handle them (auto-releasing them
> then). But I don't have more details about that.
>

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

[Geliang Tang]

Hi Matt,

On Mon, 2025-03-17 at 14:57 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 17/03/2025 11:59, Geliang Tang wrote:
> > 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.
> 
> (...)
> 
> > > > > +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.

I checked and found that other struct_ops **cannot** use mptcp_subflow
bpf_iter. Although we registered this bpf_iter for use with
BPF_PROG_TYPE_STRUCT_OPS type, we checked in bpf_iter_mptcp_subflow_new
that sk->sk_protocol must be IPPROTO_MPTCP. Does this mean that other
struct_ops cannot use mptcp_subflow bpf_iter successfully? I don't know
if this check is sufficient.

> 
> Yes, that's correct. I didn't check, but **maybe** some
> mptcp_sched_ops
> and mptcp_pm_ops callback might not be allowed to use bpf_iter. In
> this
> case, it might be needed to allow only some of them to use bpf_iter.

I guess it's not easy to allow only some callbacks of a struct_ops to
access certain functions, because The BPF verifier verifies the
struct_ops as a whole. But I will continue to look for a solution.

> 
> Note that it sounds like all mptcp_{pm,sched}_ops callbacks should be
> done while holding the msk lock, e.g. being called from the worker
> and
> not from a subflow event, etc. but maybe there are some exceptions
> needed.

I checked the following 19 callbacks of mptcp_{pm,sched}_ops that have
been implemented in my code tree. Except for the three exceptions
(get_local_id, get_priority and add_addr_received), the other 17
callbacks are all be done while holding the msk lock:

struct mptcp_sched_ops {
        int (*get_send)(struct mptcp_sock *msk);
        int (*get_retrans)(struct mptcp_sock *msk);
}

struct mptcp_pm_ops {
        /* required */
        int (*get_local_id)(struct mptcp_sock *msk,
                            struct mptcp_pm_addr_entry *skc);
        bool (*get_priority)(struct mptcp_sock *msk,
                             struct mptcp_addr_info *skc);

        /* optional */
        void (*established)(struct mptcp_sock *msk);
        void (*subflow_established)(struct mptcp_sock *msk);

        /* required */
        bool (*allow_new_subflow)(struct mptcp_sock *msk);
        bool (*accept_new_subflow)(const struct mptcp_sock *msk);
        bool (*add_addr_echo)(struct mptcp_sock *msk,
                              const struct mptcp_addr_info *addr);

        /* optional */
        int (*add_addr_received)(struct mptcp_sock *msk,
                                 const struct mptcp_addr_info *addr);
        void (*rm_addr_received)(struct mptcp_sock *msk);

        /* optional */
        int (*add_addr)(struct mptcp_sock *msk,
                        struct mptcp_pm_addr_entry *entry);
        int (*del_addr)(struct mptcp_sock *msk,
                        const struct mptcp_pm_addr_entry *entry);
        int (*flush_addrs)(struct mptcp_sock *msk,
                           struct list_head *rm_list);

        /* optional */
        int (*address_announce)(struct mptcp_sock *msk,
                                struct mptcp_pm_addr_entry *local);
        int (*address_remove)(struct mptcp_sock *msk,
                              struct mptcp_pm_addr_entry *local);
        int (*subflow_create)(struct mptcp_sock *msk,
                              struct mptcp_pm_addr_entry *local,
                              struct mptcp_addr_info *remote);
        int (*subflow_destroy)(struct mptcp_sock *msk,
                               struct mptcp_pm_addr_entry *local,
                               struct mptcp_addr_info *remote);

        /* required */
        int (*set_priority)(struct mptcp_sock *msk,
                            struct mptcp_pm_addr_entry *local,
                            struct mptcp_pm_addr_entry *remote);
};

For these three exceptions, add_addr_received is invoked in
mptcp_pm_add_addr_received, here the socket lock of ssk is already
holding.

Similarly, in subflow_chk_local_id, get_local_id and get_priority are
called, where the socket lock of ssk is already holding too.

In addition, get_local_id and get_priority are also called in
subflow_token_join_request, which is in atomic and can hold msk lock
through bh_lock_sock. If locking is required here, I can send a patch
to do this.

In this way, all callbacks of mptcp_{pm,sched}_ops can be done while
holding the msk lock or the ssk lock.

Thanks,
-Geliang

> 
> > > 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.
> 
> Indeed. I hope to be able to find a bit of time this week to send the
> v3.
> 
> Cheers,
> Matt

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

[Matthieu Baerts]

Hi Geliang,

On 18/03/2025 11:35, Geliang Tang wrote:
> Hi Matt,
> 
> On Mon, 2025-03-17 at 14:57 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 17/03/2025 11:59, Geliang Tang wrote:
>>> 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.
>>
>> (...)
>>
>>>>>> +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.
> 
> I checked and found that other struct_ops **cannot** use mptcp_subflow
> bpf_iter. Although we registered this bpf_iter for use with
> BPF_PROG_TYPE_STRUCT_OPS type, we checked in bpf_iter_mptcp_subflow_new
> that sk->sk_protocol must be IPPROTO_MPTCP. Does this mean that other
> struct_ops cannot use mptcp_subflow bpf_iter successfully? I don't know
> if this check is sufficient.

Sorry, I don't know. With the current version, I don't see any links
between mptcp_subflow bpf_iter and mptcp_{sched,pm}_ops, then I don't
know how the restriction works, right? I guess there might be a
restriction because "struct mptcp_sock*" are used in arguments? But I
guess that's not enough because such structures can be obtained from
different struct_ops. Probably something else is missing to have this
link then?

>> Yes, that's correct. I didn't check, but **maybe** some
>> mptcp_sched_ops
>> and mptcp_pm_ops callback might not be allowed to use bpf_iter. In
>> this
>> case, it might be needed to allow only some of them to use bpf_iter.
> 
> I guess it's not easy to allow only some callbacks of a struct_ops to
> access certain functions, because The BPF verifier verifies the
> struct_ops as a whole. But I will continue to look for a solution.

Alexei told me that this work was in progress for sched_ext, but I don't
know more about that, sorry.

>> Note that it sounds like all mptcp_{pm,sched}_ops callbacks should be
>> done while holding the msk lock, e.g. being called from the worker
>> and
>> not from a subflow event, etc. but maybe there are some exceptions
>> needed.
> 
> I checked the following 19 callbacks of mptcp_{pm,sched}_ops that have
> been implemented in my code tree. Except for the three exceptions
> (get_local_id, get_priority and add_addr_received), the other 17
> callbacks are all be done while holding the msk lock:

Thank you for having checked!

> struct mptcp_sched_ops {
>         int (*get_send)(struct mptcp_sock *msk);
>         int (*get_retrans)(struct mptcp_sock *msk);
> }
> 
> struct mptcp_pm_ops {
>         /* required */
>         int (*get_local_id)(struct mptcp_sock *msk,
>                             struct mptcp_pm_addr_entry *skc);
>         bool (*get_priority)(struct mptcp_sock *msk,
>                              struct mptcp_addr_info *skc);
> 
>         /* optional */
>         void (*established)(struct mptcp_sock *msk);
>         void (*subflow_established)(struct mptcp_sock *msk);
> 
>         /* required */
>         bool (*allow_new_subflow)(struct mptcp_sock *msk);
>         bool (*accept_new_subflow)(const struct mptcp_sock *msk);
>         bool (*add_addr_echo)(struct mptcp_sock *msk,
>                               const struct mptcp_addr_info *addr);
> 
>         /* optional */
>         int (*add_addr_received)(struct mptcp_sock *msk,
>                                  const struct mptcp_addr_info *addr);
>         void (*rm_addr_received)(struct mptcp_sock *msk);
> 
>         /* optional */
>         int (*add_addr)(struct mptcp_sock *msk,
>                         struct mptcp_pm_addr_entry *entry);
>         int (*del_addr)(struct mptcp_sock *msk,
>                         const struct mptcp_pm_addr_entry *entry);
>         int (*flush_addrs)(struct mptcp_sock *msk,
>                            struct list_head *rm_list);
> 
>         /* optional */
>         int (*address_announce)(struct mptcp_sock *msk,
>                                 struct mptcp_pm_addr_entry *local);
>         int (*address_remove)(struct mptcp_sock *msk,
>                               struct mptcp_pm_addr_entry *local);
>         int (*subflow_create)(struct mptcp_sock *msk,
>                               struct mptcp_pm_addr_entry *local,
>                               struct mptcp_addr_info *remote);
>         int (*subflow_destroy)(struct mptcp_sock *msk,
>                                struct mptcp_pm_addr_entry *local,
>                                struct mptcp_addr_info *remote);
> 
>         /* required */
>         int (*set_priority)(struct mptcp_sock *msk,
>                             struct mptcp_pm_addr_entry *local,
>                             struct mptcp_pm_addr_entry *remote);

Mmh, all 8 callbacks from add_addr to here seem to be linked to Netlink
commands, right? If yes, they should not be here: they don't make sense
for a BPF PM, no? BPF PMs should be configured with BPF, and not via
Netlink. Netlink is only for the built-in PMs (in-kernel and userspace PMs)

> };
> 
> For these three exceptions, add_addr_received is invoked in
> mptcp_pm_add_addr_received, here the socket lock of ssk is already
> holding.

For add_addr_received, I think it is safer to have the callback from the
worker context. In other words, when an ADD_ADDR received on a subflow:

- the ADD_ADDR echo should be sent: I don't think it is worth it letting
the other peer resending it just in case the userspace PM was not "ready"

- if pm->ops->add_addr_received is set, schedule the worker and set
  msk->pm.remote

- then pm->ops->add_addr_received will be called from the worker.

> Similarly, in subflow_chk_local_id, get_local_id and get_priority are
> called, where the socket lock of ssk is already holding too.
> 
> In addition, get_local_id and get_priority are also called in
> subflow_token_join_request, which is in atomic and can hold msk lock
> through bh_lock_sock. If locking is required here, I can send a patch
> to do this.

Yes, for the ID and backup, I guess we will need an exception there, and
a way not to let the BPF PMs calling bpf_iter. We should not hold the
msk lock here.

I guess the easier would be to ask the BPF maintainers what we should do
here. Maybe this can be done after having sent the mptcp_subflow v3
series, or at the same time. I will see what I can do.

> In this way, all callbacks of mptcp_{pm,sched}_ops can be done while
> holding the msk lock or the ssk lock.

At least on the scheduler side, that will be the case (or maybe not if
we change the API to cover more cases :) ).

Anyway, probably best to wait for BPF maintainers recommendations
instead of guessing.

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

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

[Geliang Tang]

Hi Matt,

On Tue, 2025-03-18 at 12:26 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 18/03/2025 11:35, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Mon, 2025-03-17 at 14:57 +0100, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 17/03/2025 11:59, Geliang Tang wrote:
> > > > 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.
> > > 
> > > (...)
> > > 
> > > > > > > +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.
> > 
> > I checked and found that other struct_ops **cannot** use
> > mptcp_subflow
> > bpf_iter. Although we registered this bpf_iter for use with
> > BPF_PROG_TYPE_STRUCT_OPS type, we checked in
> > bpf_iter_mptcp_subflow_new
> > that sk->sk_protocol must be IPPROTO_MPTCP. Does this mean that
> > other
> > struct_ops cannot use mptcp_subflow bpf_iter successfully? I don't
> > know
> > if this check is sufficient.
> 
> Sorry, I don't know. With the current version, I don't see any links
> between mptcp_subflow bpf_iter and mptcp_{sched,pm}_ops, then I don't
> know how the restriction works, right? I guess there might be a
> restriction because "struct mptcp_sock*" are used in arguments? But I
> guess that's not enough because such structures can be obtained from
> different struct_ops. Probably something else is missing to have this
> link then?
> 
> > > Yes, that's correct. I didn't check, but **maybe** some
> > > mptcp_sched_ops
> > > and mptcp_pm_ops callback might not be allowed to use bpf_iter.
> > > In
> > > this
> > > case, it might be needed to allow only some of them to use
> > > bpf_iter.
> > 
> > I guess it's not easy to allow only some callbacks of a struct_ops
> > to
> > access certain functions, because The BPF verifier verifies the
> > struct_ops as a whole. But I will continue to look for a solution.
> 
> Alexei told me that this work was in progress for sched_ext, but I
> don't
> know more about that, sorry.
> 
> > > Note that it sounds like all mptcp_{pm,sched}_ops callbacks
> > > should be
> > > done while holding the msk lock, e.g. being called from the
> > > worker
> > > and
> > > not from a subflow event, etc. but maybe there are some
> > > exceptions
> > > needed.
> > 
> > I checked the following 19 callbacks of mptcp_{pm,sched}_ops that
> > have
> > been implemented in my code tree. Except for the three exceptions
> > (get_local_id, get_priority and add_addr_received), the other 17
> > callbacks are all be done while holding the msk lock:
> 
> Thank you for having checked!
> 
> > struct mptcp_sched_ops {
> >         int (*get_send)(struct mptcp_sock *msk);
> >         int (*get_retrans)(struct mptcp_sock *msk);
> > }
> > 
> > struct mptcp_pm_ops {
> >         /* required */
> >         int (*get_local_id)(struct mptcp_sock *msk,
> >                             struct mptcp_pm_addr_entry *skc);
> >         bool (*get_priority)(struct mptcp_sock *msk,
> >                              struct mptcp_addr_info *skc);
> > 
> >         /* optional */
> >         void (*established)(struct mptcp_sock *msk);
> >         void (*subflow_established)(struct mptcp_sock *msk);
> > 
> >         /* required */
> >         bool (*allow_new_subflow)(struct mptcp_sock *msk);
> >         bool (*accept_new_subflow)(const struct mptcp_sock *msk);
> >         bool (*add_addr_echo)(struct mptcp_sock *msk,
> >                               const struct mptcp_addr_info *addr);
> > 
> >         /* optional */
> >         int (*add_addr_received)(struct mptcp_sock *msk,
> >                                  const struct mptcp_addr_info
> > *addr);
> >         void (*rm_addr_received)(struct mptcp_sock *msk);
> > 
> >         /* optional */
> >         int (*add_addr)(struct mptcp_sock *msk,
> >                         struct mptcp_pm_addr_entry *entry);
> >         int (*del_addr)(struct mptcp_sock *msk,
> >                         const struct mptcp_pm_addr_entry *entry);
> >         int (*flush_addrs)(struct mptcp_sock *msk,
> >                            struct list_head *rm_list);
> > 
> >         /* optional */
> >         int (*address_announce)(struct mptcp_sock *msk,
> >                                 struct mptcp_pm_addr_entry *local);
> >         int (*address_remove)(struct mptcp_sock *msk,
> >                               struct mptcp_pm_addr_entry *local);
> >         int (*subflow_create)(struct mptcp_sock *msk,
> >                               struct mptcp_pm_addr_entry *local,
> >                               struct mptcp_addr_info *remote);
> >         int (*subflow_destroy)(struct mptcp_sock *msk,
> >                                struct mptcp_pm_addr_entry *local,
> >                                struct mptcp_addr_info *remote);
> > 
> >         /* required */
> >         int (*set_priority)(struct mptcp_sock *msk,
> >                             struct mptcp_pm_addr_entry *local,
> >                             struct mptcp_pm_addr_entry *remote);
> 
> Mmh, all 8 callbacks from add_addr to here seem to be linked to
> Netlink
> commands, right? If yes, they should not be here: they don't make
> sense
> for a BPF PM, no? BPF PMs should be configured with BPF, and not via

Do you have any idea how we can pass commands like ADD_ADDR and
addresses from user space to BPF?

What I can think of is passing through sockopt, the user space passes
the command and the address through setsockopt, and the BPF program
handles it through the custom "cgroup/setsockopt". I would like to hear
your opinions. If you also agree to use sockopt to implement it, I can
write a test program for this.

Thanks,
-Geliang

> Netlink. Netlink is only for the built-in PMs (in-kernel and
> userspace PMs)
> 
> > };
> > 
> > For these three exceptions, add_addr_received is invoked in
> > mptcp_pm_add_addr_received, here the socket lock of ssk is already
> > holding.
> 
> For add_addr_received, I think it is safer to have the callback from
> the
> worker context. In other words, when an ADD_ADDR received on a
> subflow:
> 
> - the ADD_ADDR echo should be sent: I don't think it is worth it
> letting
> the other peer resending it just in case the userspace PM was not
> "ready"
> 
> - if pm->ops->add_addr_received is set, schedule the worker and set
>   msk->pm.remote
> 
> - then pm->ops->add_addr_received will be called from the worker.
> 
> > Similarly, in subflow_chk_local_id, get_local_id and get_priority
> > are
> > called, where the socket lock of ssk is already holding too.
> > 
> > In addition, get_local_id and get_priority are also called in
> > subflow_token_join_request, which is in atomic and can hold msk
> > lock
> > through bh_lock_sock. If locking is required here, I can send a
> > patch
> > to do this.
> 
> Yes, for the ID and backup, I guess we will need an exception there,
> and
> a way not to let the BPF PMs calling bpf_iter. We should not hold the
> msk lock here.
> 
> I guess the easier would be to ask the BPF maintainers what we should
> do
> here. Maybe this can be done after having sent the mptcp_subflow v3
> series, or at the same time. I will see what I can do.
> 
> > In this way, all callbacks of mptcp_{pm,sched}_ops can be done
> > while
> > holding the msk lock or the ssk lock.
> 
> At least on the scheduler side, that will be the case (or maybe not
> if
> we change the API to cover more cases :) ).
> 
> Anyway, probably best to wait for BPF maintainers recommendations
> instead of guessing.
> 
> Cheers,
> Matt

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

[Matthieu Baerts]

Hi Geliang,

On 21/03/2025 05:14, Geliang Tang wrote:
> On Tue, 2025-03-18 at 12:26 +0100, Matthieu Baerts wrote:

(...)

>> Mmh, all 8 callbacks from add_addr to here seem to be linked to
>> Netlink
>> commands, right? If yes, they should not be here: they don't make
>> sense
>> for a BPF PM, no? BPF PMs should be configured with BPF, and not via
>
> Do you have any idea how we can pass commands like ADD_ADDR and
> addresses from user space to BPF?

The userspace can inject any kind of BPF program. Additional addresses
can then be hardcoded in this program, or it can load them from eBPF
maps, or read skel->bss->(...), etc.

Netlink is then not needed.

> What I can think of is passing through sockopt, the user space passes
> the command and the address through setsockopt, and the BPF program
> handles it through the custom "cgroup/setsockopt". I would like to hear
> your opinions. If you also agree to use sockopt to implement it, I can
> write a test program for this.
That's a good idea for userspace apps which want to control how the
MPTCP PM is acting. I guess MPTCP BPF PM will generally not be linked to
network apps: a separate application dedicated to inject and configure
the BPF PM, no?

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

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

[Geliang Tang]

Hi Matt,

On Fri, 2025-03-21 at 10:19 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 21/03/2025 05:14, Geliang Tang wrote:
> > On Tue, 2025-03-18 at 12:26 +0100, Matthieu Baerts wrote:
> 
> (...)
> 
> > > Mmh, all 8 callbacks from add_addr to here seem to be linked to
> > > Netlink
> > > commands, right? If yes, they should not be here: they don't make
> > > sense
> > > for a BPF PM, no? BPF PMs should be configured with BPF, and not
> > > via
> > 
> > Do you have any idea how we can pass commands like ADD_ADDR and
> > addresses from user space to BPF?
> 
> The userspace can inject any kind of BPF program. Additional
> addresses
> can then be hardcoded in this program, or it can load them from eBPF
> maps, or read skel->bss->(...), etc.
> 
> Netlink is then not needed.
> 
> > What I can think of is passing through sockopt, the user space
> > passes
> > the command and the address through setsockopt, and the BPF program
> > handles it through the custom "cgroup/setsockopt". I would like to
> > hear
> > your opinions. If you also agree to use sockopt to implement it, I
> > can
> > write a test program for this.

I just tested it and found it difficult to use setsockopt.

"cgroup/setsockopt" program doesn't work. We need to invoke sleepable
kfuncs in BPF PM:

BTF_ID_FLAGS(func, mptcp_pm_remove_addr_entry, KF_SLEEPABLE)
BTF_ID_FLAGS(func, mptcp_pm_addr_send_ack, KF_SLEEPABLE)
BTF_ID_FLAGS(func, mptcp_pm_mp_prio_send_ack, KF_SLEEPABLE)
BTF_ID_FLAGS(func, bpf_mptcp_subflow_connect, KF_SLEEPABLE)
BTF_ID_FLAGS(func, mptcp_subflow_shutdown, KF_SLEEPABLE)
BTF_ID_FLAGS(func, mptcp_close_ssk, KF_SLEEPABLE)

They can't be invoked in "cgroup/setsockopt" program. And
"cgroup/setsockopt" program can't be set with BPF_F_SLEEPABLE flag:

        err = err ?: bpf_program__set_flags(skel->progs.pm_setsockopt,
                                            BPF_F_SLEEPABLE);

With this error occurs:

test_bpf_hashmap_pm:PASS:set sleepable flags 0 nsec
libbpf: prog 'pm_setsockopt': BPF program load failed: -EINVAL
libbpf: prog 'pm_setsockopt': -- BEGIN PROG LOAD LOG --
Only fentry/fexit/fmod_ret, lsm, iter, uprobe, and struct_ops programs
can be sleepable
processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0
peak_states 0 mark_read 0

It seems that we can only choose one of fentry/fexit/fmod_ret, lsm,
iter, uprobe, or struct_ops type to implement BPF PM.

Thanks,
-Geliang

> That's a good idea for userspace apps which want to control how the
> MPTCP PM is acting. I guess MPTCP BPF PM will generally not be linked
> to
> network apps: a separate application dedicated to inject and
> configure
> the BPF PM, no?
> 
> Cheers,
> Matt

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

[Matthieu Baerts]

On 21/03/2025 11:05, Geliang Tang wrote:
> Hi Matt,
> 
> On Fri, 2025-03-21 at 10:19 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 21/03/2025 05:14, Geliang Tang wrote:
>>> On Tue, 2025-03-18 at 12:26 +0100, Matthieu Baerts wrote:
>>
>> (...)
>>
>>>> Mmh, all 8 callbacks from add_addr to here seem to be linked to
>>>> Netlink
>>>> commands, right? If yes, they should not be here: they don't make
>>>> sense
>>>> for a BPF PM, no? BPF PMs should be configured with BPF, and not
>>>> via
>>>
>>> Do you have any idea how we can pass commands like ADD_ADDR and
>>> addresses from user space to BPF?
>>
>> The userspace can inject any kind of BPF program. Additional
>> addresses
>> can then be hardcoded in this program, or it can load them from eBPF
>> maps, or read skel->bss->(...), etc.
>>
>> Netlink is then not needed.
>>
>>> What I can think of is passing through sockopt, the user space
>>> passes
>>> the command and the address through setsockopt, and the BPF program
>>> handles it through the custom "cgroup/setsockopt". I would like to
>>> hear
>>> your opinions. If you also agree to use sockopt to implement it, I
>>> can
>>> write a test program for this.
> 
> I just tested it and found it difficult to use setsockopt.
> 
> "cgroup/setsockopt" program doesn't work. We need to invoke sleepable
> kfuncs in BPF PM:

(...)

> It seems that we can only choose one of fentry/fexit/fmod_ret, lsm,
> iter, uprobe, or struct_ops type to implement BPF PM.
Thank you for having checked. I think we can look at that aspect later.
I don't think many apps will want this kind of control. They can always
set something in a map, and the BPF program can look at it when needed.

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

[PATCH mptcp-next v3 2/2] bpf: Customize mptcp's own sock lock

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

In order to customize mptcp's own sock lock and release functions,
sk_lock_sock() and sk_release_sock() function pointers are added to
struct sock, and these pointers are called in BPF Cgroup getsockopt
and setsockopt.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/sock.h   |  2 ++
 kernel/bpf/cgroup.c  |  8 ++++----
 net/mptcp/protocol.c | 15 +++++++++++++++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 8daf1b3b12c6..4341c58e351e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -540,6 +540,8 @@ struct sock {
 	int			(*sk_backlog_rcv)(struct sock *sk,
 						  struct sk_buff *skb);
 	void                    (*sk_destruct)(struct sock *sk);
+	void                    (*sk_lock_sock)(struct sock *sk);
+	void                    (*sk_release_sock)(struct sock *sk);
 	struct sock_reuseport __rcu	*sk_reuseport_cb;
 #ifdef CONFIG_BPF_SYSCALL
 	struct bpf_local_storage __rcu	*sk_bpf_storage;
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 46e5db65dbc8..a105e9e5b9c8 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1843,10 +1843,10 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
 		goto out;
 	}
 
-	lock_sock(sk);
+	sk->sk_lock_sock ? sk->sk_lock_sock(sk) : lock_sock(sk);
 	ret = bpf_prog_run_array_cg(&cgrp->bpf, CGROUP_SETSOCKOPT,
 				    &ctx, bpf_prog_run, 0, NULL);
-	release_sock(sk);
+	sk->sk_release_sock ? sk->sk_release_sock(sk) : release_sock(sk);
 
 	if (ret)
 		goto out;
@@ -1952,10 +1952,10 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
 		}
 	}
 
-	lock_sock(sk);
+	sk->sk_lock_sock ? sk->sk_lock_sock(sk) : lock_sock(sk);
 	ret = bpf_prog_run_array_cg(&cgrp->bpf, CGROUP_GETSOCKOPT,
 				    &ctx, bpf_prog_run, retval, NULL);
-	release_sock(sk);
+	sk->sk_release_sock ? sk->sk_release_sock(sk) : release_sock(sk);
 
 	if (ret < 0)
 		goto out;
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d98e48ce8cd8..29c3ee2fb4cd 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2712,6 +2712,18 @@ static void mptcp_worker(struct work_struct *work)
 	sock_put(sk);
 }
 
+static void mptcp_sk_lock_sock(struct sock *sk)
+{
+	lock_sock(sk);
+	mptcp_set_bpf_iter_task(mptcp_sk(sk));
+}
+
+static void mptcp_sk_release_sock(struct sock *sk)
+{
+	mptcp_clear_bpf_iter_task(mptcp_sk(sk));
+	release_sock(sk);
+}
+
 static void __mptcp_init_sock(struct sock *sk)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
@@ -2741,6 +2753,9 @@ static void __mptcp_init_sock(struct sock *sk)
 	/* re-use the csk retrans timer for MPTCP-level retrans */
 	timer_setup(&msk->sk.icsk_retransmit_timer, mptcp_retransmit_timer, 0);
 	timer_setup(&sk->sk_timer, mptcp_tout_timer, 0);
+
+	sk->sk_lock_sock = mptcp_sk_lock_sock;
+	sk->sk_release_sock = mptcp_sk_release_sock;
 }
 
 static void mptcp_ca_reset(struct sock *sk)
-- 
2.43.0

Re: [PATCH mptcp-next v3 0/2] add bpf_iter_task

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Critical: Global Timeout ❌
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13756425685

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ae5890ca8bfa
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=942077


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)