Re: [PATCH mptcp-next v7 01/24] mptcp: set set_id flag when parsing addr

[Mat Martineau <martineau@kernel.org>]

On Sat, 30 Dec 2023, Geliang Tang wrote:

> When userspace PM requires to create an ID 0 subflow in "userspace pm
> create id 0 subflow" test like this:
>
>        userspace_pm_add_sf $ns2 10.0.3.2 0
>
> An ID 1 subflow, in fact, is created.
>
> Since in mptcp_pm_nl_append_new_local_addr(), 'id 0' will be treated as
> no ID is set by userspace, and will allocate a new ID immediately:
>
>     if (!e->addr.id)
>             e->addr.id = find_next_zero_bit(pernet->id_bitmap,
>                                             MPTCP_PM_MAX_ADDR_ID + 1,
>                                             1);
>
> To solve this issue, a new flag 'MPTCP_PM_ADDR_FLAG_SET_ID' is added
> to distinguish between whether userspace PM has set an ID 0 or whether
> userspace PM has not set any address.

Hi Geliang -

It's better to not modify the UAPI here, and it isn't necessary to get the 
userspace PM behavior we need.

mptcp_pm_nl_append_new_local_addr() is only has two callers:

mptcp_pm_nl_get_local_id(), which always needs a new ID allocated

and

mptcp_pm_nl_add_addr_doit(), which needs to allow ID 0.


Instead of changing the UAPI, modify this function to add a 3rd arg:

static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 					     struct mptcp_pm_addr_entry *entry,
 					     bool needs_id)

change the code you mentioned above to:

      if (needs_id)
              e->addr.id = find_next_zero_bit(pernet->id_bitmap,
                                              MPTCP_PM_MAX_ADDR_ID + 1,
                                              1);

and update the two callers. This replaces patches 1-3.



- Mat




>
> Add a new parameter 'set_id' for mptcp_pm_parse_pm_addr_attr(), and
> pass a 'set_id' flag to them. If an address id is set from userspace,
> this 'set_id' will be set as true. If 'set_id' is set, then the newly
> added flag MPTCP_PM_ADDR_FLAG_SET_ID will be set.
>
> Fixes: e5ed101a6028 ("mptcp: userspace pm allow creating id 0 subflow")
> Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
> ---
> include/uapi/linux/mptcp.h |  1 +
> net/mptcp/pm_netlink.c     | 17 +++++++++++++----
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index 74cfe496891e..ef3663792765 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -36,6 +36,7 @@
> #define MPTCP_PM_ADDR_FLAG_BACKUP                      (1 << 2)
> #define MPTCP_PM_ADDR_FLAG_FULLMESH                    (1 << 3)
> #define MPTCP_PM_ADDR_FLAG_IMPLICIT                    (1 << 4)
> +#define MPTCP_PM_ADDR_FLAG_SET_ID                      (1 << 5)
>
> struct mptcp_info {
> 	__u8	mptcpi_subflows;
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 661c226dad18..dedc5a038b10 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1159,7 +1159,8 @@ static int mptcp_pm_parse_pm_addr_attr(struct nlattr *tb[],
> 				       const struct nlattr *attr,
> 				       struct genl_info *info,
> 				       struct mptcp_addr_info *addr,
> -				       bool require_family)
> +				       bool require_family,
> +				       bool *set_id)
> {
> 	int err, addr_addr;
>
> @@ -1174,8 +1175,11 @@ static int mptcp_pm_parse_pm_addr_attr(struct nlattr *tb[],
> 	if (err)
> 		return err;
>
> -	if (tb[MPTCP_PM_ADDR_ATTR_ID])
> +	if (tb[MPTCP_PM_ADDR_ATTR_ID]) {
> 		addr->id = nla_get_u8(tb[MPTCP_PM_ADDR_ATTR_ID]);
> +		if (set_id)
> +			*set_id = true;
> +	}
>
> 	if (!tb[MPTCP_PM_ADDR_ATTR_FAMILY]) {
> 		if (!require_family)
> @@ -1223,7 +1227,7 @@ int mptcp_pm_parse_addr(struct nlattr *attr, struct genl_info *info,
>
> 	memset(addr, 0, sizeof(*addr));
>
> -	return mptcp_pm_parse_pm_addr_attr(tb, attr, info, addr, true);
> +	return mptcp_pm_parse_pm_addr_attr(tb, attr, info, addr, true, NULL);
> }
>
> int mptcp_pm_parse_entry(struct nlattr *attr, struct genl_info *info,
> @@ -1231,11 +1235,13 @@ int mptcp_pm_parse_entry(struct nlattr *attr, struct genl_info *info,
> 			 struct mptcp_pm_addr_entry *entry)
> {
> 	struct nlattr *tb[MPTCP_PM_ADDR_ATTR_MAX + 1];
> +	bool set_id = false;
> 	int err;
>
> 	memset(entry, 0, sizeof(*entry));
>
> -	err = mptcp_pm_parse_pm_addr_attr(tb, attr, info, &entry->addr, require_family);
> +	err = mptcp_pm_parse_pm_addr_attr(tb, attr, info, &entry->addr,
> +					  require_family, &set_id);
> 	if (err)
> 		return err;
>
> @@ -1248,6 +1254,9 @@ int mptcp_pm_parse_entry(struct nlattr *attr, struct genl_info *info,
> 	if (tb[MPTCP_PM_ADDR_ATTR_FLAGS])
> 		entry->flags = nla_get_u32(tb[MPTCP_PM_ADDR_ATTR_FLAGS]);
>
> +	if (set_id)
> +		entry->flags |= MPTCP_PM_ADDR_FLAG_SET_ID;
> +
> 	if (tb[MPTCP_PM_ADDR_ATTR_PORT])
> 		entry->addr.port = htons(nla_get_u16(tb[MPTCP_PM_ADDR_ATTR_PORT]));
>
> -- 
> 2.39.2
>
>
>