Re: [PATCH mptcp-next v10 10/23] mptcp: check userspace pm subflow flag

[Geliang Tang <geliang.tang@linux.dev>]

Hi Mat & Matt,

On Tue, 2024-01-30 at 18:28 -0800, Mat Martineau wrote:
> On Thu, 18 Jan 2024, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Just like MPTCP_PM_ADDR_FLAG_SIGNAL flag is checked in userspace PM
> > announce mptcp_pm_nl_announce_doit(), MPTCP_PM_ADDR_FLAG_SUBFLOW
> > flag
> > should be checked in mptcp_pm_nl_subflow_create_doit() too.
> > 
> > If MPTCP_PM_ADDR_FLAG_SUBFLOW flag is not set, there's no flags
> > field
> > in the output of dump_addr. This looks a bit strange:
> > 
> > 	id 10 flags  10.0.3.2
> > 
> > This patch uses mptcp_pm_parse_entry() instead of
> > mptcp_pm_parse_addr()
> > to get the flags of the entry. Add MPTCP_PM_ADDR_FLAG_SUBFLOW flag
> > check
> > in mptcp_pm_nl_subflow_create_doit().
> > 
> 
> Hi Geliang -
> 
> Have you tested this change with mptcpd?

What do you think to test 'mptcpd' in our CI too? If you agree, I'll
try to do this, and add a new ticket about it in Github.

> 
> Is the addr flag (SUBFLOW or SIGNAL) relevant for userspace PM
> addresses? 
> It doesn't seem like a useful distinction: the userspace PM is always
> creating a new subflow in this context, since address signaling is
> handled 
> by mptcp_pm_nl_announce_doit().
> 
> Two options I see:
>   * update the userspace tools to cleanly handle missing flags for 
> userspace connections
> or
>   * have mptcp_pm_nl_subflow_create_doit() always set 
> MPTCP_PM_ADDR_FLAG_SUBFLOW for the local addr even if it's missing
> from 
> the netlink attribute (and maybe don't allow
> MPTCP_PM_ADDR_FLAG_SIGNAL?)

Thanks for your suggestions. Option 2 has been updated in v11.

-Geliang

> 
> - Mat
> 
> 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > net/mptcp/pm_userspace.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index f1582f40f70e..ca0d6e1dfade 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -361,11 +361,18 @@ int mptcp_pm_nl_subflow_create_doit(struct
> > sk_buff *skb, struct genl_info *info)
> > 		goto create_err;
> > 	}
> > 
> > -	err = mptcp_pm_parse_addr(laddr, info, &addr_l);
> > +	err = mptcp_pm_parse_entry(laddr, info, true, &local);
> > 	if (err < 0) {
> > 		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error
> > parsing local addr");
> > 		goto create_err;
> > 	}
> > +	addr_l = local.addr;
> > +
> > +	if (!(local.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)) {
> > +		GENL_SET_ERR_MSG(info, "invalid addr flags");
> > +		err = -EINVAL;
> > +		goto create_err;
> > +	}
> > 
> > 	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
> > 	if (err < 0) {
> > @@ -379,7 +386,6 @@ int mptcp_pm_nl_subflow_create_doit(struct
> > sk_buff *skb, struct genl_info *info)
> > 		goto create_err;
> > 	}
> > 
> > -	local.addr = addr_l;
> > 	err = mptcp_userspace_pm_append_new_local_addr(msk,
> > &local,
> > 						      
> > !mptcp_pm_has_addr_attr_id(laddr, info));
> > 	if (err < 0) {
> > -- 
> > 2.40.1
> > 
> > 
> >