1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v8 0/5] update userspace pm mptcp_info fields, pt 1
  4. View patch

[PATCH mptcp-next v8 3/5] mptcp: add addr into userspace pm list

Geliang Tang posted 5 patches 2 years, 9 months ago
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Kishen Maloor <kishen.maloor@intel.com>
There is a newer version of this series
[PATCH mptcp-next v8 3/5] mptcp: add addr into userspace pm list
Posted by Geliang Tang 2 years, 9 months ago 
Add the address into userspace_pm_local_addr_list when the subflow is
created. And delete it in mptcp_nl_cmd_sf_destroy().

A non-zero address id is needed in this case. So don't clear the addr
id in mptcp_userspace_pm_get_local_id(), clear it in
mptcp_pm_nl_get_local_id() instead.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_netlink.c   |  2 +-
 net/mptcp/pm_userspace.c | 19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d85649bc27e2..bb237abb99bb 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1055,8 +1055,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 
 int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 {
+	struct mptcp_addr_info skc_local = { 0 };
 	struct mptcp_pm_addr_entry *entry;
-	struct mptcp_addr_info skc_local;
 	struct mptcp_addr_info msk_local;
 	struct pm_nl_pernet *pernet;
 	int ret = -1;
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6beadea8c67d..a1f8d2fab08d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -113,7 +113,6 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
 
 	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)
@@ -302,6 +301,12 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 		goto create_err;
 	}
 
+	err = mptcp_userspace_pm_get_local_id(msk, &addr_l);
+	if (err < 0) {
+		GENL_SET_ERR_MSG(info, "did not match address and id");
+		goto create_err;
+	}
+
 	lock_sock(sk);
 
 	err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
@@ -420,6 +425,18 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
 	if (ssk) {
 		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+		struct mptcp_pm_addr_entry *entry, *tmp;
+
+		spin_lock_bh(&msk->pm.lock);
+		list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
+			if (mptcp_addresses_equal(&entry->addr, &addr_l, false) &&
+			    msk->pm.subflows == 1) {
+				list_del_rcu(&entry->list);
+				kfree(entry);
+				break;
+			}
+		}
+		spin_unlock_bh(&msk->pm.lock);
 
 		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 		mptcp_close_ssk(sk, ssk, subflow);
-- 
2.35.3
Re: [PATCH mptcp-next v8 3/5] mptcp: add addr into userspace pm list
Posted by Matthieu Baerts 2 years, 9 months ago 
Hi Geliang,

On 20/04/2023 05:11, Geliang Tang wrote:
> Add the address into userspace_pm_local_addr_list when the subflow is
> created. And delete it in mptcp_nl_cmd_sf_destroy().

I'm sorry to insist but can you explain the reason(s) why you need to
add addresses into the list? Is it to be able to send a RM_ADDR for a
previously used subflow?

  By doing that, the "REMOVE" command also works with subflows that have
  been created via the "SUB_CREATE" command instead of restricting to
  the addresses that have been announced via the "ANNOUNCE" command.

  (...)

  Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
  Link: https://github.com/multipath-tcp/mptcp_net-next/issues/379

> A non-zero address id is needed in this case. So don't clear the addr
> id in mptcp_userspace_pm_get_local_id(), clear it in
> mptcp_pm_nl_get_local_id() instead.

> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_netlink.c   |  2 +-
>  net/mptcp/pm_userspace.c | 19 ++++++++++++++++++-
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d85649bc27e2..bb237abb99bb 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1055,8 +1055,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  
>  int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  {
> +	struct mptcp_addr_info skc_local = { 0 };
>  	struct mptcp_pm_addr_entry *entry;
> -	struct mptcp_addr_info skc_local;
>  	struct mptcp_addr_info msk_local;
>  	struct pm_nl_pernet *pernet;
>  	int ret = -1;
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6beadea8c67d..a1f8d2fab08d 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -113,7 +113,6 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
>  
>  	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)
> @@ -302,6 +301,12 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
>  		goto create_err;
>  	}
>  
> +	err = mptcp_userspace_pm_get_local_id(msk, &addr_l);

The name is not very clear: It adds the address into
userspace_pm_local_addr_list, right?
If yes, please add a comment above (or rename the function).

Why can you not call mptcp_userspace_pm_append_new_local_addr() directly?

> +	if (err < 0) {
> +		GENL_SET_ERR_MSG(info, "did not match address and id");
> +		goto create_err;
> +	}
> +
>  	lock_sock(sk);
>  
>  	err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);

In case of error, I guess you should remove the entry from the list, no?

And when the subflow is deleted from the other side, no?

> @@ -420,6 +425,18 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
>  	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
>  	if (ssk) {
>  		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +		struct mptcp_pm_addr_entry *entry, *tmp;
> +
> +		spin_lock_bh(&msk->pm.lock);
> +		list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> +			if (mptcp_addresses_equal(&entry->addr, &addr_l, false) &&
> +			    msk->pm.subflows == 1) {

Why did you add "msk->pm.subflows == 1"? It looks like a workaround but
not a proper solution :)

Should you not instead add a refcount in "struct mptcp_pm_addr_entry"?

> +				list_del_rcu(&entry->list);
> +				kfree(entry);
> +				break;
> +			}
> +		}
> +		spin_unlock_bh(&msk->pm.lock);
>  
>  		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
>  		mptcp_close_ssk(sk, ssk, subflow);

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v8 3/5] mptcp: add addr into userspace pm list
Posted by Geliang Tang 2 years, 9 months ago 
Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年4月21日周五 22:17写道：
>
> Hi Geliang,
>
> On 20/04/2023 05:11, Geliang Tang wrote:
> > Add the address into userspace_pm_local_addr_list when the subflow is
> > created. And delete it in mptcp_nl_cmd_sf_destroy().
>
> I'm sorry to insist but can you explain the reason(s) why you need to
> add addresses into the list? Is it to be able to send a RM_ADDR for a
> previously used subflow?
>
>   By doing that, the "REMOVE" command also works with subflows that have
>   been created via the "SUB_CREATE" command instead of restricting to
>   the addresses that have been announced via the "ANNOUNCE" command.
>
>   (...)
>
>   Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
>   Link: https://github.com/multipath-tcp/mptcp_net-next/issues/379

Updated in v9.

>
> > A non-zero address id is needed in this case. So don't clear the addr
> > id in mptcp_userspace_pm_get_local_id(), clear it in
> > mptcp_pm_nl_get_local_id() instead.
>
> >
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  net/mptcp/pm_netlink.c   |  2 +-
> >  net/mptcp/pm_userspace.c | 19 ++++++++++++++++++-
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index d85649bc27e2..bb237abb99bb 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -1055,8 +1055,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
> >
> >  int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
> >  {
> > +     struct mptcp_addr_info skc_local = { 0 };
> >       struct mptcp_pm_addr_entry *entry;
> > -     struct mptcp_addr_info skc_local;
> >       struct mptcp_addr_info msk_local;
> >       struct pm_nl_pernet *pernet;
> >       int ret = -1;
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index 6beadea8c67d..a1f8d2fab08d 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -113,7 +113,6 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
> >
> >       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)
> > @@ -302,6 +301,12 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
> >               goto create_err;
> >       }
> >
> > +     err = mptcp_userspace_pm_get_local_id(msk, &addr_l);
>
> The name is not very clear: It adds the address into
> userspace_pm_local_addr_list, right?
> If yes, please add a comment above (or rename the function).
>
> Why can you not call mptcp_userspace_pm_append_new_local_addr() directly?

Yes, mptcp_userspace_pm_append_new_local_addr is much better. Updated in v9.

>
> > +     if (err < 0) {
> > +             GENL_SET_ERR_MSG(info, "did not match address and id");
> > +             goto create_err;
> > +     }
> > +
> >       lock_sock(sk);
> >
> >       err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
>
> In case of error, I guess you should remove the entry from the list, no?
>
> And when the subflow is deleted from the other side, no?

Updated in v9.

>
> > @@ -420,6 +425,18 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
> >       ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
> >       if (ssk) {
> >               struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> > +             struct mptcp_pm_addr_entry *entry, *tmp;
> > +
> > +             spin_lock_bh(&msk->pm.lock);
> > +             list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> > +                     if (mptcp_addresses_equal(&entry->addr, &addr_l, false) &&
> > +                         msk->pm.subflows == 1) {
>
> Why did you add "msk->pm.subflows == 1"? It looks like a workaround but
> not a proper solution :)
>
> Should you not instead add a refcount in "struct mptcp_pm_addr_entry"?

I still use this workaround in v9. Let's add the recount in future.

>
> > +                             list_del_rcu(&entry->list);
> > +                             kfree(entry);
> > +                             break;
> > +                     }
> > +             }
> > +             spin_unlock_bh(&msk->pm.lock);
> >
> >               mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
> >               mptcp_close_ssk(sk, ssk, subflow);
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.