This patch adds the ability to send RM_ADDR for local ID 0 and the
ability to remove id 0 subflow.

Put id 0 into a removing list, pass it to mptcp_pm_remove_addr() to
remve id 0 address and pass it to mptcp_pm_nl_rm_subflow_received() to
remove id 0 subflow.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index d042d32beb4d..38629ebc4ec6 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -239,6 +239,21 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 		goto remove_err;
 	}
 
+	if (id_val == 0) {
+		struct mptcp_rm_list list = { .nr = 0 };
+
+		list.ids[list.nr++] = 0;
+
+		lock_sock((struct sock *)msk);
+		spin_lock_bh(&msk->pm.lock);
+		mptcp_pm_remove_addr(msk, &list);
+		spin_unlock_bh(&msk->pm.lock);
+		release_sock((struct sock *)msk);
+
+		err = 0;
+		goto remove_err;
+	}
+
 	lock_sock((struct sock *)msk);
 
 	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
@@ -399,14 +414,16 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 	struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
 	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
 	struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
+	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
 	struct mptcp_addr_info addr_l;
 	struct mptcp_addr_info addr_r;
 	struct mptcp_sock *msk;
 	struct sock *sk, *ssk;
 	int err = -EINVAL;
 	u32 token_val;
+	u8 id_val;
 
-	if (!laddr || !raddr || !token) {
+	if (((!laddr || !raddr) && !id) || !token) {
 		GENL_SET_ERR_MSG(info, "missing required inputs");
 		return err;
 	}
@@ -424,6 +441,27 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 		goto destroy_err;
 	}
 
+	if (id) {
+		id_val = nla_get_u8(id);
+		if (id_val == 0) {
+			struct mptcp_rm_list list = { .nr = 0 };
+
+			list.ids[list.nr++] = 0;
+
+			sk = (struct sock *)msk;
+			lock_sock(sk);
+			spin_lock_bh(&msk->pm.lock);
+			mptcp_pm_nl_rm_subflow_received(msk, &list);
+			spin_unlock_bh(&msk->pm.lock);
+			release_sock(sk);
+
+			err = 0;
+		} else {
+			err = -EINVAL;
+		}
+		goto destroy_err;
+	}
+
 	err = mptcp_pm_parse_addr(laddr, info, &addr_l);
 	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
-- 
2.35.3

Hi Geliang,

On 09/08/2023 09:06, Geliang Tang wrote:
> This patch adds the ability to send RM_ADDR for local ID 0 and the
> ability to remove id 0 subflow.
> 
> Put id 0 into a removing list, pass it to mptcp_pm_remove_addr() to
> remve id 0 address and pass it to mptcp_pm_nl_rm_subflow_received() to

(s/remve/remove/)

> remove id 0 subflow.

Please always add the reason, e.g.

  There is no reason not to allow the userspace to remove the initial
address (ID 0) and destroy the initial subflow. This special case was
not taken into account not letting the userspace to delete all
addresses/subflows as announced.

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379

I wonder if we should consider this as a fix: it looks like a bug that
the remove address for ID 0 and the destroy of the initial subflow don't
work.

In this case, we might need:

  Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
  Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow
establishment")

While at it, it might then be better to split this commit in two: one to
remove the address and one to destroy the subflow with id 0. WDYT?

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index d042d32beb4d..38629ebc4ec6 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -239,6 +239,21 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
>  		goto remove_err;
>  	}
>  
> +	if (id_val == 0) {
> +		struct mptcp_rm_list list = { .nr = 0 };
> +
> +		list.ids[list.nr++] = 0;
> +
> +		lock_sock((struct sock *)msk);
> +		spin_lock_bh(&msk->pm.lock);
> +		mptcp_pm_remove_addr(msk, &list);
> +		spin_unlock_bh(&msk->pm.lock);
> +		release_sock((struct sock *)msk);
> +
> +		err = 0;

I think an additional check is needed to make sure ID 0 has not already
been removed, no?

By doing that, we would have a similar behaviour to what we have with
the other IDs, e.g. if we send two remove address command for the same
ID, the second one will fail with "address with specified id not found"
message. We should do the same for ID 0.

> +		goto remove_err;

It is strange to use a label with "err" in the name when there is no
error. Probably better to rename it? ("out"?)

> +	}
> +
>  	lock_sock((struct sock *)msk);
>  
>  	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
> @@ -399,14 +414,16 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
>  	struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
>  	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
>  	struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
> +	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
>  	struct mptcp_addr_info addr_l;
>  	struct mptcp_addr_info addr_r;
>  	struct mptcp_sock *msk;
>  	struct sock *sk, *ssk;
>  	int err = -EINVAL;
>  	u32 token_val;
> +	u8 id_val;
>  
> -	if (!laddr || !raddr || !token) {
> +	if (((!laddr || !raddr) && !id) || !token) {
>  		GENL_SET_ERR_MSG(info, "missing required inputs");
>  		return err;
>  	}
> @@ -424,6 +441,27 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
>  		goto destroy_err;
>  	}
>  
> +	if (id) {
> +		id_val = nla_get_u8(id);
> +		if (id_val == 0) {
> +			struct mptcp_rm_list list = { .nr = 0 };
> +
> +			list.ids[list.nr++] = 0;
> +
> +			sk = (struct sock *)msk;
> +			lock_sock(sk);
> +			spin_lock_bh(&msk->pm.lock);
> +			mptcp_pm_nl_rm_subflow_received(msk, &list);
> +			spin_unlock_bh(&msk->pm.lock);
> +			release_sock(sk);
> +
> +			err = 0;
> +		} else {
> +			err = -EINVAL;
> +		}

I don't think we should do that to fix the issue #379: an ID is linked
to a local IP address and you can have multiple subflows using the same
local IP address, including the initial local IP address (with ID 0).
The current goal of the destroy subflow command is to destroy a specific
subflow. For the issue #379, we should instead accept the destruction of
the initial subflow when we give its local and remove address like we
would do for any other subflows. Or maybe this is something that already
work? e.g. if you create an MPTCP connection from A to B, then a new
subflow from C to D, can you remove the initial subflow (A <-> B)?

What you did here in mptcp_nl_cmd_sf_destroy() looks like a new feature
to be able to destroy all subflows linked to an ID address: an
optimisation not to send a destroy address for each subflows linked to
this address. But is there really a need to be able to do that? If yes,
there is no reason to restrict that to ID 0 and the comment in the
header file in uapi should be updated too. Anyway, that's for a
dedicated commit and that's for -next.

> +		goto destroy_err;

Same here: here, there is potentially no error, best to rename the label
to avoid confusions

> +	}
> +
>  	err = mptcp_pm_parse_addr(laddr, info, &addr_l);
>  	if (err < 0) {
>  		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");

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