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.3Hi 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.