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

[Geliang Tang <geliang@kernel.org>]

Hi Mat, Matt,

On Fri, 2025-04-11 at 17:26 -0700, Mat Martineau wrote:
> 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?

I haven't tested it. To be honest, I don't know how to run the test
items of mptcpd. Can you give me some guidance?

Thanks,
-Geliang

> 
> 
> - 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
> > 
> > 
> >