Re: [PATCH mptcp-next v2 1/2] mptcp: pm: userspace: drop delete_local_addr helper

[Mat Martineau <martineau@kernel.org>]

On Thu, 10 Apr 2025, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Address entries should not be removed from local_addr_list when a subflow
> is deleted by the userspace PM, should only be removed when sending a
> REMOVE_ADDR.
>
> So mptcp_userspace_pm_delete_local_addr() helper shouldn't be called in
> mptcp_pm_nl_subflow_create_doit() and mptcp_pm_nl_subflow_destroy_doit().
>
> Since this helper is open-coding in mptcp_pm_nl_remove_doit(), it can be
> dropped now.
>
> Address entries are removed from local_addr_list when sending a REMOVE_ADDR
> by the userspace PM, the local_addr_used counter of PM should also be
> decremented accordingly.
>

Hi Geliang -

Have you tried this patch with mptcpd? Does it affect any tests there?


- Mat


> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/403
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> net/mptcp/pm_userspace.c | 37 +++++--------------------------------
> 1 file changed, 5 insertions(+), 32 deletions(-)
>
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 7fc19b844384..3824b4165421 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -90,30 +90,6 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
> 	return ret;
> }
>
> -/* 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.
> - */
> -static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
> -						struct mptcp_pm_addr_entry *addr)
> -{
> -	struct sock *sk = (struct sock *)msk;
> -	struct mptcp_pm_addr_entry *entry;
> -
> -	entry = mptcp_userspace_pm_lookup_addr(msk, &addr->addr);
> -	if (!entry)
> -		return -EINVAL;
> -
> -	/* TODO: a refcount is needed because the entry can
> -	 * be used multiple times (e.g. fullmesh mode).
> -	 */
> -	list_del_rcu(&entry->list);
> -	sock_kfree_s(sk, entry, sizeof(*entry));
> -	msk->pm.local_addr_used--;
> -	return 0;
> -}
> -
> static struct mptcp_pm_addr_entry *
> mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
> {
> @@ -331,6 +307,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
> 	}
>
> 	list_del_rcu(&match->list);
> +	msk->pm.local_addr_used--;
> 	spin_unlock_bh(&msk->pm.lock);
>
> 	mptcp_pm_remove_addr_entry(msk, match);
> @@ -408,14 +385,13 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
> 	err = __mptcp_subflow_connect(sk, &local, &addr_r);
> 	release_sock(sk);
>
> -	if (err)
> +	if (err) {
> 		GENL_SET_ERR_MSG_FMT(info, "connect error: %d", err);
> +		goto create_err;
> +	}
>
> 	spin_lock_bh(&msk->pm.lock);
> -	if (err)
> -		mptcp_userspace_pm_delete_local_addr(msk, &entry);
> -	else
> -		msk->pm.subflows++;
> +	msk->pm.subflows++;
> 	spin_unlock_bh(&msk->pm.lock);
>
>  create_err:
> @@ -534,9 +510,6 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
> 		goto release_sock;
> 	}
>
> -	spin_lock_bh(&msk->pm.lock);
> -	mptcp_userspace_pm_delete_local_addr(msk, &addr_l);
> -	spin_unlock_bh(&msk->pm.lock);
> 	mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
> 	mptcp_close_ssk(sk, ssk, mptcp_subflow_ctx(ssk));
> 	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
> -- 
> 2.43.0
>
>
>