[RFC PATCH 0/4] mptcp: Update netlink interface for userspace PM

[RFC PATCH 0/4] mptcp: Update netlink interface for userspace PM

[Kishen Maloor]

This patch series marks the beginning of an attempt to update the MPTCP
netlink interface with commands to support the offloading of path management 
decisions to userspace path managers.

Specifically, the commits in this series introduce the following commands:

* MPTCP_PM_CMD_ANNOUNCE: Issue an ADD_ADDR advertisement over the selected MPTCP
  connection.

* MPTCP_PM_CMD_REMOVE: Issue a REMOVE_ADDR signal to remove a previously added
  address over the selected MPTCP connection.

Additionally, the PM netlink test controller (pm_nl_ctl) was updated to
support the new commands.

The netlink implementations above exercise the "userspace PM mode" defined
in the following patch series:
https://lore.kernel.org/mptcp/20211006235953.212243-1-mathew.j.martineau@linux.intel.com/T/#t

Kishen Maloor (4):
  mptcp: pm_netlink: Add MPTCP_PM_CMD_ANNOUNCE
  mptcp: selftests: MPTCP_PM_CMD_ANNOUNCE
  mptcp: pm_netlink: Add MPTCP_PM_CMD_REMOVE
  mptcp: selftests: MPTCP_PM_CMD_REMOVE

 include/uapi/linux/mptcp.h                    |   4 +
 net/mptcp/pm_netlink.c                        |  88 +++++++++
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 172 ++++++++++++++++++
 3 files changed, 264 insertions(+)

-- 
2.31.1

[RFC PATCH 1/4] mptcp: pm_netlink: Add MPTCP_PM_CMD_ANNOUNCE

[Kishen Maloor]

This change adds a MPTCP netlink interface command 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, saddr4 | saddr6 [, sport]} }.

Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
---
 include/uapi/linux/mptcp.h |  2 ++
 net/mptcp/pm_netlink.c     | 45 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index c8cc46f80a16..e22e891016e3 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
 };
@@ -92,6 +93,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 2a3589b67768..1b41bbc70529 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1028,6 +1028,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)
@@ -1735,6 +1736,45 @@ static int mptcp_nl_addr_backup(struct net *net,
 	return ret;
 }
 
+static int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
+{
+	struct nlattr *addr = info->attrs[MPTCP_PM_ATTR_ADDR],
+		*token = info->attrs[MPTCP_PM_ATTR_TOKEN];
+	struct mptcp_sock *msk;
+	struct mptcp_pm_addr_entry _addr;
+	u32 _token;
+	int err;
+
+	if (!addr || !token) {
+		GENL_SET_ERR_MSG(info, "missing announce info");
+		return -EINVAL;
+	}
+
+	err = mptcp_pm_parse_addr(addr, info, true, &_addr);
+	if (err < 0)
+		return err;
+
+	_token = nla_get_u32(token);
+
+	msk = mptcp_token_get_sock(sock_net(skb->sk), _token);
+	if (!msk) {
+		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
+		return -EINVAL;
+	}
+
+	lock_sock((struct sock *)msk);
+	spin_lock_bh(&msk->pm.lock);
+
+	msk->pm.userspace = true;
+	mptcp_pm_announce_addr(msk, &_addr.addr, false);
+	mptcp_pm_nl_addr_send_ack(msk);
+
+	spin_unlock_bh(&msk->pm.lock);
+	release_sock((struct sock *)msk);
+
+	return 0;
+}
+
 static int mptcp_nl_cmd_set_flags(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
@@ -2067,6 +2107,11 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
 		.doit   = mptcp_nl_cmd_set_flags,
 		.flags  = GENL_ADMIN_PERM,
 	},
+	{
+		.cmd    = MPTCP_PM_CMD_ANNOUNCE,
+		.doit   = mptcp_nl_cmd_announce,
+		.flags  = GENL_ADMIN_PERM,
+	},
 };
 
 static struct genl_family mptcp_genl_family __ro_after_init = {
-- 
2.31.1

[RFC PATCH 2/4] mptcp: selftests: MPTCP_PM_CMD_ANNOUNCE

[Kishen Maloor]

This change updates the "pm_nl_ctl" testing sample with an "ann" (announce)
option to support the newly added netlink interface command
MPTCP_PM_CMD_ANNOUNCE to issue ADD_ADDR advertisements over the
chosen MPTCP connection.

E.g. ./pm_nl_ctl ann 192.168.122.75 token 823274047 id 2345 dev enp1s0

Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
---
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 121 ++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 354784512748..47c2667ad676 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -26,6 +26,7 @@ static void syntax(char *argv[])
 {
 	fprintf(stderr, "%s add|get|set|del|flush|dump|accept [<args>]\n", argv[0]);
 	fprintf(stderr, "\tadd [flags signal|subflow|backup|fullmesh] [id <nr>] [dev <name>] <ip>\n");
+	fprintf(stderr, "\tann <ip> token <token> [port <port>] [id <nr>] [dev <name>]\n");
 	fprintf(stderr, "\tdel <id> [<ip>]\n");
 	fprintf(stderr, "\tget <id>\n");
 	fprintf(stderr, "\tset <ip> [flags backup|nobackup]\n");
@@ -170,6 +171,124 @@ static int resolve_mptcp_pm_netlink(int fd)
 	return genl_parse_getfamily((void *)data);
 }
 
+int announce_addr(int fd, int pm_family, int argc, char *argv[])
+{
+	char data[NLMSG_ALIGN(sizeof(struct nlmsghdr)) +
+		  NLMSG_ALIGN(sizeof(struct genlmsghdr)) +
+		  1024];
+	struct rtattr *rta, *addr;
+	struct nlmsghdr *nh;
+	u_int32_t flags = 0;
+	u_int16_t family;
+	u_int32_t token;
+	int addr_start;
+	u_int8_t id, got_token = 0;
+	int off = 0;
+	int arg;
+
+	memset(data, 0, sizeof(data));
+	nh = (void *)data;
+	off = init_genl_req(data, pm_family, MPTCP_PM_CMD_ANNOUNCE,
+			    MPTCP_PM_VER);
+
+	if (argc < 5)
+		syntax(argv);
+
+	/* addr header */
+	addr_start = off;
+	addr = (void *)(data + off);
+	addr->rta_type = NLA_F_NESTED | MPTCP_PM_ATTR_ADDR;
+	addr->rta_len = RTA_LENGTH(0);
+	off += NLMSG_ALIGN(addr->rta_len);
+
+	/* addr data */
+	rta = (void *)(data + off);
+	if (inet_pton(AF_INET, argv[2], RTA_DATA(rta))) {
+		family = AF_INET;
+		rta->rta_type = MPTCP_PM_ADDR_ATTR_ADDR4;
+		rta->rta_len = RTA_LENGTH(4);
+	} else if (inet_pton(AF_INET6, argv[2], RTA_DATA(rta))) {
+		family = AF_INET6;
+		rta->rta_type = MPTCP_PM_ADDR_ATTR_ADDR6;
+		rta->rta_len = RTA_LENGTH(16);
+	} else
+		error(1, errno, "can't parse ip %s", argv[2]);
+	off += NLMSG_ALIGN(rta->rta_len);
+
+	/* family */
+	rta = (void *)(data + off);
+	rta->rta_type = MPTCP_PM_ADDR_ATTR_FAMILY;
+	rta->rta_len = RTA_LENGTH(2);
+	memcpy(RTA_DATA(rta), &family, 2);
+	off += NLMSG_ALIGN(rta->rta_len);
+
+	flags |= MPTCP_PM_ADDR_FLAG_SIGNAL;
+
+	for (arg = 3; arg < argc; arg++) {
+		if (!strcmp(argv[arg], "id")) {
+			if (++arg >= argc)
+				error(1, 0, " missing id value");
+
+			id = atoi(argv[arg]);
+			rta = (void *)(data + off);
+			rta->rta_type = MPTCP_PM_ADDR_ATTR_ID;
+			rta->rta_len = RTA_LENGTH(1);
+			memcpy(RTA_DATA(rta), &id, 1);
+			off += NLMSG_ALIGN(rta->rta_len);
+		} else if (!strcmp(argv[arg], "dev")) {
+			int32_t ifindex;
+
+			if (++arg >= argc)
+				error(1, 0, " missing dev name");
+
+			ifindex = if_nametoindex(argv[arg]);
+			if (!ifindex)
+				error(1, errno, "unknown device %s", argv[arg]);
+
+			rta = (void *)(data + off);
+			rta->rta_type = MPTCP_PM_ADDR_ATTR_IF_IDX;
+			rta->rta_len = RTA_LENGTH(4);
+			memcpy(RTA_DATA(rta), &ifindex, 4);
+			off += NLMSG_ALIGN(rta->rta_len);
+		} else if (!strcmp(argv[arg], "port")) {
+			u_int16_t port;
+
+			if (++arg >= argc)
+				error(1, 0, " missing port value");
+			if (!(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
+				error(1, 0, " flags must be signal when using port");
+
+			port = atoi(argv[arg]);
+			rta = (void *)(data + off);
+			rta->rta_type = MPTCP_PM_ADDR_ATTR_PORT;
+			rta->rta_len = RTA_LENGTH(2);
+			memcpy(RTA_DATA(rta), &port, 2);
+			off += NLMSG_ALIGN(rta->rta_len);
+		} else if (!strcmp(argv[arg], "token")) {
+			if (++arg >= argc)
+				error(1, 0, " missing token value");
+			token = atoi(argv[arg]);
+			got_token = 1;
+		} else
+			error(1, 0, "unknown keyword %s", argv[arg]);
+	}
+	addr->rta_len = off - addr_start;
+
+	if (!got_token)
+		error(1, 0, " missing token value");
+
+	/* token */
+	rta = (void *)(data + off);
+	rta->rta_type = MPTCP_PM_ATTR_TOKEN;
+	rta->rta_len = RTA_LENGTH(4);
+	memcpy(RTA_DATA(rta), &token, 4);
+	off += NLMSG_ALIGN(rta->rta_len);
+
+	do_nl_req(fd, nh, off, 0);
+
+	return 0;
+}
+
 int add_addr(int fd, int pm_family, int argc, char *argv[])
 {
 	char data[NLMSG_ALIGN(sizeof(struct nlmsghdr)) +
@@ -744,6 +863,8 @@ int main(int argc, char *argv[])
 
 	if (!strcmp(argv[1], "add"))
 		return add_addr(fd, pm_family, argc, argv);
+	else if (!strcmp(argv[1], "ann"))
+		return announce_addr(fd, pm_family, argc, argv);
 	else if (!strcmp(argv[1], "del"))
 		return del_addr(fd, pm_family, argc, argv);
 	else if (!strcmp(argv[1], "flush"))
-- 
2.31.1

[RFC PATCH 3/4] mptcp: pm_netlink: Add MPTCP_PM_CMD_REMOVE

[Kishen Maloor]

This change adds a MPTCP netlink interface command for issuing
REMOVE_ADDR signals for a specific address over the chosen MPTCP
connection from a userspace path manager.

The command requires the following parameters: {token, loc_id}.

Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
---
 include/uapi/linux/mptcp.h |  2 ++
 net/mptcp/pm_netlink.c     | 43 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index e22e891016e3..0ce8abce6d54 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -56,6 +56,7 @@ enum {
 	MPTCP_PM_ATTR_RCV_ADD_ADDRS,			/* u32 */
 	MPTCP_PM_ATTR_SUBFLOWS,				/* u32 */
 	MPTCP_PM_ATTR_TOKEN,				/* u32 */
+	MPTCP_PM_ATTR_LOC_ID,				/* u8 */
 
 	__MPTCP_PM_ATTR_MAX
 };
@@ -94,6 +95,7 @@ enum {
 	MPTCP_PM_CMD_GET_LIMITS,
 	MPTCP_PM_CMD_SET_FLAGS,
 	MPTCP_PM_CMD_ANNOUNCE,
+	MPTCP_PM_CMD_REMOVE,
 
 	__MPTCP_PM_CMD_AFTER_LAST
 };
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 1b41bbc70529..bf92ef605043 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1029,6 +1029,7 @@ static const struct nla_policy mptcp_pm_policy[MPTCP_PM_ATTR_MAX + 1] = {
 	[MPTCP_PM_ATTR_RCV_ADD_ADDRS]	= { .type	= NLA_U32,	},
 	[MPTCP_PM_ATTR_SUBFLOWS]	= { .type	= NLA_U32,	},
 	[MPTCP_PM_ATTR_TOKEN]		= { .type	= NLA_U32,	},
+	[MPTCP_PM_ATTR_LOC_ID]		= { .type	= NLA_U8,	},
 };
 
 void mptcp_pm_nl_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
@@ -1736,6 +1737,43 @@ static int mptcp_nl_addr_backup(struct net *net,
 	return ret;
 }
 
+static int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
+{
+	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID],
+		*token = info->attrs[MPTCP_PM_ATTR_TOKEN];
+	struct mptcp_sock *msk;
+	struct mptcp_rm_list rm_list = { .nr = 0 };
+	u32 _token;
+	u8 _id;
+
+	if (!id || !token) {
+		GENL_SET_ERR_MSG(info, "missing remove info");
+		return -EINVAL;
+	}
+
+	_id = nla_get_u8(id);
+	_token = nla_get_u32(token);
+
+	msk = mptcp_token_get_sock(sock_net(skb->sk), _token);
+	if (!msk) {
+		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
+		return -EINVAL;
+	}
+
+	rm_list.ids[rm_list.nr++] = _id;
+
+	lock_sock((struct sock *)msk);
+	spin_lock_bh(&msk->pm.lock);
+
+	msk->pm.userspace = true;
+	mptcp_pm_remove_addr(msk, &rm_list);
+
+	spin_unlock_bh(&msk->pm.lock);
+	release_sock((struct sock *)msk);
+
+	return 0;
+}
+
 static int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *addr = info->attrs[MPTCP_PM_ATTR_ADDR],
@@ -2112,6 +2150,11 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
 		.doit   = mptcp_nl_cmd_announce,
 		.flags  = GENL_ADMIN_PERM,
 	},
+	{
+		.cmd    = MPTCP_PM_CMD_REMOVE,
+		.doit   = mptcp_nl_cmd_remove,
+		.flags  = GENL_ADMIN_PERM,
+	},
 };
 
 static struct genl_family mptcp_genl_family __ro_after_init = {
-- 
2.31.1

[RFC PATCH 4/4] mptcp: selftests: MPTCP_PM_CMD_REMOVE

[Kishen Maloor]

This change updates the "pm_nl_ctl" testing sample with a "rem" (remove)
option to support the newly added netlink interface command
MPTCP_PM_CMD_REMOVE to issue REMOVE_ADDR signals over the
chosen MPTCP connection.

E.g. ./pm_nl_ctl rem token 823274047 id 2345

Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
---
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 47c2667ad676..8adc370060d6 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -27,6 +27,7 @@ static void syntax(char *argv[])
 	fprintf(stderr, "%s add|get|set|del|flush|dump|accept [<args>]\n", argv[0]);
 	fprintf(stderr, "\tadd [flags signal|subflow|backup|fullmesh] [id <nr>] [dev <name>] <ip>\n");
 	fprintf(stderr, "\tann <ip> token <token> [port <port>] [id <nr>] [dev <name>]\n");
+	fprintf(stderr, "\trem id <nr> token <token>");
 	fprintf(stderr, "\tdel <id> [<ip>]\n");
 	fprintf(stderr, "\tget <id>\n");
 	fprintf(stderr, "\tset <ip> [flags backup|nobackup]\n");
@@ -171,6 +172,54 @@ static int resolve_mptcp_pm_netlink(int fd)
 	return genl_parse_getfamily((void *)data);
 }
 
+int remove_addr(int fd, int pm_family, int argc, char *argv[])
+{
+	char data[NLMSG_ALIGN(sizeof(struct nlmsghdr)) +
+		  NLMSG_ALIGN(sizeof(struct genlmsghdr)) +
+		  1024];
+	struct rtattr *rta;
+	struct nlmsghdr *nh;
+	u_int8_t id;
+	u_int32_t token;
+	int off = 0, arg;
+
+	memset(data, 0, sizeof(data));
+	nh = (void *)data;
+	off = init_genl_req(data, pm_family, MPTCP_PM_CMD_REMOVE,
+			    MPTCP_PM_VER);
+
+	if (argc < 6)
+		syntax(argv);
+
+	for (arg = 2; arg < argc; arg++) {
+		if (!strcmp(argv[arg], "id")) {
+			if (++arg >= argc)
+				error(1, 0, " missing id value");
+
+			id = atoi(argv[arg]);
+			rta = (void *)(data + off);
+			rta->rta_type = MPTCP_PM_ATTR_LOC_ID;
+			rta->rta_len = RTA_LENGTH(1);
+			memcpy(RTA_DATA(rta), &id, 1);
+			off += NLMSG_ALIGN(rta->rta_len);
+		} else if (!strcmp(argv[arg], "token")) {
+			if (++arg >= argc)
+				error(1, 0, " missing token value");
+
+			token = atoi(argv[arg]);
+			rta = (void *)(data + off);
+			rta->rta_type = MPTCP_PM_ATTR_TOKEN;
+			rta->rta_len = RTA_LENGTH(4);
+			memcpy(RTA_DATA(rta), &token, 4);
+			off += NLMSG_ALIGN(rta->rta_len);
+		} else
+			error(1, 0, "unknown keyword %s", argv[arg]);
+	}
+
+	do_nl_req(fd, nh, off, 0);
+	return 0;
+}
+
 int announce_addr(int fd, int pm_family, int argc, char *argv[])
 {
 	char data[NLMSG_ALIGN(sizeof(struct nlmsghdr)) +
@@ -865,6 +914,8 @@ int main(int argc, char *argv[])
 		return add_addr(fd, pm_family, argc, argv);
 	else if (!strcmp(argv[1], "ann"))
 		return announce_addr(fd, pm_family, argc, argv);
+	else if (!strcmp(argv[1], "rem"))
+		return remove_addr(fd, pm_family, argc, argv);
 	else if (!strcmp(argv[1], "del"))
 		return del_addr(fd, pm_family, argc, argv);
 	else if (!strcmp(argv[1], "flush"))
-- 
2.31.1

Re: [RFC PATCH 0/4] mptcp: Update netlink interface for userspace PM

[Florian Westphal]

Kishen Maloor <kishen.maloor@intel.com> wrote:
> This patch series marks the beginning of an attempt to update the MPTCP
> netlink interface with commands to support the offloading of path management 
> decisions to userspace path managers.
> 
> Specifically, the commits in this series introduce the following commands:
> 
> * MPTCP_PM_CMD_ANNOUNCE: Issue an ADD_ADDR advertisement over the selected MPTCP
>   connection.

Looks like you guys are further ahead than me.
I've pushed my work here:

iproute2:
https://git.breakpoint.cc/cgit/fw/iproute2.git/log/?h=mptcp_sf_03

kernel:
https://git.breakpoint.cc/cgit/fw/mptcp_net-next.git/log/?h=netlink_cmd_07

I had planned to work on packetdrill tests next but to avoid
further duplicate work I will refrain from doing so.

Feel free to take what you need and/or mangle and discard this at will.

Re: [RFC PATCH 1/4] mptcp: pm_netlink: Add MPTCP_PM_CMD_ANNOUNCE

[Paolo Abeni]

On Thu, 2021-10-07 at 00:36 -0400, Kishen Maloor wrote:
> This change adds a MPTCP netlink interface command 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, saddr4 | saddr6 [, sport]} }.
> 
> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
> ---
>  include/uapi/linux/mptcp.h |  2 ++
>  net/mptcp/pm_netlink.c     | 45 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index c8cc46f80a16..e22e891016e3 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
>  };
> @@ -92,6 +93,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 2a3589b67768..1b41bbc70529 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1028,6 +1028,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)
> @@ -1735,6 +1736,45 @@ static int mptcp_nl_addr_backup(struct net *net,
>  	return ret;
>  }
>  
> +static int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct nlattr *addr = info->attrs[MPTCP_PM_ATTR_ADDR],
> +		*token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> +	struct mptcp_sock *msk;
> +	struct mptcp_pm_addr_entry _addr;

The above should be change to respect the reverse xmas tree order, and
it's better to split the 2 assignement in separate definition:

	struct nlattr *addr = info->attrs[MPTCP_PM_ATTR_ADDR];
	struct nlattr *token = /...

> +	u32 _token;
> +	int err;
> +
> +	if (!addr || !token) {
> +		GENL_SET_ERR_MSG(info, "missing announce info");
> +		return -EINVAL;
> +	}
> +
> +	err = mptcp_pm_parse_addr(addr, info, true, &_addr);
> +	if (err < 0)
> +		return err;
> +
> +	_token = nla_get_u32(token);

I'm wondering if a new helper 'mptcp_pm_parse_addr_ex', additionally
parsing the code would help.

Cheers,

Paolo

Re: [RFC PATCH 2/4] mptcp: selftests: MPTCP_PM_CMD_ANNOUNCE

[Paolo Abeni]

On Thu, 2021-10-07 at 00:36 -0400, Kishen Maloor wrote:
> This change updates the "pm_nl_ctl" testing sample with an "ann" (announce)
> option to support the newly added netlink interface command
> MPTCP_PM_CMD_ANNOUNCE to issue ADD_ADDR advertisements over the
> chosen MPTCP connection.
> 
> E.g. ./pm_nl_ctl ann 192.168.122.75 token 823274047 id 2345 dev enp1s0

We need an actual test case with the above, likely in a new script.

Thanks!

Paolo

Re: [RFC PATCH 3/4] mptcp: pm_netlink: Add MPTCP_PM_CMD_REMOVE

[Paolo Abeni]

On Thu, 2021-10-07 at 00:36 -0400, Kishen Maloor wrote:
> This change adds a MPTCP netlink interface command for issuing
> REMOVE_ADDR signals for a specific address over the chosen MPTCP
> connection from a userspace path manager.
> 
> The command requires the following parameters: {token, loc_id}.
> 
> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
> ---
>  include/uapi/linux/mptcp.h |  2 ++
>  net/mptcp/pm_netlink.c     | 43 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index e22e891016e3..0ce8abce6d54 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -56,6 +56,7 @@ enum {
>  	MPTCP_PM_ATTR_RCV_ADD_ADDRS,			/* u32 */
>  	MPTCP_PM_ATTR_SUBFLOWS,				/* u32 */
>  	MPTCP_PM_ATTR_TOKEN,				/* u32 */
> +	MPTCP_PM_ATTR_LOC_ID,				/* u8 */
>  
>  	__MPTCP_PM_ATTR_MAX
>  };
> @@ -94,6 +95,7 @@ enum {
>  	MPTCP_PM_CMD_GET_LIMITS,
>  	MPTCP_PM_CMD_SET_FLAGS,
>  	MPTCP_PM_CMD_ANNOUNCE,
> +	MPTCP_PM_CMD_REMOVE,
>  
>  	__MPTCP_PM_CMD_AFTER_LAST
>  };
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 1b41bbc70529..bf92ef605043 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1029,6 +1029,7 @@ static const struct nla_policy mptcp_pm_policy[MPTCP_PM_ATTR_MAX + 1] = {
>  	[MPTCP_PM_ATTR_RCV_ADD_ADDRS]	= { .type	= NLA_U32,	},
>  	[MPTCP_PM_ATTR_SUBFLOWS]	= { .type	= NLA_U32,	},
>  	[MPTCP_PM_ATTR_TOKEN]		= { .type	= NLA_U32,	},
> +	[MPTCP_PM_ATTR_LOC_ID]		= { .type	= NLA_U8,	},
>  };
>  
>  void mptcp_pm_nl_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
> @@ -1736,6 +1737,43 @@ static int mptcp_nl_addr_backup(struct net *net,
>  	return ret;
>  }
>  
> +static int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID],
> +		*token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> +	struct mptcp_sock *msk;
> +	struct mptcp_rm_list rm_list = { .nr = 0 };
> +	u32 _token;
> +	u8 _id;

Same comment as patch 1/4 about xmas tree order ;)

/P

Re: [RFC PATCH 0/4] mptcp: Update netlink interface for userspace PM

[Paolo Abeni]

On Thu, 2021-10-07 at 10:32 +0200, Florian Westphal wrote:
> Kishen Maloor <kishen.maloor@intel.com> wrote:
> > This patch series marks the beginning of an attempt to update the MPTCP
> > netlink interface with commands to support the offloading of path management 
> > decisions to userspace path managers.
> > 
> > Specifically, the commits in this series introduce the following commands:
> > 
> > * MPTCP_PM_CMD_ANNOUNCE: Issue an ADD_ADDR advertisement over the selected MPTCP
> >   connection.
> 
> Looks like you guys are further ahead than me.
> I've pushed my work here:
> 
> iproute2:
> https://git.breakpoint.cc/cgit/fw/iproute2.git/log/?h=mptcp_sf_03
> 
> kernel:
> https://git.breakpoint.cc/cgit/fw/mptcp_net-next.git/log/?h=netlink_cmd_07
> 
> I had planned to work on packetdrill tests next but to avoid
> further duplicate work I will refrain from doing so.
> 
> Feel free to take what you need and/or mangle and discard this at will.

It looks like your patch:

mptcp: netlink: allow userspace-driven subflow establishment

implements feature not available here, while:

mptcp: netlink: allow userspace to perform address announcements

overlaps with this effort.

I think we can merge the 2 series into a single one dropping only the
later patch, if there is agreement.

Cheers,

Paolo

Re: [RFC PATCH 1/4] mptcp: pm_netlink: Add MPTCP_PM_CMD_ANNOUNCE

[Matthieu Baerts]

Hi Kishen,

On 07/10/2021 06:36, Kishen Maloor wrote:
> This change adds a MPTCP netlink interface command 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, saddr4 | saddr6 [, sport]} }.

Thank you for looking at that!

(...)

> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 2a3589b67768..1b41bbc70529 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c

(...)

> +static int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct nlattr *addr = info->attrs[MPTCP_PM_ATTR_ADDR],
> +		*token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> +	struct mptcp_sock *msk;
> +	struct mptcp_pm_addr_entry _addr;
> +	u32 _token;

Small detail but maybe clearer to avoid using variable with almost the
same name: 'addr' vs '_addr' and 'token', vs '_token'. What about
'addr_attr' and 'token_attr'?

Same in 'mptcp_nl_cmd_remove()'.

> +	int err;
> +
> +	if (!addr || !token) {
> +		GENL_SET_ERR_MSG(info, "missing announce info");
> +		return -EINVAL;
> +	}
> +
> +	err = mptcp_pm_parse_addr(addr, info, true, &_addr);
> +	if (err < 0)
> +		return err;
> +
> +	_token = nla_get_u32(token);
> +
> +	msk = mptcp_token_get_sock(sock_net(skb->sk), _token);
> +	if (!msk) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
> +		return -EINVAL;

Should we try to return different err code per kind of error? Or the
error message is enough?

For example, we could return -ENOTCONN or -ENOKEY even if the string
linked to it is not 100% accurate? ("Transport endpoint is not
connected" and "Required key not available").

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [RFC PATCH 0/4] mptcp: Update netlink interface for userspace PM

[Kishen Maloor]

On 10/7/21 4:26 AM, Paolo Abeni wrote:
> On Thu, 2021-10-07 at 10:32 +0200, Florian Westphal wrote:
>> Kishen Maloor <kishen.maloor@intel.com> wrote:
>>> This patch series marks the beginning of an attempt to update the MPTCP
>>> netlink interface with commands to support the offloading of path management 
>>> decisions to userspace path managers.
>>>
>>> Specifically, the commits in this series introduce the following commands:
>>>
>>> * MPTCP_PM_CMD_ANNOUNCE: Issue an ADD_ADDR advertisement over the selected MPTCP
>>>   connection.
>>
>> Looks like you guys are further ahead than me.
>> I've pushed my work here:
>>
>> iproute2:
>> https://git.breakpoint.cc/cgit/fw/iproute2.git/log/?h=mptcp_sf_03
>>
>> kernel:
>> https://git.breakpoint.cc/cgit/fw/mptcp_net-next.git/log/?h=netlink_cmd_07
>>

Thank you. Just so its clear, I'd picked up this Github issue assigned to Mat mostly to avoid such overlap :)

>> I had planned to work on packetdrill tests next but to avoid
>> further duplicate work I will refrain from doing so.

I can say that you won't see any duplication here specifically :)

>>
>> Feel free to take what you need and/or mangle and discard this at will.
> 
> It looks like your patch:
> 
> mptcp: netlink: allow userspace-driven subflow establishment
> 
> implements feature not available here, while:
> 
> mptcp: netlink: allow userspace to perform address announcements
> 
> overlaps with this effort.
> 
> I think we can merge the 2 series into a single one dropping only the
> later patch, if there is agreement.

Yup, we can look into this.

> 
> Cheers,
> 
> Paolo
>