diff mbox series

[mptcp-next,v5,3/5] Squash to "bpf: Add mptcp_subflow bpf_iter"

Message ID f8d59dcb8391483787f832ddb43035846055aafb.1740997925.git.tanggeliang@kylinos.cn (mailing list archive)
State Rejected, archived
Delegated to: Matthieu Baerts
Headers show
Series Squash to "Add mptcp_subflow bpf_iter support" | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 39 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang March 3, 2025, 10:33 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

Drop the NULL check for 'msk' as Martin suggested, add more checks
for 'sk'.

Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as the
argument in the bpf_iter_mptcp_subflow_new as Martin suggested.

v5:
 - check bpf_iter_task in mptcp_subflow_new()

v4:
 - drop sock_owned_by_user_nocheck and spin_is_locked. According to
   comments from Mat [2] and Martin [1], in this set mptcp_subflow
   bpf_iter only used from a cg sockopt bpf prog, no need to add these
   check at this moment.

[1]
https://lore.kernel.org/all/fdf0ddbe-e007-4a5f-bbdf-9a144e8fbe35@linux.dev/
[2]
https://patchwork.kernel.org/project/mptcp/patch/f6469225598beecbf0bda12a4c33fafa86c0ff15.1739787744.git.tanggeliang@kylinos.cn/

v3:
 - continue to use sock_owned_by_user_nocheck and spin_is_locked
checks instead of using msk_owned_by_me().

v2:
 - check the owner before assigning the msk as Mat suggested.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/bpf.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Comments

Mat Martineau March 4, 2025, 1:39 a.m. UTC | #1 
On Mon, 3 Mar 2025, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Drop the NULL check for 'msk' as Martin suggested, add more checks
> for 'sk'.
>
> Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as the
> argument in the bpf_iter_mptcp_subflow_new as Martin suggested.
>
> v5:
> - check bpf_iter_task in mptcp_subflow_new()
>
> v4:
> - drop sock_owned_by_user_nocheck and spin_is_locked. According to
>   comments from Mat [2] and Martin [1], in this set mptcp_subflow
>   bpf_iter only used from a cg sockopt bpf prog, no need to add these
>   check at this moment.
>
> [1]
> https://lore.kernel.org/all/fdf0ddbe-e007-4a5f-bbdf-9a144e8fbe35@linux.dev/
> [2]
> https://patchwork.kernel.org/project/mptcp/patch/f6469225598beecbf0bda12a4c33fafa86c0ff15.1739787744.git.tanggeliang@kylinos.cn/
>
> v3:
> - continue to use sock_owned_by_user_nocheck and spin_is_locked
> checks instead of using msk_owned_by_me().
>
> v2:
> - check the owner before assigning the msk as Mat suggested.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> net/mptcp/bpf.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index aff92f458e7f..a307490bb20e 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -249,24 +249,33 @@ bpf_mptcp_subflow_ctx(const struct sock *sk)
>
> __bpf_kfunc static int
> bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
> -			   struct mptcp_sock *msk)
> +			   struct sock *sk)
> {
> 	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> -	struct sock *sk = (struct sock *)msk;
> +	struct task_struct *task;
> +	struct mptcp_sock *msk;
>
> 	BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) >
> 		     sizeof(struct bpf_iter_mptcp_subflow));
> 	BUILD_BUG_ON(__alignof__(struct bpf_iter_mptcp_subflow_kern) !=
> 		     __alignof__(struct bpf_iter_mptcp_subflow));
>
> -	kit->msk = msk;
> -	if (!msk)
> +	if (unlikely(!sk || !sk_fullsock(sk)))
> 		return -EINVAL;
>
> -	if (!sock_owned_by_user_nocheck(sk) &&
> -	    !spin_is_locked(&sk->sk_lock.slock))
> +	if (sk->sk_protocol != IPPROTO_MPTCP)
> 		return -EINVAL;
>
> +	msk = mptcp_sk(sk);
> +	task = mptcp_get_bpf_iter_task(msk);
> +	if (!task || task != current)
> +		return -EINVAL;

Hi Geliang -

The task checking logic would fit better inside the helper, since every 
caller will be checking to see if it matches 'current'.

I also think it would read better as (task && task == current)


> +
> +	mptcp_clear_bpf_iter_task(msk);

This needs to be cleared where the lock is released, not where the task is 
checked.

- Mat

> +
> +	msk_owned_by_me(msk);
> +
> +	kit->msk = msk;
> 	kit->pos = &msk->conn_list;
> 	return 0;
> }
> -- 
> 2.43.0
>
>
>
Geliang Tang March 7, 2025, 3:54 a.m. UTC | #2 
Hi Mat,

Thanks for the review.

On Mon, 2025-03-03 at 17:39 -0800, Mat Martineau wrote:
> On Mon, 3 Mar 2025, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Drop the NULL check for 'msk' as Martin suggested, add more checks
> > for 'sk'.
> > 
> > Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as
> > the
> > argument in the bpf_iter_mptcp_subflow_new as Martin suggested.
> > 
> > v5:
> > - check bpf_iter_task in mptcp_subflow_new()
> > 
> > v4:
> > - drop sock_owned_by_user_nocheck and spin_is_locked. According to
> >   comments from Mat [2] and Martin [1], in this set mptcp_subflow
> >   bpf_iter only used from a cg sockopt bpf prog, no need to add
> > these
> >   check at this moment.
> > 
> > [1]
> > https://lore.kernel.org/all/fdf0ddbe-e007-4a5f-bbdf-9a144e8fbe35@linux.dev/
> > [2]
> > https://patchwork.kernel.org/project/mptcp/patch/f6469225598beecbf0bda12a4c33fafa86c0ff15.1739787744.git.tanggeliang@kylinos.cn/
> > 
> > v3:
> > - continue to use sock_owned_by_user_nocheck and spin_is_locked
> > checks instead of using msk_owned_by_me().
> > 
> > v2:
> > - check the owner before assigning the msk as Mat suggested.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > net/mptcp/bpf.c | 21 +++++++++++++++------
> > 1 file changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> > index aff92f458e7f..a307490bb20e 100644
> > --- a/net/mptcp/bpf.c
> > +++ b/net/mptcp/bpf.c
> > @@ -249,24 +249,33 @@ bpf_mptcp_subflow_ctx(const struct sock *sk)
> > 
> > __bpf_kfunc static int
> > bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
> > -			   struct mptcp_sock *msk)
> > +			   struct sock *sk)
> > {
> > 	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> > -	struct sock *sk = (struct sock *)msk;
> > +	struct task_struct *task;
> > +	struct mptcp_sock *msk;
> > 
> > 	BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) >
> > 		     sizeof(struct bpf_iter_mptcp_subflow));
> > 	BUILD_BUG_ON(__alignof__(struct
> > bpf_iter_mptcp_subflow_kern) !=
> > 		     __alignof__(struct bpf_iter_mptcp_subflow));
> > 
> > -	kit->msk = msk;
> > -	if (!msk)
> > +	if (unlikely(!sk || !sk_fullsock(sk)))
> > 		return -EINVAL;
> > 
> > -	if (!sock_owned_by_user_nocheck(sk) &&
> > -	    !spin_is_locked(&sk->sk_lock.slock))
> > +	if (sk->sk_protocol != IPPROTO_MPTCP)
> > 		return -EINVAL;
> > 
> > +	msk = mptcp_sk(sk);
> > +	task = mptcp_get_bpf_iter_task(msk);
> > +	if (!task || task != current)
> > +		return -EINVAL;
> 
> Hi Geliang -
> 
> The task checking logic would fit better inside the helper, since
> every 
> caller will be checking to see if it matches 'current'.
> 
> I also think it would read better as (task && task == current)

Yes, indeed.

> 
> 
> > +
> > +	mptcp_clear_bpf_iter_task(msk);
> 
> This needs to be cleared where the lock is released, not where the
> task is 
> checked.

Yes, indeed, I'll do that in the next version.

Thanks,
-Geliang

> 
> - Mat
> 
> > +
> > +	msk_owned_by_me(msk);
> > +
> > +	kit->msk = msk;
> > 	kit->pos = &msk->conn_list;
> > 	return 0;
> > }
> > -- 
> > 2.43.0
> > 
> > 
> >
diff mbox series

Patch

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index aff92f458e7f..a307490bb20e 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -249,24 +249,33 @@  bpf_mptcp_subflow_ctx(const struct sock *sk)
 
 __bpf_kfunc static int
 bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
-			   struct mptcp_sock *msk)
+			   struct sock *sk)
 {
 	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
-	struct sock *sk = (struct sock *)msk;
+	struct task_struct *task;
+	struct mptcp_sock *msk;
 
 	BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) >
 		     sizeof(struct bpf_iter_mptcp_subflow));
 	BUILD_BUG_ON(__alignof__(struct bpf_iter_mptcp_subflow_kern) !=
 		     __alignof__(struct bpf_iter_mptcp_subflow));
 
-	kit->msk = msk;
-	if (!msk)
+	if (unlikely(!sk || !sk_fullsock(sk)))
 		return -EINVAL;
 
-	if (!sock_owned_by_user_nocheck(sk) &&
-	    !spin_is_locked(&sk->sk_lock.slock))
+	if (sk->sk_protocol != IPPROTO_MPTCP)
 		return -EINVAL;
 
+	msk = mptcp_sk(sk);
+	task = mptcp_get_bpf_iter_task(msk);
+	if (!task || task != current)
+		return -EINVAL;
+
+	mptcp_clear_bpf_iter_task(msk);
+
+	msk_owned_by_me(msk);
+
+	kit->msk = msk;
 	kit->pos = &msk->conn_list;
 	return 0;
 }