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

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

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