Re: [PATCH mptcp-next v5 05/14] mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2022-03-16 at 19:16 -0400, Kishen Maloor wrote:
> This change adds a MPTCP netlink interface for issuing
> ADD_ADDR advertisements over the chosen MPTCP connection from a
> userspace path manager.
> 
> The command requires the following parameters:
> { token, { loc_id, family, daddr4 | daddr6 [, dport] } [, if_idx],
> flags[signal] }.
> 
> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
> ---
>  include/uapi/linux/mptcp.h |  2 ++
>  net/mptcp/pm_netlink.c     | 64 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index e41ea01a94bb..ac66c1263f02 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -55,6 +55,7 @@ enum {
>  	MPTCP_PM_ATTR_ADDR,				/* nested address */
>  	MPTCP_PM_ATTR_RCV_ADD_ADDRS,			/* u32 */
>  	MPTCP_PM_ATTR_SUBFLOWS,				/* u32 */
> +	MPTCP_PM_ATTR_TOKEN,				/* u32 */
>  
>  	__MPTCP_PM_ATTR_MAX
>  };
> @@ -93,6 +94,7 @@ enum {
>  	MPTCP_PM_CMD_SET_LIMITS,
>  	MPTCP_PM_CMD_GET_LIMITS,
>  	MPTCP_PM_CMD_SET_FLAGS,
> +	MPTCP_PM_CMD_ANNOUNCE,
>  
>  	__MPTCP_PM_CMD_AFTER_LAST
>  };
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index aa1a9ae6b530..2b1d8b3b7891 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1178,6 +1178,7 @@ static const struct nla_policy mptcp_pm_policy[MPTCP_PM_ATTR_MAX + 1] = {
>  					NLA_POLICY_NESTED(mptcp_pm_addr_policy),
>  	[MPTCP_PM_ATTR_RCV_ADD_ADDRS]	= { .type	= NLA_U32,	},
>  	[MPTCP_PM_ATTR_SUBFLOWS]	= { .type	= NLA_U32,	},
> +	[MPTCP_PM_ATTR_TOKEN]		= { .type	= NLA_U32,	},
>  };
>  
>  void mptcp_pm_nl_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
> @@ -1930,6 +1931,64 @@ static int mptcp_nl_set_flags(struct net *net,
>  	return ret;
>  }
>  
> +static int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> +	struct nlattr *addr = info->attrs[MPTCP_PM_ATTR_ADDR];
> +	struct mptcp_pm_addr_entry addr_val;
> +	struct mptcp_sock *msk;
> +	u32 token_val;
> +	int err;
> +
> +	if (!addr || !token) {
> +		GENL_SET_ERR_MSG(info, "missing required inputs");
> +		return -EINVAL;
> +	}
> +
> +	token_val = nla_get_u32(token);
> +
> +	msk = mptcp_token_get_sock(sock_net(skb->sk), token_val);
> +	if (!msk) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
> +		return -EINVAL;
> +	}
> +
> +	if (!mptcp_pm_is_userspace(msk)) {
> +		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
> +		return -EINVAL;
> +	}
> +
> +	err = mptcp_pm_parse_entry(addr, info, true, &addr_val);
> +	if (err < 0) {
> +		GENL_SET_ERR_MSG(info, "error parsing local address");
> +		return err;
> +	}
> +
> +	if (addr_val.addr.id == 0 || !(addr_val.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> +		GENL_SET_ERR_MSG(info, "invalid addr id or flags");
> +		return -EINVAL;
> +	}
> +
> +	err = mptcp_userspace_pm_append_new_local_addr(msk, &addr_val);
> +	if (err < 0) {
> +		GENL_SET_ERR_MSG(info, "did not match address and id");
> +		return err;
> +	}
> +
> +	lock_sock((struct sock *)msk);
> +	spin_lock_bh(&msk->pm.lock);
> +
> +	if (mptcp_pm_alloc_anno_list(msk, &addr_val)) {
> +		mptcp_pm_announce_addr(msk, &addr_val.addr, false);
> +		mptcp_pm_nl_addr_send_ack(msk);
> +	}
> +
> +	spin_unlock_bh(&msk->pm.lock);
> +	release_sock((struct sock *)msk);
> +
> +	return 0;
> +}

If you are going to create a new pm_userspace.c compilation unit, I
think the above function could land there - yep, on the flip side it
will not be static anymore, still I think it would make easier the long
-term maintenance.

Similar comments for mptcp_nl_cmd_remove(), mptcp_nl_cmd_sf_create(),
mptcp_nl_find_ssk(), mptcp_nl_cmd_sf_destroy() in the following
patches.

/P