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

[Geliang Tang <geliang@kernel.org>]

Hi Mat, Matt,

On Tue, 2025-04-15 at 11:09 +0800, Geliang Tang wrote:
> 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?

I just tested it this way, and here are the test results:

~/mptcpd/tests$ LD_PRELOAD=../src/.libs/libmptcpwrap.so ./mptcpwrap-
tester 
Test case 0: PASS
Test case 1: PASS
Test case 2: PASS
Test case 3: PASS
Test case 4: PASS
Test case 5: PASS
Test case 6: PASS

~/mptcpd/tests$ LD_PRELOAD=../src/.libs/libmptcpwrap.so ./test-
mptcpwrap 
Test case 0: PASS
Test case 1: PASS
Test case 2: PASS
Test case 3: mptcpwrap: changing socket protocol from 0x0 to 0x106
(IPPROTO_MPTCP) for family 0x2 type 0x1 fd 3
PASS
Test case 4: mptcpwrap: changing socket protocol from 0x0 to 0x106
(IPPROTO_MPTCP) for family 0xa type 0x1 fd 3
PASS
Test case 5: mptcpwrap: changing socket protocol from 0x6 to 0x106
(IPPROTO_MPTCP) for family 0x2 type 0x1 fd 3
PASS
Test case 6: mptcpwrap: changing socket protocol from 0x6 to 0x106
(IPPROTO_MPTCP) for family 0xa type 0x1 fd 3
PASS

~/mptcpd/tests$ LD_PRELOAD=../src/.libs/libmptcpwrap.so
TEST_PLUGIN_DIR=./plugins ./test-start-stop
Running non-interactive sudo check...
    succeeded
MPTCP is not enabled in the kernel.
Try 'sysctl -w net.mptcp.enabled=1'.
Required kernel MPTCP support not available.
TEST CASE: net.mptcp.enabled = 0 ...passed
Unable to load path manager plugins.
TEST CASE: net.mptcp.enabled = 1 ...failed

~/mptcpd/tests$ sudo ./test-bad-log-empty 
mptcpd: Unknown logging option: ""
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-log-long 
mptcpd: Unknown logging option: "foo"
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-log-short 
mptcpd: Unknown logging option: "foo"
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-option 
/home/tgl/mptcpd/src/.libs/mptcpd: unrecognized option '--foo'
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-path-manager 
mptcpd: Empty default path manager plugin command line option.
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-plugin-dir 
mptcpd: Empty plugin directory command line option.
Try `mptcpd --help' or `mptcpd --usage' for more information.

These results appear to be unchanged from before. And in fact these
tests do not test userspace pm.

So it seems that this patch will not affect the tests of mptcpd.

Thanks,
-Geliang

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