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.

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 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 7e9d9c9a04cf..06fb0f88e35f 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -235,24 +235,28 @@ 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 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 (sk->sk_protocol != IPPROTO_MPTCP)
 		return -EINVAL;
 
 	if (!sock_owned_by_user_nocheck(sk) &&
 	    !spin_is_locked(&sk->sk_lock.slock))
 		return -EINVAL;
 
+	msk = mptcp_sk(sk);
+	kit->msk = msk;
 	kit->pos = &msk->conn_list;
 	return 0;
 }
-- 
2.43.0

Hi Geliang,

On 17/02/2025 11:34, 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.
> 
> v3:
>  - continue to use sock_owned_by_user_nocheck and spin_is_locked
> checks instead of using msk_owned_by_me().

Why should we continue to do so?

From what I understood, with the current version, this new
bpf_iter_mptcp_subflow_new kfunc can only be called from a cg sockopt
bpf prog, which means these checks are not needed for the moment, no?

So maybe we should only add these checks later, when extending its usage
to struct_ops? In the meantime, msk_owned_by_me() could then be used
instead?

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

Hi Mat,

Thanks for the review.

On Thu, 2025-02-20 at 19:25 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 17/02/2025 11:34, 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.
> > 
> > v3:
> >  - continue to use sock_owned_by_user_nocheck and spin_is_locked
> > checks instead of using msk_owned_by_me().
> 
> Why should we continue to do so?
> 
> From what I understood, with the current version, this new
> bpf_iter_mptcp_subflow_new kfunc can only be called from a cg sockopt
> bpf prog, which means these checks are not needed for the moment, no?

Thanks for the clarification. I re-read Martin's comments and he also
suggested removing these checks. So I removed it in v4.

> 
> So maybe we should only add these checks later, when extending its
> usage
> to struct_ops? In the meantime, msk_owned_by_me() could then be used
> instead?

Do I need to add a patch into the "use bpf_iter in bpf schedulers" set
that is under review to do these checks? Do you mean that using
msk_owned_by_me() is enough for struct_ops, and there is no need to use
sock_owned_by_user_nocheck() and spin_is_locked() checks?

Thanks,
-Geliang

> 
> Cheers,
> Matt

Hi Geliang,

On 24/02/2025 09:31, Geliang Tang wrote:
> Hi Mat,
> 
> Thanks for the review.
> 
> On Thu, 2025-02-20 at 19:25 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 17/02/2025 11:34, 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.
>>>
>>> v3:
>>>  - continue to use sock_owned_by_user_nocheck and spin_is_locked
>>> checks instead of using msk_owned_by_me().
>>
>> Why should we continue to do so?
>>
>> From what I understood, with the current version, this new
>> bpf_iter_mptcp_subflow_new kfunc can only be called from a cg sockopt
>> bpf prog, which means these checks are not needed for the moment, no?
> 
> Thanks for the clarification. I re-read Martin's comments and he also
> suggested removing these checks. So I removed it in v4.

Thanks!

>> So maybe we should only add these checks later, when extending its
>> usage
>> to struct_ops? In the meantime, msk_owned_by_me() could then be used
>> instead?
> 
> Do I need to add a patch into the "use bpf_iter in bpf schedulers" set
> that is under review to do these checks? Do you mean that using
> msk_owned_by_me() is enough for struct_ops, and there is no need to use
> sock_owned_by_user_nocheck() and spin_is_locked() checks?

My understanding is that if a kfunc is set to used in 'struct_ops', it
can be used with any 'struct_ops', and not a specific one. Is this correct?

- If yes, it means we don't really know in which context a kfunc will be
used, thus we need additional checks. But... this feels wrong: if
kfunc's cannot be restricted to specific 'struct_ops', probably this
should be reported to the BPF maintainers, and see if it could be
possible to have such restriction not to add unneeded checks at run time.

- If the MPTCP kfunc can be restricted to MPTCP 'struct_ops' where we
know the msk lock will be taken, then no need to add these checks if
they are useless.

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