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

[Mat Martineau <martineau@kernel.org>]

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?

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

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