On 25/04/2023 09:55, Geliang Tang wrote:
> Add the address into userspace_pm_local_addr_list when the subflow is
> created. And delete it in the new helper
> mptcp_userspace_pm_delete_local_addr().
>
> 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
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/pm_userspace.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6beadea8c67d..b494c72efe2b 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -79,6 +79,23 @@ 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) &&
> + msk->pm.subflows == 1) {
I'm not a big fan of this workaround, especially for a fix that is
supposed to be backported.
Do you need to do a lot of modifications to add a "refcount_t" in
"struct mptcp_pm_addr_entry" and use it?
If really it is complex, I think we will at least need a comment with a
TODO explaining why we need a refcount. (and a new ticket)
(and probably better without "msk->pm.subflows == 1" then)
> + 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 +268,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 +320,25 @@ 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;
> + }
> +
> lock_sock(sk);
>
> err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
>
> release_sock(sk);
>
> + if (err) {
> + spin_lock_bh(&msk->pm.lock);
> + mptcp_userspace_pm_delete_local_addr(msk, &local);
> + spin_unlock_bh(&msk->pm.lock);
> + }
> +
When the subflow is deleted by the other peer, we also need to delete
the local address, no?
Or maybe we want to keep it not to assign the same ID to another
address? And/or to be able to send RM_ADDR after the removal of the
subflow? If yes, it would be clearer to add a comment about that.
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net