Re: [PATCH mptcp-next v4 5/6] mptcp: userspace pm remove id 0 address

[Geliang Tang <geliang.tang@suse.com>]

Hi Matt,

On Fri, Aug 18, 2023 at 11:30:01AM +0200, Matthieu Baerts wrote:
> Hi Geliang
> 
> On 18/08/2023 09:12, Geliang Tang wrote:
> > This patch adds the ability to send RM_ADDR for local ID 0. Put id 0
> > into a removing list, pass it to mptcp_pm_remove_addr() to remove id
> > 0 address.
> > 
> > There is no reason not to allow the userspace to remove the initial
> > address (ID 0). This special case was not taken into account not
> > letting the userspace to delete all addresses as announced.
> 
> Thank you for the new version!
> 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
> > Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  net/mptcp/pm_userspace.c | 42 +++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index d042d32beb4d..059b672b55f7 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -236,7 +236,43 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
> >  
> >  	if (!mptcp_pm_is_userspace(msk)) {
> >  		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
> > -		goto remove_err;
> > +		goto out;
> > +	}
> > +
> > +	if (id_val == 0) {
> > +		struct mptcp_rm_list list = { .nr = 0 };
> > +		struct mptcp_subflow_context *subflow;
> > +		bool has_id_0 = false;
> > +
> > +		if (!READ_ONCE(msk->first)) {
> > +			GENL_SET_ERR_MSG(info, "no subflow in conn_list");
> > +			goto out;
> > +		}
> 
> Sorry, my previous message was probably not clear: if (msk->first !=
> NULL), it means the initial subflow is still there and ID 0 is still
> being used. If it is no longer there, ID 0 might still be used by
> another subflow and it is required to iterate over the subflow list.
> 
> But maybe easier here to skip this "optimisation" (remove this block
> above) and simply iterate over the subflow list like we would do for the
> other IDs.

I prefer to skip this block in v5. 

> 
> EDIT: mmh, if there is another subflow created with ID 0, will it be in
> msk->pm.userspace_pm_local_addr_list list? (I didn't check) If yes, it
> should also be handled the same way we do for the other subflows. In
> this case, maybe it is not needed to iterate over the different subflows
> but only the local addr list and if not found (!match), just for ID==0
> case, we check if (msk->first != NULL) and create a fake entry? Or we
> always create an entry in local_addr_list for ID == 0? (I didn't check
> what would be best)
> 
> > +
> > +		spin_lock_bh(&msk->pm.lock);
> 
> Is it enough? To iterate over the subflow list, you don't need the msk
> lock? (lock_sock(sk))
> 
> > +		mptcp_for_each_subflow(msk, subflow) {
> > +			if (subflow->remote_id == 0) {
> > +				has_id_0 = true;
> > +				break;
> > +			}
> > +		}
> > +		spin_unlock_bh(&msk->pm.lock);
> > +
> > +		if (!has_id_0) {
> > +			GENL_SET_ERR_MSG(info, "address with id 0 not found");
> > +			goto out;
> > +		}
> > +
> > +		list.ids[list.nr++] = 0;
> > +
> > +		lock_sock((struct sock *)msk);
> > +		spin_lock_bh(&msk->pm.lock);
> > +		mptcp_pm_remove_addr(msk, &list);
> > +		spin_unlock_bh(&msk->pm.lock);
> > +		release_sock((struct sock *)msk);
> > +
> > +		err = 0;
> > +		goto out;
> >  	}
> >  
> >  	lock_sock((struct sock *)msk);
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net