[PATCH mptcp-next 0/3] mptcp: fix bug in userspace PM APIs

[PATCH mptcp-next 0/3] mptcp: fix bug in userspace PM APIs

[Kishen Maloor]

This series fixes issue #267:
https://github.com/multipath-tcp/mptcp_net-next/issues/267

Specifically, calls to mptcp_token_get_sock() from userspace PM APIs
hold a reference to the msk which needs to be freed before leaving those
functions. This was causing a memory leak which was caught in
the CI debug build.

Kishen Maloor (3):
  Squash-to: mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE
  Squash-to: mptcp: netlink: Add MPTCP_PM_CMD_REMOVE
  Squash-to: mptcp: netlink: allow userspace-driven subflow
    establishment

 net/mptcp/pm_userspace.c | 93 +++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 40 deletions(-)


base-commit: e561667a821b0ecccba582520829f38da50567a3
--
2.31.1

[PATCH mptcp-next 1/3] Squash-to: mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE

[Kishen Maloor]

Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
---
 net/mptcp/pm_userspace.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index fce61530fe9e..fca3e4bdf5d4 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -127,11 +127,11 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
 	struct mptcp_pm_addr_entry addr_val;
 	struct mptcp_sock *msk;
 	u32 token_val;
-	int err;
+	int err = -EINVAL;
 
 	if (!addr || !token) {
 		GENL_SET_ERR_MSG(info, "missing required inputs");
-		return -EINVAL;
+		return err;
 	}
 
 	token_val = nla_get_u32(token);
@@ -139,29 +139,29 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
 	msk = mptcp_token_get_sock(sock_net(skb->sk), token_val);
 	if (!msk) {
 		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
-		return -EINVAL;
+		return err;
 	}
 
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
-		return -EINVAL;
+		goto announce_err;
 	}
 
 	err = mptcp_pm_parse_entry(addr, info, true, &addr_val);
 	if (err < 0) {
 		GENL_SET_ERR_MSG(info, "error parsing local address");
-		return err;
+		goto announce_err;
 	}
 
 	if (addr_val.addr.id == 0 || !(addr_val.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
 		GENL_SET_ERR_MSG(info, "invalid addr id or flags");
-		return -EINVAL;
+		goto announce_err;
 	}
 
 	err = mptcp_userspace_pm_append_new_local_addr(msk, &addr_val);
 	if (err < 0) {
 		GENL_SET_ERR_MSG(info, "did not match address and id");
-		return err;
+		goto announce_err;
 	}
 
 	lock_sock((struct sock *)msk);
@@ -175,7 +175,10 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
 	spin_unlock_bh(&msk->pm.lock);
 	release_sock((struct sock *)msk);
 
-	return 0;
+	err = 0;
+ announce_err:
+	sock_put((struct sock *)msk);
+	return err;
 }
 
 int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
-- 
2.31.1

Re: [PATCH mptcp-next 1/3] Squash-to: mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE

[Geliang Tang]

Hi Kishen,

Kishen Maloor <kishen.maloor@intel.com> 于2022年4月20日周三 07:03写道：
>
> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
> ---
>  net/mptcp/pm_userspace.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index fce61530fe9e..fca3e4bdf5d4 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -127,11 +127,11 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
>         struct mptcp_pm_addr_entry addr_val;
>         struct mptcp_sock *msk;
>         u32 token_val;
> -       int err;
> +       int err = -EINVAL;

This breaks the "reversed XMas tree" order.

Thanks,
-Geliang

>
>         if (!addr || !token) {
>                 GENL_SET_ERR_MSG(info, "missing required inputs");
> -               return -EINVAL;
> +               return err;
>         }
>
>         token_val = nla_get_u32(token);
> @@ -139,29 +139,29 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
>         msk = mptcp_token_get_sock(sock_net(skb->sk), token_val);
>         if (!msk) {
>                 NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
> -               return -EINVAL;
> +               return err;
>         }
>
>         if (!mptcp_pm_is_userspace(msk)) {
>                 GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
> -               return -EINVAL;
> +               goto announce_err;
>         }
>
>         err = mptcp_pm_parse_entry(addr, info, true, &addr_val);
>         if (err < 0) {
>                 GENL_SET_ERR_MSG(info, "error parsing local address");
> -               return err;
> +               goto announce_err;
>         }
>
>         if (addr_val.addr.id == 0 || !(addr_val.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>                 GENL_SET_ERR_MSG(info, "invalid addr id or flags");
> -               return -EINVAL;
> +               goto announce_err;
>         }
>
>         err = mptcp_userspace_pm_append_new_local_addr(msk, &addr_val);
>         if (err < 0) {
>                 GENL_SET_ERR_MSG(info, "did not match address and id");
> -               return err;
> +               goto announce_err;
>         }
>
>         lock_sock((struct sock *)msk);
> @@ -175,7 +175,10 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
>         spin_unlock_bh(&msk->pm.lock);
>         release_sock((struct sock *)msk);
>
> -       return 0;
> +       err = 0;
> + announce_err:
> +       sock_put((struct sock *)msk);
> +       return err;
>  }
>
>  int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
> --
> 2.31.1
>
>

[PATCH mptcp-next 2/3] Squash-to: mptcp: netlink: Add MPTCP_PM_CMD_REMOVE

[Kishen Maloor]

Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
---
 net/mptcp/pm_userspace.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index fca3e4bdf5d4..3d8f6126b952 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -189,12 +189,13 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 	struct mptcp_pm_addr_entry *entry;
 	struct mptcp_sock *msk;
 	LIST_HEAD(free_list);
+	int err = -EINVAL;
 	u32 token_val;
 	u8 id_val;
 
 	if (!id || !token) {
 		GENL_SET_ERR_MSG(info, "missing required inputs");
-		return -EINVAL;
+		return err;
 	}
 
 	id_val = nla_get_u8(id);
@@ -203,12 +204,12 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 	msk = mptcp_token_get_sock(sock_net(skb->sk), token_val);
 	if (!msk) {
 		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
-		return -EINVAL;
+		return err;
 	}
 
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
-		return -EINVAL;
+		goto remove_err;
 	}
 
 	lock_sock((struct sock *)msk);
@@ -223,7 +224,7 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 	if (!match) {
 		GENL_SET_ERR_MSG(info, "address with specified id not found");
 		release_sock((struct sock *)msk);
-		return -EINVAL;
+		goto remove_err;
 	}
 
 	list_move(&match->list, &free_list);
@@ -235,7 +236,11 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 	list_for_each_entry_safe(match, entry, &free_list, list) {
 		sock_kfree_s((struct sock *)msk, match, sizeof(*match));
 	}
-	return 0;
+
+	err = 0;
+ remove_err:
+	sock_put((struct sock *)msk);
+	return err;
 }
 
 int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
-- 
2.31.1

[PATCH mptcp-next 3/3] Squash-to: mptcp: netlink: allow userspace-driven subflow establishment

[Kishen Maloor]

Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
---
 net/mptcp/pm_userspace.c | 59 ++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 3d8f6126b952..9f6b5eda5524 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -251,13 +251,13 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 	struct mptcp_addr_info addr_r;
 	struct mptcp_addr_info addr_l;
 	struct mptcp_sock *msk;
+	int err = -EINVAL;
 	struct sock *sk;
 	u32 token_val;
-	int ret;
 
 	if (!laddr || !raddr || !token) {
 		GENL_SET_ERR_MSG(info, "missing required inputs");
-		return -EINVAL;
+		return err;
 	}
 
 	token_val = nla_get_u32(token);
@@ -265,39 +265,41 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 	msk = mptcp_token_get_sock(genl_info_net(info), token_val);
 	if (!msk) {
 		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
-		return -EINVAL;
+		return err;
 	}
 
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
-		return -EINVAL;
+		goto create_err;
 	}
 
-	ret = mptcp_pm_parse_addr(laddr, info, &addr_l);
-	if (ret < 0) {
+	err = mptcp_pm_parse_addr(laddr, info, &addr_l);
+	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
-		return -EINVAL;
+		goto create_err;
 	}
 
 	if (addr_l.id == 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "missing local addr id");
-		return -EINVAL;
+		goto create_err;
 	}
 
-	ret = mptcp_pm_parse_addr(raddr, info, &addr_r);
-	if (ret < 0) {
+	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
+	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");
-		return -EINVAL;
+		goto create_err;
 	}
 
 	sk = &msk->sk.icsk_inet.sk;
 	lock_sock(sk);
 
-	ret = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
+	err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
 
 	release_sock(sk);
 
-	return ret;
+ create_err:
+	sock_put((struct sock *)msk);
+	return err;
 }
 
 static struct sock *mptcp_nl_find_ssk(struct mptcp_sock *msk,
@@ -366,12 +368,12 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 	struct mptcp_addr_info addr_r;
 	struct mptcp_sock *msk;
 	struct sock *sk, *ssk;
+	int err = -EINVAL;
 	u32 token_val;
-	int ret;
 
 	if (!laddr || !raddr || !token) {
 		GENL_SET_ERR_MSG(info, "missing required inputs");
-		return -EINVAL;
+		return err;
 	}
 
 	token_val = nla_get_u32(token);
@@ -379,34 +381,34 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 	msk = mptcp_token_get_sock(genl_info_net(info), token_val);
 	if (!msk) {
 		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
-		return -EINVAL;
+		return err;
 	}
 
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
-		return -EINVAL;
+		goto destroy_err;
 	}
 
-	ret = mptcp_pm_parse_addr(laddr, info, &addr_l);
-	if (ret < 0) {
+	err = mptcp_pm_parse_addr(laddr, info, &addr_l);
+	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
-		return ret;
+		goto destroy_err;
 	}
 
-	ret = mptcp_pm_parse_addr(raddr, info, &addr_r);
-	if (ret < 0) {
+	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
+	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");
-		return ret;
+		goto destroy_err;
 	}
 
 	if (addr_l.family != addr_r.family) {
 		GENL_SET_ERR_MSG(info, "address families do not match");
-		return -EINVAL;
+		goto destroy_err;
 	}
 
 	if (!addr_l.port || !addr_r.port) {
 		GENL_SET_ERR_MSG(info, "missing local or remote port");
-		return -EINVAL;
+		goto destroy_err;
 	}
 
 	sk = &msk->sk.icsk_inet.sk;
@@ -416,9 +418,12 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 
 		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 		mptcp_close_ssk(sk, ssk, subflow);
+		err = 0;
 	} else {
-		ret = -ESRCH;
+		err = -ESRCH;
 	}
 
-	return ret;
+ destroy_err:
+	sock_put((struct sock *)msk);
+	return err;
 }
-- 
2.31.1

Re: Squash-to: mptcp: netlink: allow userspace-driven subflow establishment: Tests Results

[MPTCP CI]

Hi Kishen,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5545811772702720
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5545811772702720/summary/summary.txt

- {"code":404,"message":
  - "HTTP 404 Not Found"}:
  - Task: https://cirrus-ci.com/task/6671711679545344
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6671711679545344/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b0d39b257105


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-next 0/3] mptcp: fix bug in userspace PM APIs

[Mat Martineau]

On Tue, 19 Apr 2022, Kishen Maloor wrote:

> This series fixes issue #267:
> https://github.com/multipath-tcp/mptcp_net-next/issues/267
>
> Specifically, calls to mptcp_token_get_sock() from userspace PM APIs
> hold a reference to the msk which needs to be freed before leaving those
> functions. This was causing a memory leak which was caught in
> the CI debug build.
>
> Kishen Maloor (3):
>  Squash-to: mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE
>  Squash-to: mptcp: netlink: Add MPTCP_PM_CMD_REMOVE
>  Squash-to: mptcp: netlink: allow userspace-driven subflow
>    establishment
>
> net/mptcp/pm_userspace.c | 93 +++++++++++++++++++++++-----------------
> 1 file changed, 53 insertions(+), 40 deletions(-)
>
>
> base-commit: e561667a821b0ecccba582520829f38da50567a3
> --
> 2.31.1
>
>

Thanks Kishen - patches are ready to squash unless the CI surprises us.

Matthieu, I did run these in a virtme environment to confirm the leaks 
were fixed. The only remaining leak I saw was the upstream one (in 
msr_build_context()).

--
Mat Martineau
Intel