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
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
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 >
© 2016 - 2024 Red Hat, Inc.