[PATCH iproute2-next 0/3] more patches from pm_nl_ctl

[PATCH iproute2-next 0/3] more patches from pm_nl_ctl

[Geliang Tang]

This patched added id 0 address deleting, fullmesh flag and backup setting
from pm_nl_ctl. With these patches, we can replace all pm_nl_ctl in the
selftests with 'ip mptcp' command.

Geliang Tang (3):
  mptcp: add id check for deleting address
  mptcp: add the addr flag fullmesh
  mptcp: add the backup flag setting

 include/uapi/linux/mptcp.h |  2 ++
 ip/ipmptcp.c               | 31 +++++++++++++++++++++++++------
 2 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH RESEND iproute2-next 1/3] mptcp: add id check for deleting address

[Geliang Tang]

This patch added the id check for deleting address in mptcp_parse_opt().
The ADDRESS argument is invalid for the non-zero id address, only needed
for the id 0 address.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/171
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 ip/ipmptcp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
index fd042da8..c2691ef4 100644
--- a/ip/ipmptcp.c
+++ b/ip/ipmptcp.c
@@ -18,7 +18,7 @@ static void usage(void)
 	fprintf(stderr,
 		"Usage:	ip mptcp endpoint add ADDRESS [ dev NAME ] [ id ID ]\n"
 		"				      [ port NR ] [ FLAG-LIST ]\n"
-		"	ip mptcp endpoint delete id ID\n"
+		"	ip mptcp endpoint delete id ID [ ADDRESS ]\n"
 		"	ip mptcp endpoint show [ id ID ]\n"
 		"	ip mptcp endpoint flush\n"
 		"	ip mptcp limits set [ subflows NR ] [ add_addr_accepted NR ]\n"
@@ -142,6 +142,12 @@ static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n,
 
 	if (!id_set && !adding)
 		missarg("ID");
+	else if (id_set && !adding) {
+		if (id && addr_set)
+			invarg("invalid for non-zero id address\n", "ADDRESS");
+		else if (!id && !addr_set)
+			invarg("address is needed for deleting id 0 address\n", "ID");
+	}
 
 	if (port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
 		invarg("flags must have signal when using port", "port");
-- 
2.31.1

[PATCH iproute2-next 2/3] mptcp: add the addr flag fullmesh

[Geliang Tang]

The address flag fullmesh had been added into pm_nl_ctl.c in the commit
371b90377e60 ("selftests: mptcp: set and print the fullmesh flag"). This
patch added it to 'ip mptcp' too.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 ip/ipmptcp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
index c2691ef4..8c4fd18c 100644
--- a/ip/ipmptcp.c
+++ b/ip/ipmptcp.c
@@ -25,7 +25,7 @@ static void usage(void)
 		"	ip mptcp limits show\n"
 		"	ip mptcp monitor\n"
 		"FLAG-LIST := [ FLAG-LIST ] FLAG\n"
-		"FLAG  := [ signal | subflow | backup ]\n");
+		"FLAG  := [ signal | subflow | backup | fullmesh ]\n");
 
 	exit(-1);
 }
@@ -47,6 +47,7 @@ static const struct {
 	{ "signal",		MPTCP_PM_ADDR_FLAG_SIGNAL },
 	{ "subflow",		MPTCP_PM_ADDR_FLAG_SUBFLOW },
 	{ "backup",		MPTCP_PM_ADDR_FLAG_BACKUP },
+	{ "fullmesh",		MPTCP_PM_ADDR_FLAG_FULLMESH },
 };
 
 static void print_mptcp_addr_flags(unsigned int flags)
-- 
2.31.1

[PATCH iproute2-next 3/3] mptcp: add the backup flag setting

[Geliang Tang]

The address flag backup setting had been added into pm_nl_ctl.c in commit
6e8b244a3e9d ("selftests: mptcp: add set_flags command in pm_nl_ctl"). This
patch added it to 'ip mptcp' too.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/uapi/linux/mptcp.h |  2 ++
 ip/ipmptcp.c               | 22 +++++++++++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 957743ce..2b76e526 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -75,6 +75,8 @@ enum {
 #define MPTCP_PM_ADDR_FLAG_BACKUP			(1 << 2)
 #define MPTCP_PM_ADDR_FLAG_FULLMESH			(1 << 3)
 
+#define MPTCP_PM_ADDR_FLAG_MAX				(1 << 8)
+
 enum {
 	MPTCP_PM_CMD_UNSPEC,
 
diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
index 8c4fd18c..fc1fd5ce 100644
--- a/ip/ipmptcp.c
+++ b/ip/ipmptcp.c
@@ -19,6 +19,7 @@ static void usage(void)
 		"Usage:	ip mptcp endpoint add ADDRESS [ dev NAME ] [ id ID ]\n"
 		"				      [ port NR ] [ FLAG-LIST ]\n"
 		"	ip mptcp endpoint delete id ID [ ADDRESS ]\n"
+		"	ip mptcp endpoint set ADDRESS [ backup | nobackup ]"
 		"	ip mptcp endpoint show [ id ID ]\n"
 		"	ip mptcp endpoint flush\n"
 		"	ip mptcp limits set [ subflows NR ] [ add_addr_accepted NR ]\n"
@@ -47,6 +48,7 @@ static const struct {
 	{ "signal",		MPTCP_PM_ADDR_FLAG_SIGNAL },
 	{ "subflow",		MPTCP_PM_ADDR_FLAG_SUBFLOW },
 	{ "backup",		MPTCP_PM_ADDR_FLAG_BACKUP },
+	{ "nobackup",		MPTCP_PM_ADDR_FLAG_MAX },
 	{ "fullmesh",		MPTCP_PM_ADDR_FLAG_FULLMESH },
 };
 
@@ -91,17 +93,24 @@ static int get_flags(const char *arg, __u32 *flags)
 }
 
 static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n,
-			 bool adding)
+			 int cmd)
 {
 	struct rtattr *attr_addr;
 	bool addr_set = false;
 	inet_prefix address;
 	bool id_set = false;
+	bool adding = 0;
+	bool deling = 0;
 	__u32 index = 0;
 	__u32 flags = 0;
 	__u16 port = 0;
 	__u8 id = 0;
 
+	if (cmd == MPTCP_PM_CMD_ADD_ADDR)
+		adding = 1;
+	else if (cmd == MPTCP_PM_CMD_DEL_ADDR)
+		deling = 1;
+
 	ll_init_map(&rth);
 	while (argc > 0) {
 		if (get_flags(*argv, &flags) == 0) {
@@ -141,9 +150,9 @@ static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n,
 	if (!addr_set && adding)
 		missarg("ADDRESS");
 
-	if (!id_set && !adding)
+	if (!id_set && deling)
 		missarg("ID");
-	else if (id_set && !adding) {
+	else if (id_set && deling) {
 		if (id && addr_set)
 			invarg("invalid for non-zero id address\n", "ADDRESS");
 		else if (!id && !addr_set)
@@ -183,7 +192,7 @@ static int mptcp_addr_modify(int argc, char **argv, int cmd)
 	MPTCP_REQUEST(req, cmd, NLM_F_REQUEST);
 	int ret;
 
-	ret = mptcp_parse_opt(argc, argv, &req.n, cmd == MPTCP_PM_CMD_ADD_ADDR);
+	ret = mptcp_parse_opt(argc, argv, &req.n, cmd);
 	if (ret)
 		return ret;
 
@@ -299,7 +308,7 @@ static int mptcp_addr_show(int argc, char **argv)
 	if (argc <= 0)
 		return mptcp_addr_dump();
 
-	ret = mptcp_parse_opt(argc, argv, &req.n, false);
+	ret = mptcp_parse_opt(argc, argv, &req.n, MPTCP_PM_CMD_DEL_ADDR);
 	if (ret)
 		return ret;
 
@@ -534,6 +543,9 @@ int do_mptcp(int argc, char **argv)
 		if (matches(*argv, "delete") == 0)
 			return mptcp_addr_modify(argc-1, argv+1,
 						 MPTCP_PM_CMD_DEL_ADDR);
+		if (matches(*argv, "set") == 0)
+			return mptcp_addr_modify(argc-1, argv+1,
+						 MPTCP_PM_CMD_SET_FLAGS);
 		if (matches(*argv, "show") == 0)
 			return mptcp_addr_show(argc-1, argv+1);
 		if (matches(*argv, "flush") == 0)
-- 
2.31.1

Re: [PATCH iproute2-next 0/3] more patches from pm_nl_ctl

[Mat Martineau]

On Tue, 4 Jan 2022, Geliang Tang wrote:

> This patched added id 0 address deleting, fullmesh flag and backup setting
> from pm_nl_ctl. With these patches, we can replace all pm_nl_ctl in the
> selftests with 'ip mptcp' command.
>
> Geliang Tang (3):
>  mptcp: add id check for deleting address
>  mptcp: add the addr flag fullmesh
>  mptcp: add the backup flag setting
>
> include/uapi/linux/mptcp.h |  2 ++
> ip/ipmptcp.c               | 31 +++++++++++++++++++++++++------
> 2 files changed, 27 insertions(+), 6 deletions(-)
>
> -- 
> 2.31.1

Hi Geliang -

I found that this doesn't apply to 
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git, but it 
does apply to iproute2.git

If you check iproute2-next, there's already a patch in there from Paolo 
that adds the fullmesh flag.

Can you rebase the other two patches on iproute2-next for v2?


Thanks,


--
Mat Martineau
Intel

Re: [PATCH RESEND iproute2-next 1/3] mptcp: add id check for deleting address

[Mat Martineau]

On Tue, 4 Jan 2022, Geliang Tang wrote:

> This patch added the id check for deleting address in mptcp_parse_opt().
> The ADDRESS argument is invalid for the non-zero id address, only needed
> for the id 0 address.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/171
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> ip/ipmptcp.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
> index fd042da8..c2691ef4 100644
> --- a/ip/ipmptcp.c
> +++ b/ip/ipmptcp.c
> @@ -18,7 +18,7 @@ static void usage(void)
> 	fprintf(stderr,
> 		"Usage:	ip mptcp endpoint add ADDRESS [ dev NAME ] [ id ID ]\n"
> 		"				      [ port NR ] [ FLAG-LIST ]\n"
> -		"	ip mptcp endpoint delete id ID\n"
> +		"	ip mptcp endpoint delete id ID [ ADDRESS ]\n"

Please also update man/man8/ip-mptcp.8 for the optional ADDRESS parameter.

> 		"	ip mptcp endpoint show [ id ID ]\n"
> 		"	ip mptcp endpoint flush\n"
> 		"	ip mptcp limits set [ subflows NR ] [ add_addr_accepted NR ]\n"
> @@ -142,6 +142,12 @@ static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n,
>
> 	if (!id_set && !adding)
> 		missarg("ID");
> +	else if (id_set && !adding) {

I think it would be good to move the 'deling' changes (and the change to 
the last parameter of mptcp_parse_opt()) from patch 3 to this patch.

> +		if (id && addr_set)
> +			invarg("invalid for non-zero id address\n", "ADDRESS");
> +		else if (!id && !addr_set)
> +			invarg("address is needed for deleting id 0 address\n", "ID");
> +	}
>
> 	if (port && !(flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
> 		invarg("flags must have signal when using port", "port");
> -- 
> 2.31.1


--
Mat Martineau
Intel

Re: [PATCH iproute2-next 3/3] mptcp: add the backup flag setting

[Mat Martineau]

On Tue, 4 Jan 2022, Geliang Tang wrote:

> The address flag backup setting had been added into pm_nl_ctl.c in commit
> 6e8b244a3e9d ("selftests: mptcp: add set_flags command in pm_nl_ctl"). This
> patch added it to 'ip mptcp' too.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> include/uapi/linux/mptcp.h |  2 ++
> ip/ipmptcp.c               | 22 +++++++++++++++++-----
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index 957743ce..2b76e526 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -75,6 +75,8 @@ enum {
> #define MPTCP_PM_ADDR_FLAG_BACKUP			(1 << 2)
> #define MPTCP_PM_ADDR_FLAG_FULLMESH			(1 << 3)
>
> +#define MPTCP_PM_ADDR_FLAG_MAX				(1 << 8)
> +
> enum {
> 	MPTCP_PM_CMD_UNSPEC,
>
> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
> index 8c4fd18c..fc1fd5ce 100644
> --- a/ip/ipmptcp.c
> +++ b/ip/ipmptcp.c
> @@ -19,6 +19,7 @@ static void usage(void)
> 		"Usage:	ip mptcp endpoint add ADDRESS [ dev NAME ] [ id ID ]\n"
> 		"				      [ port NR ] [ FLAG-LIST ]\n"
> 		"	ip mptcp endpoint delete id ID [ ADDRESS ]\n"
> +		"	ip mptcp endpoint set ADDRESS [ backup | nobackup ]"

Like patch 1, please update the man page too.

> 		"	ip mptcp endpoint show [ id ID ]\n"
> 		"	ip mptcp endpoint flush\n"
> 		"	ip mptcp limits set [ subflows NR ] [ add_addr_accepted NR ]\n"
> @@ -47,6 +48,7 @@ static const struct {
> 	{ "signal",		MPTCP_PM_ADDR_FLAG_SIGNAL },
> 	{ "subflow",		MPTCP_PM_ADDR_FLAG_SUBFLOW },
> 	{ "backup",		MPTCP_PM_ADDR_FLAG_BACKUP },
> +	{ "nobackup",		MPTCP_PM_ADDR_FLAG_MAX },

Since MPTCP_PM_ADDR_FLAG_MAX is 0x100, and the flags attribute in the 
netlink command is 32 bits wide, this ends up setting a bit and sending it 
to the kernel. While that bit isn't used right now, it could be in a 
future kernel version - and this approach to implementing 'nobackup' could 
have unexpected effects. Could you try a different way of implementing 
this that doesn't have a side effect?


- Mat


> 	{ "fullmesh",		MPTCP_PM_ADDR_FLAG_FULLMESH },
> };
>
> @@ -91,17 +93,24 @@ static int get_flags(const char *arg, __u32 *flags)
> }
>
> static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n,
> -			 bool adding)
> +			 int cmd)
> {
> 	struct rtattr *attr_addr;
> 	bool addr_set = false;
> 	inet_prefix address;
> 	bool id_set = false;
> +	bool adding = 0;
> +	bool deling = 0;
> 	__u32 index = 0;
> 	__u32 flags = 0;
> 	__u16 port = 0;
> 	__u8 id = 0;
>
> +	if (cmd == MPTCP_PM_CMD_ADD_ADDR)
> +		adding = 1;
> +	else if (cmd == MPTCP_PM_CMD_DEL_ADDR)
> +		deling = 1;
> +
> 	ll_init_map(&rth);
> 	while (argc > 0) {
> 		if (get_flags(*argv, &flags) == 0) {
> @@ -141,9 +150,9 @@ static int mptcp_parse_opt(int argc, char **argv, struct nlmsghdr *n,
> 	if (!addr_set && adding)
> 		missarg("ADDRESS");
>
> -	if (!id_set && !adding)
> +	if (!id_set && deling)
> 		missarg("ID");
> -	else if (id_set && !adding) {
> +	else if (id_set && deling) {
> 		if (id && addr_set)
> 			invarg("invalid for non-zero id address\n", "ADDRESS");
> 		else if (!id && !addr_set)
> @@ -183,7 +192,7 @@ static int mptcp_addr_modify(int argc, char **argv, int cmd)
> 	MPTCP_REQUEST(req, cmd, NLM_F_REQUEST);
> 	int ret;
>
> -	ret = mptcp_parse_opt(argc, argv, &req.n, cmd == MPTCP_PM_CMD_ADD_ADDR);
> +	ret = mptcp_parse_opt(argc, argv, &req.n, cmd);
> 	if (ret)
> 		return ret;
>
> @@ -299,7 +308,7 @@ static int mptcp_addr_show(int argc, char **argv)
> 	if (argc <= 0)
> 		return mptcp_addr_dump();
>
> -	ret = mptcp_parse_opt(argc, argv, &req.n, false);
> +	ret = mptcp_parse_opt(argc, argv, &req.n, MPTCP_PM_CMD_DEL_ADDR);
> 	if (ret)
> 		return ret;
>
> @@ -534,6 +543,9 @@ int do_mptcp(int argc, char **argv)
> 		if (matches(*argv, "delete") == 0)
> 			return mptcp_addr_modify(argc-1, argv+1,
> 						 MPTCP_PM_CMD_DEL_ADDR);
> +		if (matches(*argv, "set") == 0)
> +			return mptcp_addr_modify(argc-1, argv+1,
> +						 MPTCP_PM_CMD_SET_FLAGS);
> 		if (matches(*argv, "show") == 0)
> 			return mptcp_addr_show(argc-1, argv+1);
> 		if (matches(*argv, "flush") == 0)
> -- 
> 2.31.1
>

--
Mat Martineau
Intel

Re: [PATCH iproute2-next 0/3] more patches from pm_nl_ctl

[Geliang Tang]

Hi Mat,

Thanks for your review.

On Fri, Jan 07, 2022 at 04:11:43PM -0800, Mat Martineau wrote:
> On Tue, 4 Jan 2022, Geliang Tang wrote:
> 
> > This patched added id 0 address deleting, fullmesh flag and backup setting
> > from pm_nl_ctl. With these patches, we can replace all pm_nl_ctl in the
> > selftests with 'ip mptcp' command.
> > 
> > Geliang Tang (3):
> >  mptcp: add id check for deleting address
> >  mptcp: add the addr flag fullmesh
> >  mptcp: add the backup flag setting
> > 
> > include/uapi/linux/mptcp.h |  2 ++
> > ip/ipmptcp.c               | 31 +++++++++++++++++++++++++------
> > 2 files changed, 27 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.31.1
> 
> Hi Geliang -
> 
> I found that this doesn't apply to
> https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git, but it
> does apply to iproute2.git
> 
> If you check iproute2-next, there's already a patch in there from Paolo that
> adds the fullmesh flag.

Yes, v1 was against the wrong repo, not iproute2-next.git.

I just noticed there's already a patch in iproute2-next from Davide that
adds the backup flag setting too:
 "mptcp: add support for changing the backup flag"

So patch 2 and 3 in this series should be dropped.

> 
> Can you rebase the other two patches on iproute2-next for v2?

I only updated patch 1 for v2.

Thanks,
-Geliang

> 
> 
> Thanks,
> 
> 
> --
> Mat Martineau
> Intel
>