Add the address into userspace_pm_local_addr_list when the subflow is
created. Make sure it can be found in mptcp_nl_cmd_remove(). And delete
it in the new helper mptcp_userspace_pm_delete_local_addr().
Add address into pm anno_list in mptcp_nl_cmd_sf_create(). Remove
it when connecting fails.
By doing this, 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
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
net/mptcp/pm_userspace.c | 51 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6beadea8c67d..c50e1507ae35 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -79,6 +79,24 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
return ret;
}
+static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
+ struct mptcp_pm_addr_entry *addr)
+{
+ struct mptcp_pm_addr_entry *entry, *tmp;
+
+ list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
+ if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
+ /* TODO: a refcount is needed because the entry can
+ * be used multiple times (e.g. fullmesh mode). */
+ list_del_rcu(&entry->list);
+ kfree(entry);
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}
+
int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
unsigned int id,
u8 *flags, int *ifindex)
@@ -251,6 +269,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
+ struct mptcp_pm_addr_entry local = { 0 };
struct mptcp_addr_info addr_r;
struct mptcp_addr_info addr_l;
struct mptcp_sock *msk;
@@ -302,12 +321,40 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
goto create_err;
}
+ local.addr = addr_l;
+ err = mptcp_userspace_pm_append_new_local_addr(msk, &local);
+ if (err < 0) {
+ GENL_SET_ERR_MSG(info, "did not match address and id");
+ goto create_err;
+ }
+
+ spin_lock_bh(&msk->pm.lock);
+ if (!mptcp_pm_alloc_anno_list(msk, &local)) {
+ mptcp_userspace_pm_delete_local_addr(msk, &local);
+ spin_unlock_bh(&msk->pm.lock);
+ goto create_err;
+ }
+ spin_unlock_bh(&msk->pm.lock);
+
lock_sock(sk);
err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
release_sock(sk);
+ spin_lock_bh(&msk->pm.lock);
+ if (err) {
+ mptcp_pm_remove_anno_list_by_saddr(msk, &addr_l);
+ mptcp_userspace_pm_delete_local_addr(msk, &local);
+ }
+ spin_unlock_bh(&msk->pm.lock);
+
+ /* If the subflow is closed from the other peer (not via a
+ * subflow destroy command then), we want to keep the entry
+ * not to assign the same ID to another address and to be
+ * able to send RM_ADDR after the removal of the subflow.
+ */
+
create_err:
sock_put((struct sock *)msk);
return err;
@@ -420,10 +467,14 @@ 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 = { .addr = addr_l };
mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
mptcp_close_ssk(sk, ssk, subflow);
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
+ spin_lock_bh(&msk->pm.lock);
+ mptcp_userspace_pm_delete_local_addr(msk, &entry);
+ spin_unlock_bh(&msk->pm.lock);
err = 0;
} else {
err = -ESRCH;
--
2.35.3Hi Geliang,
On 04/05/2023 12:20, Geliang Tang wrote:
> Add the address into userspace_pm_local_addr_list when the subflow is
> created. Make sure it can be found in mptcp_nl_cmd_remove(). And delete
> it in the new helper mptcp_userspace_pm_delete_local_addr().
>
> Add address into pm anno_list in mptcp_nl_cmd_sf_create(). Remove
> it when connecting fails.
>
> By doing this, 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
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/pm_userspace.c | 51 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6beadea8c67d..c50e1507ae35 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -79,6 +79,24 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
> return ret;
> }
>
> +static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
> + struct mptcp_pm_addr_entry *addr)
> +{
> + struct mptcp_pm_addr_entry *entry, *tmp;
> +
> + list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> + if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
> + /* TODO: a refcount is needed because the entry can
> + * be used multiple times (e.g. fullmesh mode). */
Thank you for the new comment.
Just to know if we need to create a new ticket: are you already looking
at that or you don't plan to? Do you prefer if I create a new ticket?
> + list_del_rcu(&entry->list);
> + kfree(entry);
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> unsigned int id,
> u8 *flags, int *ifindex)
> @@ -251,6 +269,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
> struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
> struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
> + struct mptcp_pm_addr_entry local = { 0 };
> struct mptcp_addr_info addr_r;
> struct mptcp_addr_info addr_l;
> struct mptcp_sock *msk;
> @@ -302,12 +321,40 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
> goto create_err;
> }
>
> + local.addr = addr_l;
> + err = mptcp_userspace_pm_append_new_local_addr(msk, &local);
> + if (err < 0) {
> + GENL_SET_ERR_MSG(info, "did not match address and id");
> + goto create_err;
> + }
> +
> + spin_lock_bh(&msk->pm.lock);
> + if (!mptcp_pm_alloc_anno_list(msk, &local)) {
> + mptcp_userspace_pm_delete_local_addr(msk, &local);
> + spin_unlock_bh(&msk->pm.lock);
> + goto create_err;
Here, you need to set err to a negative value, e.g. -EINVAL. And
probably good to send an NL error message as well, like above.
if (!mptcp_pm_alloc_anno_list(msk, &local)) {
GENL_SET_ERR_MSG(info, "cannot alloc address list");
err = -EINVAL;
goto alloc_anno_list_fail;
}
While at it, it might be good to use a new label (alloc_anno_list_fail),
so you don't have to clean and unlock again, see below.
> + }
> + spin_unlock_bh(&msk->pm.lock);
> +
> lock_sock(sk);
>
> err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
>
> release_sock(sk);
>
> + spin_lock_bh(&msk->pm.lock);
> + if (err) {
Maybe better to put the spin_lock inside the 'if (err)' section, no?
(i.e. no need to lock if there is no error)
> + mptcp_pm_remove_anno_list_by_saddr(msk, &addr_l);
It might be good to add a new label here (e.g. "alloc_anno_list_fail"),
so we can go here in case of error with mptcp_pm_alloc_anno_list() above.
> + mptcp_userspace_pm_delete_local_addr(msk, &local);
> + }
> + spin_unlock_bh(&msk->pm.lock);
> +
> + /* If the subflow is closed from the other peer (not via a
> + * subflow destroy command then), we want to keep the entry
> + * not to assign the same ID to another address and to be
> + * able to send RM_ADDR after the removal of the subflow.
> + */
(maybe good to add this comment just above the declaration of the
mptcp_userspace_pm_delete_local_addr() function?)
> +
> create_err:
> sock_put((struct sock *)msk);
> return err;
> @@ -420,10 +467,14 @@ 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 = { .addr = addr_l };
>
> mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
> mptcp_close_ssk(sk, ssk, subflow);
> MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
(maybe keep this operation with the MIBs as the last step as we
generally do?)
> + spin_lock_bh(&msk->pm.lock);
> + mptcp_userspace_pm_delete_local_addr(msk, &entry);
Why do you not need to call mptcp_pm_remove_anno_list_by_saddr() here?
> + spin_unlock_bh(&msk->pm.lock);
> err = 0;
> } else {
> err = -ESRCH;
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Hi Geliang,
Sorry for the delay to review this series (due to some holidays).
On 04/05/2023 16:59, Matthieu Baerts wrote:
> Hi Geliang,
>
> On 04/05/2023 12:20, Geliang Tang wrote:
>> Add the address into userspace_pm_local_addr_list when the subflow is
>> created. Make sure it can be found in mptcp_nl_cmd_remove(). And delete
>> it in the new helper mptcp_userspace_pm_delete_local_addr().
>>
>> Add address into pm anno_list in mptcp_nl_cmd_sf_create(). Remove
>> it when connecting fails.
>>
>> By doing this, 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
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>> ---
>> net/mptcp/pm_userspace.c | 51 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 51 insertions(+)
>>
>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index 6beadea8c67d..c50e1507ae35 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c
>> @@ -79,6 +79,24 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>> return ret;
>> }
>>
>> +static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>> + struct mptcp_pm_addr_entry *addr)
>> +{
>> + struct mptcp_pm_addr_entry *entry, *tmp;
>> +
>> + list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
>> + if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
>> + /* TODO: a refcount is needed because the entry can
>> + * be used multiple times (e.g. fullmesh mode). */
>
> Thank you for the new comment.
>
> Just to know if we need to create a new ticket: are you already looking
> at that or you don't plan to? Do you prefer if I create a new ticket?
FYI, I just created a new ticket not to forget about that:
https://github.com/multipath-tcp/mptcp_net-next/issues/403
I was not sure if you were planning to look at it or not but at least we
can track this now.
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
On Thu, May 04, 2023 at 06:20:09PM +0800, Geliang Tang wrote:
> Add the address into userspace_pm_local_addr_list when the subflow is
> created. Make sure it can be found in mptcp_nl_cmd_remove(). And delete
> it in the new helper mptcp_userspace_pm_delete_local_addr().
>
> Add address into pm anno_list in mptcp_nl_cmd_sf_create(). Remove
> it when connecting fails.
>
> By doing this, 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
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/pm_userspace.c | 51 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6beadea8c67d..c50e1507ae35 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -79,6 +79,24 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
> return ret;
> }
>
> +static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
> + struct mptcp_pm_addr_entry *addr)
> +{
> + struct mptcp_pm_addr_entry *entry, *tmp;
> +
> + list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> + if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
> + /* TODO: a refcount is needed because the entry can
> + * be used multiple times (e.g. fullmesh mode). */
CI reported a checkpatch warning here. It should be:
* be used multiple times (e.g. fullmesh mode).
*/
> + list_del_rcu(&entry->list);
> + kfree(entry);
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> unsigned int id,
> u8 *flags, int *ifindex)
> @@ -251,6 +269,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
> struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
> struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
> + struct mptcp_pm_addr_entry local = { 0 };
> struct mptcp_addr_info addr_r;
> struct mptcp_addr_info addr_l;
> struct mptcp_sock *msk;
> @@ -302,12 +321,40 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
> goto create_err;
> }
>
> + local.addr = addr_l;
> + err = mptcp_userspace_pm_append_new_local_addr(msk, &local);
> + if (err < 0) {
> + GENL_SET_ERR_MSG(info, "did not match address and id");
> + goto create_err;
> + }
> +
> + spin_lock_bh(&msk->pm.lock);
> + if (!mptcp_pm_alloc_anno_list(msk, &local)) {
> + mptcp_userspace_pm_delete_local_addr(msk, &local);
> + spin_unlock_bh(&msk->pm.lock);
> + goto create_err;
> + }
> + spin_unlock_bh(&msk->pm.lock);
> +
> lock_sock(sk);
>
> err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
>
> release_sock(sk);
>
> + spin_lock_bh(&msk->pm.lock);
> + if (err) {
> + mptcp_pm_remove_anno_list_by_saddr(msk, &addr_l);
> + mptcp_userspace_pm_delete_local_addr(msk, &local);
> + }
> + spin_unlock_bh(&msk->pm.lock);
> +
> + /* If the subflow is closed from the other peer (not via a
> + * subflow destroy command then), we want to keep the entry
> + * not to assign the same ID to another address and to be
> + * able to send RM_ADDR after the removal of the subflow.
> + */
> +
> create_err:
> sock_put((struct sock *)msk);
> return err;
> @@ -420,10 +467,14 @@ 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 = { .addr = addr_l };
>
> mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
> mptcp_close_ssk(sk, ssk, subflow);
> MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
> + spin_lock_bh(&msk->pm.lock);
> + mptcp_userspace_pm_delete_local_addr(msk, &entry);
> + spin_unlock_bh(&msk->pm.lock);
> err = 0;
> } else {
> err = -ESRCH;
> --
> 2.35.3
>
© 2016 - 2024 Red Hat, Inc.