diff mbox series

[mptcp-next,v2,21/36] mptcp: update address type of get_local_id

Message ID 697ad8ffc8987b12a232b3855e6960b49c186d4b.1729588019.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded, archived
Delegated to: Matthieu Baerts
Headers show
Series BPF path manager | expand

Checks

Context Check Description
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 95 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build warning Build error with: make C=1 net/mptcp/bpf.o
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 Oct. 22, 2024, 9:14 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

The following code in mptcp_userspace_pm_get_local_id() that assigns "skc"
to "new_entry" is not allowed in BPF if we use the same code to implement
the get_local_id() interface of a BFP path manager:

	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
	new_entry.addr = *skc;
	new_entry.addr.id = 0;
	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;

To solve the issue, this patch moves this assignment to "new_entry" forward
to mptcp_pm_get_local_id(), and then passing "new_entry" as a parameter to
both mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           | 10 ++++++++--
 net/mptcp/pm_netlink.c   | 11 +++--------
 net/mptcp/pm_userspace.c | 17 ++++++-----------
 net/mptcp/protocol.h     |  4 ++--
 4 files changed, 19 insertions(+), 23 deletions(-)

Comments

Matthieu Baerts Nov. 4, 2024, 6:48 p.m. UTC | #1 
Hi Geliang,

On 22/10/2024 11:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The following code in mptcp_userspace_pm_get_local_id() that assigns "skc"
> to "new_entry" is not allowed in BPF if we use the same code to implement
> the get_local_id() interface of a BFP path manager:

Can you give more details about the fact it is not allowed please?

Do you mean you cannot do the following code in BPF? Why?

> 	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
> 	new_entry.addr = *skc;
> 	new_entry.addr.id = 0;
> 	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
> 
> To solve the issue, this patch moves this assignment to "new_entry" forward
> to mptcp_pm_get_local_id(), and then passing "new_entry" as a parameter to
> both mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c           | 10 ++++++++--
>  net/mptcp/pm_netlink.c   | 11 +++--------
>  net/mptcp/pm_userspace.c | 17 ++++++-----------
>  net/mptcp/protocol.h     |  4 ++--
>  4 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index b6e6859903ef..d28e844eba2d 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -404,6 +404,7 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  {
>  	struct mptcp_addr_info skc_local;
>  	struct mptcp_addr_info msk_local;
> +	struct mptcp_pm_addr_entry local;
>  
>  	if (WARN_ON_ONCE(!msk))
>  		return -1;
> @@ -416,9 +417,14 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
>  		return 0;
>  
> +	memset(&local, 0, sizeof(struct mptcp_pm_addr_entry));

Out of curiosity: do you need to call memset() here? Can you not init
"local" when you declare it?

  struct mptcp_pm_addr_entry local = { 0 };

(Or maybe that's not working with C99 with inner structures? I thought
it was, but not sure).

> +	local.addr = skc_local;

Do you still need skc_local? Can you not use 'local.addr' directly
instead above?

> +	local.addr.id = 0;

In pm_netlink.c, 'addr.port' is also reset. Should you not do that here
too? I guess that's fine for the userspace pm, no?

> +	local.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
> +
>  	if (mptcp_pm_is_userspace(msk))
> -		return mptcp_userspace_pm_get_local_id(msk, &skc_local);
> -	return mptcp_pm_nl_get_local_id(msk, &skc_local);
> +		return mptcp_userspace_pm_get_local_id(msk, &local);
> +	return mptcp_pm_nl_get_local_id(msk, &local);
>  }
>  
>  bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
(...)

Cheers,
Matt
Geliang Tang Nov. 7, 2024, 7:35 a.m. UTC | #2 
Hi Matt,

On Mon, 2024-11-04 at 19:48 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/10/2024 11:14, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > The following code in mptcp_userspace_pm_get_local_id() that
> > assigns "skc"
> > to "new_entry" is not allowed in BPF if we use the same code to
> > implement
> > the get_local_id() interface of a BFP path manager:
> 
> Can you give more details about the fact it is not allowed please?
> 
> Do you mean you cannot do the following code in BPF? Why?

Same as in patch 23, passing "new_entry" to
mptcp_userspace_pm_append_new_local_addr() will get an error:

"pointer type STRUCT mptcp_pm_addr_entry must point to scalar, or
struct with scalar".

And assigning an address to an address like this is not allowed in
BPF:

	new_entry.addr = *skc;

> 
> > 	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
> > 	new_entry.addr = *skc;
> > 	new_entry.addr.id = 0;
> > 	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
> > 
> > To solve the issue, this patch moves this assignment to "new_entry"
> > forward
> > to mptcp_pm_get_local_id(), and then passing "new_entry" as a
> > parameter to
> > both mptcp_pm_nl_get_local_id() and
> > mptcp_userspace_pm_get_local_id().
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/pm.c           | 10 ++++++++--
> >  net/mptcp/pm_netlink.c   | 11 +++--------
> >  net/mptcp/pm_userspace.c | 17 ++++++-----------
> >  net/mptcp/protocol.h     |  4 ++--
> >  4 files changed, 19 insertions(+), 23 deletions(-)
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index b6e6859903ef..d28e844eba2d 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -404,6 +404,7 @@ int mptcp_pm_get_local_id(struct mptcp_sock
> > *msk, struct sock_common *skc)
> >  {
> >  	struct mptcp_addr_info skc_local;
> >  	struct mptcp_addr_info msk_local;
> > +	struct mptcp_pm_addr_entry local;
> >  
> >  	if (WARN_ON_ONCE(!msk))
> >  		return -1;
> > @@ -416,9 +417,14 @@ int mptcp_pm_get_local_id(struct mptcp_sock
> > *msk, struct sock_common *skc)
> >  	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
> >  		return 0;
> >  
> > +	memset(&local, 0, sizeof(struct mptcp_pm_addr_entry));
> 
> Out of curiosity: do you need to call memset() here? Can you not init
> "local" when you declare it?
> 
>   struct mptcp_pm_addr_entry local = { 0 };

Updated in v3.

> 
> (Or maybe that's not working with C99 with inner structures? I
> thought
> it was, but not sure).
> 
> > +	local.addr = skc_local;
> 
> Do you still need skc_local? Can you not use 'local.addr' directly
> instead above?
> 
> > +	local.addr.id = 0;
> 
> In pm_netlink.c, 'addr.port' is also reset. Should you not do that
> here
> too? I guess that's fine for the userspace pm, no?

Updated in v3.

Thanks,
-Geliang

> 
> > +	local.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
> > +
> >  	if (mptcp_pm_is_userspace(msk))
> > -		return mptcp_userspace_pm_get_local_id(msk,
> > &skc_local);
> > -	return mptcp_pm_nl_get_local_id(msk, &skc_local);
> > +		return mptcp_userspace_pm_get_local_id(msk,
> > &local);
> > +	return mptcp_pm_nl_get_local_id(msk, &local);
> >  }
> >  
> >  bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common
> > *skc)
> (...)
> 
> Cheers,
> Matt
diff mbox series

Patch

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index b6e6859903ef..d28e844eba2d 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -404,6 +404,7 @@  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 {
 	struct mptcp_addr_info skc_local;
 	struct mptcp_addr_info msk_local;
+	struct mptcp_pm_addr_entry local;
 
 	if (WARN_ON_ONCE(!msk))
 		return -1;
@@ -416,9 +417,14 @@  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
 		return 0;
 
+	memset(&local, 0, sizeof(struct mptcp_pm_addr_entry));
+	local.addr = skc_local;
+	local.addr.id = 0;
+	local.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
+
 	if (mptcp_pm_is_userspace(msk))
-		return mptcp_userspace_pm_get_local_id(msk, &skc_local);
-	return mptcp_pm_nl_get_local_id(msk, &skc_local);
+		return mptcp_userspace_pm_get_local_id(msk, &local);
+	return mptcp_pm_nl_get_local_id(msk, &local);
 }
 
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 5cd5241da3c6..032c9eb2e48d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1137,7 +1137,7 @@  static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	return err;
 }
 
-int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
+int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_pm_addr_entry *local)
 {
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
@@ -1146,7 +1146,7 @@  int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	pernet = pm_nl_get_pernet_from_msk(msk);
 
 	rcu_read_lock();
-	entry = __lookup_addr(pernet, skc);
+	entry = __lookup_addr(pernet, &local->addr);
 	if (entry)
 		ret = entry->addr.id;
 	rcu_read_unlock();
@@ -1158,12 +1158,7 @@  int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	if (!entry)
 		return -ENOMEM;
 
-	entry->addr = *skc;
-	entry->addr.id = 0;
-	entry->addr.port = 0;
-	entry->ifindex = 0;
-	entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
-	entry->lsk = NULL;
+	*entry = *local;
 	ret = mptcp_pm_nl_append_new_local_addr(pernet, entry, true);
 	if (ret < 0)
 		kfree(entry);
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index aa4a5d110376..c749f5dccdf9 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -131,27 +131,22 @@  mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
 }
 
 int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
-				    struct mptcp_addr_info *skc)
+				    struct mptcp_pm_addr_entry *local)
 {
-	struct mptcp_pm_addr_entry *entry = NULL, new_entry;
+	struct mptcp_pm_addr_entry *entry = NULL;
 	__be16 msk_sport =  ((struct inet_sock *)
 			     inet_sk((struct sock *)msk))->inet_sport;
 
 	spin_lock_bh(&msk->pm.lock);
-	entry = mptcp_userspace_pm_lookup_addr(msk, skc);
+	entry = mptcp_userspace_pm_lookup_addr(msk, &local->addr);
 	spin_unlock_bh(&msk->pm.lock);
 	if (entry)
 		return entry->addr.id;
 
-	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
-	new_entry.addr = *skc;
-	new_entry.addr.id = 0;
-	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
-
-	if (new_entry.addr.port == msk_sport)
-		new_entry.addr.port = 0;
+	if (local->addr.port == msk_sport)
+		local->addr.port = 0;
 
-	return mptcp_userspace_pm_append_new_local_addr(msk, &new_entry, true);
+	return mptcp_userspace_pm_append_new_local_addr(msk, local, true);
 }
 
 bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 25d2d3def17a..4ff6b7e37947 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1127,8 +1127,8 @@  bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, const struct sk_buff *skb,
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
-int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
-int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
+int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_pm_addr_entry *local);
+int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_pm_addr_entry *local);
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc);
 bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
 bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);