From: Geliang Tang <tanggeliang@kylinos.cn>

Add "sizeof" and "alignof" checks.

Address Martin's comments in v1:

- bpf_iter_mptcp_subflow_new returns -EINVAL when msk socket lock isn't
  held.

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

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index f9ba0a46a9f1..0563d3c6d9d3 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -232,12 +232,20 @@ bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
 			   struct mptcp_sock *msk)
 {
 	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
+	struct sock *sk = (struct 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)
 		return -EINVAL;
 
-	msk_owned_by_me(msk);
+	if (!sock_owned_by_user_nocheck(sk) &&
+	    !spin_is_locked(&sk->sk_lock.slock))
+		return -EINVAL;
 
 	kit->pos = &msk->conn_list;
 	return 0;
-- 
2.45.2

Hi Geliang,

On 09/12/2024 09:47, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Add "sizeof" and "alignof" checks.
> 
> Address Martin's comments in v1:
> 
> - bpf_iter_mptcp_subflow_new returns -EINVAL when msk socket lock isn't
>   held.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/bpf.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index f9ba0a46a9f1..0563d3c6d9d3 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -232,12 +232,20 @@ bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
>  			   struct mptcp_sock *msk)
>  {
>  	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> +	struct sock *sk = (struct 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)
>  		return -EINVAL;
>  
> -	msk_owned_by_me(msk);
> +	if (!sock_owned_by_user_nocheck(sk) &&
> +	    !spin_is_locked(&sk->sk_lock.slock))
> +		return -EINVAL;

Just to be sure, did you check it was always the case with the WIP
scheduler and path-manager BPF programs you have?

We don't want to be too restrictive. (Or we need different helpers)

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

Hi Matt,

On Tue, 2024-12-10 at 12:48 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 09/12/2024 09:47, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Add "sizeof" and "alignof" checks.
> > 
> > Address Martin's comments in v1:
> > 
> > - bpf_iter_mptcp_subflow_new returns -EINVAL when msk socket lock
> > isn't
> >   held.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/bpf.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> > index f9ba0a46a9f1..0563d3c6d9d3 100644
> > --- a/net/mptcp/bpf.c
> > +++ b/net/mptcp/bpf.c
> > @@ -232,12 +232,20 @@ bpf_iter_mptcp_subflow_new(struct
> > bpf_iter_mptcp_subflow *it,
> >  			   struct mptcp_sock *msk)
> >  {
> >  	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> > +	struct sock *sk = (struct 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)
> >  		return -EINVAL;
> >  
> > -	msk_owned_by_me(msk);
> > +	if (!sock_owned_by_user_nocheck(sk) &&
> > +	    !spin_is_locked(&sk->sk_lock.slock))
> > +		return -EINVAL;
> 
> Just to be sure, did you check it was always the case with the WIP
> scheduler and path-manager BPF programs you have?

Yes, I checked, BPF packet scheduler and BPF path manager are working
fine. Thanks for your other suggestions on this set, I have updated to
the new version v5.

-Geliang

> 
> We don't want to be too restrictive. (Or we need different helpers)
> 
> Cheers,
> Matt