diff mbox series

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

Message ID af7dca496d83aaf6fd7163f767ef1bd1abbab59a.1737714424.git.tanggeliang@kylinos.cn (mailing list archive)
State Changes Requested
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 success total: 0 errors, 0 warnings, 0 checks, 33 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 Jan. 24, 2025, 10:28 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

Drop the NULL check as Martin suggested.

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

Use msk_owned_by_me().

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

Comments

Matthieu Baerts Jan. 24, 2025, 3:04 p.m. UTC | #1 
Hi Geliang,

(+cc Mat who reviewed the previous versions)

On 24/01/2025 11:28, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Drop the NULL check as Martin suggested.
> 
> Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as the
> argument in the bpf_iter_mptcp_subflow_new as Martin suggested.
> 
> Use msk_owned_by_me().

Regarding this, do you mind checking my question on the thread initially
sent to the BPF mailing please?

>> This set is only showing the cg sockopt bpf prog but missing the major
>> struct_ops piece. It is hard to comment. I assumed the lock situation is
>> the same for the struct_ops where the lock will be held before calling
>> the struct_ops prog?
> 
> Can we restrict the use of this helper only for some specific callbacks?
> Or for only some specific struct_ops?
> 
> Last time I looked, I thought that you could only restrict it like that:
> "this helper can be used by (any) struct_ops". But then does it mean
> this helper could be called from a TCP BPF CC ops or another unrelated
> one for example? How can we avoid that?
Because with the suggested modification, there is no more checks
regarding the lock situation: it is probably fine now with
[gs]etsockopt(), but what about later with struct_ops? Can we restrict
this kfunc to be used only in some contexts? e.g. a part or all the
MPTCP struct_ops, but not the other ones?

Cheers,
Matt
Mat Martineau Jan. 29, 2025, 8:49 p.m. UTC | #2 
On Fri, 24 Jan 2025, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Drop the NULL check as Martin suggested.
>
> Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as the
> argument in the bpf_iter_mptcp_subflow_new as Martin suggested.
>
> Use msk_owned_by_me().
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> net/mptcp/bpf.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 923895322b2c..def4de34666d 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -234,24 +234,27 @@ 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 (!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);
> +	kit->msk = msk;
> +
> +	msk_owned_by_me(msk);

Hi Geliang -

The convention is to check the owner before assigning the msk.

I also noticed in the reply to patch 5 that Martin mentioned adding a 
"bpf_mptcp_sock_from_sock check" to bpf_iter_mptcp_subflow_new 
(https://lore.kernel.org/netdev/9b373a23-c093-42d8-b4ae-99f2e62e7681@linux.dev/)

- Mat

> +
> 	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 923895322b2c..def4de34666d 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -234,24 +234,27 @@  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 (!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);
+	kit->msk = msk;
+
+	msk_owned_by_me(msk);
+
 	kit->pos = &msk->conn_list;
 	return 0;
 }