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;
+
+ mptcp_clear_bpf_iter_task(msk);
+
+ msk_owned_by_me(msk);
+
+ kit->msk = msk;
kit->pos = &msk->conn_list;
return 0;
}
--
2.43.0On 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
>
>
>
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
> >
> >
> >
© 2016 - 2025 Red Hat, Inc.