Re: [PATCH mptcp-next v3 15/29] mptcp: userspace pm remove id 0 address

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

On Thu, Sep 28, 2023 at 11:00:55PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 25/09/2023 10:41, Geliang Tang wrote:
> > This patch adds the ability to send RM_ADDR for local ID 0. Check
> > whether id 0 address is removed, if not, 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.
> > 
> > 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 | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index 6b8083650bc1..8d97cf475cac 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -211,6 +211,38 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
> >  	return err;
> >  }
> >  
> > +static int mptcp_userspace_remove_id_zero_address(struct mptcp_sock *msk,
> > +						  struct genl_info *info)
> > +{
> > +	struct mptcp_rm_list list = { .nr = 0 };
> > +	struct mptcp_subflow_context *subflow;
> > +	struct sock *sk = (struct sock *)msk;
> > +	bool has_id_0 = false;
> > +	int err = -EINVAL;
> > +
> > +	lock_sock(sk);
> > +	spin_lock_bh(&msk->pm.lock);
> 
> Why do you need to lock the 'pm' structure? mptcp_pm_remove_addr() will
> modify the 'pm' structure but it will also do the lock, no?

mptcp_pm_remove_addrs will do the lock but mptcp_pm_remove_addr does need
the locks.

Thanks,
-Geliang

> 
> > +	mptcp_for_each_subflow(msk, subflow) {
> > +		if (subflow->remote_id == 0) {
> 
> I only noticed that now but you need to look at local_id, not remote_id:
> when we send a RM_ADDR, we announce that a previously announced (local)
> address ID is now invalid → so we need to check for local IDs here.
> 
> > +			has_id_0 = true;
> > +			break;
> > +		}
> > +	}
> > +	if (!has_id_0) {
> > +		GENL_SET_ERR_MSG(info, "address with id 0 not found");
> > +		goto out;
> > +	}
> > +
> > +	list.ids[list.nr++] = 0;
> > +	mptcp_pm_remove_addr(msk, &list);
> > +	err = 0;
> > +out:
> > +	spin_unlock_bh(&msk->pm.lock);
> > +	release_sock(sk);
> > +	sock_put(sk);
> 
> It looks strange to me to do a 'put' here because we didn't "hold" the
> socket here in this function.
> 
> Should we not do the 'sock_put(sk)' in mptcp_pm_nl_remove_doit() instead?
> 
> > +	return err;
> > +}
> > +
> >  int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
> >  {
> >  	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> > @@ -245,6 +277,9 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
> >  		goto remove_err;
> >  	}
> >  
> > +	if (id_val == 0)
> > +		return mptcp_userspace_remove_id_zero_address(msk, info);
> 
> Linked to what I said above: maybe here it would be better to have:
> 
>   err = mptcp_userspace_remove_id_zero_address(msk, info);
>   goto out; // the sock_put() will be done there
> 
> (rename 'remove_err' to 'out' as there is potentially no errors here)
> 
> WDYT?
> 
> 
> > +
> >  	lock_sock(sk);
> >  
> >  	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net