[PATCH mptcp-next v2 0/2] add pm listener events

[PATCH mptcp-next v2 0/2] add pm listener events

[Geliang Tang]

v2:
- send created event from mptcp_listen.
- add a new patch, use entry->lsk directly.

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

Geliang Tang (2):
  mptcp: use entry->lsk directly
  mptcp: add pm listener events

 include/uapi/linux/mptcp.h |  9 +++++
 net/mptcp/pm_netlink.c     | 73 +++++++++++++++++++++++++++++---------
 net/mptcp/protocol.c       |  3 ++
 net/mptcp/protocol.h       |  2 ++
 4 files changed, 71 insertions(+), 16 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v2 1/2] mptcp: use entry->lsk directly

[Geliang Tang]

This patch drops msk and ssock in mptcp_pm_nl_create_listen_socket(),
use entry->lsk directly instead of ssock.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_netlink.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 9813ed0fde9b..52a5847b9d74 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -992,8 +992,6 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 {
 	int addrlen = sizeof(struct sockaddr_in);
 	struct sockaddr_storage addr;
-	struct mptcp_sock *msk;
-	struct socket *ssock;
 	int backlog = 1024;
 	int err;
 
@@ -1002,30 +1000,18 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	if (err)
 		return err;
 
-	msk = mptcp_sk(entry->lsk->sk);
-	if (!msk) {
-		err = -EINVAL;
-		goto out;
-	}
-
-	ssock = __mptcp_nmpc_socket(msk);
-	if (!ssock) {
-		err = -EINVAL;
-		goto out;
-	}
-
 	mptcp_info2sockaddr(&entry->addr, &addr, entry->addr.family);
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 	if (entry->addr.family == AF_INET6)
 		addrlen = sizeof(struct sockaddr_in6);
 #endif
-	err = kernel_bind(ssock, (struct sockaddr *)&addr, addrlen);
+	err = kernel_bind(entry->lsk, (struct sockaddr *)&addr, addrlen);
 	if (err) {
 		pr_warn("kernel_bind error, err=%d", err);
 		goto out;
 	}
 
-	err = kernel_listen(ssock, backlog);
+	err = kernel_listen(entry->lsk, backlog);
 	if (err) {
 		pr_warn("kernel_listen error, err=%d", err);
 		goto out;
-- 
2.35.3

[PATCH mptcp-next v2 2/2] mptcp: add pm listener events

[Geliang Tang]

This patch adds MPTCP netlink events for PM listening socket create and
close.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/uapi/linux/mptcp.h |  9 +++++++
 net/mptcp/pm_netlink.c     | 55 ++++++++++++++++++++++++++++++++++++++
 net/mptcp/protocol.c       |  3 +++
 net/mptcp/protocol.h       |  2 ++
 4 files changed, 69 insertions(+)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index dfe19bf13f4c..32af2d278cb4 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -160,6 +160,12 @@ struct mptcp_info {
  *                           daddr4 | daddr6, sport, dport, backup, if_idx
  *                           [, error]
  * The priority of a subflow has changed. 'error' should not be set.
+ *
+ * MPTCP_EVENT_LISTENER_CREATED: family, sport, saddr4 | saddr6
+ * A new PM listener is created.
+ *
+ * MPTCP_EVENT_LISTENER_CLOSED: family, sport, saddr4 | saddr6
+ * A PM listener is closed.
  */
 enum mptcp_event_type {
 	MPTCP_EVENT_UNSPEC = 0,
@@ -174,6 +180,9 @@ enum mptcp_event_type {
 	MPTCP_EVENT_SUB_CLOSED = 11,
 
 	MPTCP_EVENT_SUB_PRIORITY = 13,
+
+	MPTCP_EVENT_LISTENER_CREATED = 15,
+	MPTCP_EVENT_LISTENER_CLOSED = 16,
 };
 
 enum mptcp_event_attr {
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 52a5847b9d74..02469d603261 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -2145,6 +2145,58 @@ void mptcp_event_addr_announced(const struct sock *ssk,
 	kfree_skb(skb);
 }
 
+void mptcp_event_pm_listener(const struct sock *ssk,
+			     enum mptcp_event_type event)
+{
+	const struct inet_sock *issk = inet_sk(ssk);
+	struct net *net = sock_net(ssk);
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+
+	if (!genl_has_listeners(&mptcp_genl_family, net, MPTCP_PM_EV_GRP_OFFSET))
+		return;
+
+	skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+	if (!skb)
+		return;
+
+	nlh = genlmsg_put(skb, 0, 0, &mptcp_genl_family, 0, event);
+	if (!nlh)
+		goto nla_put_failure;
+
+	if (nla_put_u16(skb, MPTCP_ATTR_FAMILY, ssk->sk_family))
+		goto nla_put_failure;
+
+	if (nla_put_be16(skb, MPTCP_ATTR_SPORT, issk->inet_sport))
+		goto nla_put_failure;
+
+	switch (ssk->sk_family) {
+	case AF_INET:
+		if (nla_put_in_addr(skb, MPTCP_ATTR_SADDR4, issk->inet_saddr))
+			goto nla_put_failure;
+		break;
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+	case AF_INET6: {
+		const struct ipv6_pinfo *np = inet6_sk(ssk);
+
+		if (nla_put_in6_addr(skb, MPTCP_ATTR_SADDR6, &np->saddr))
+			goto nla_put_failure;
+		break;
+	}
+#endif
+	default:
+		WARN_ON_ONCE(1);
+		goto nla_put_failure;
+	}
+
+	genlmsg_end(skb, nlh);
+	mptcp_nl_mcast_send(net, skb, GFP_ATOMIC);
+	return;
+
+nla_put_failure:
+	kfree_skb(skb);
+}
+
 void mptcp_event(enum mptcp_event_type type, const struct mptcp_sock *msk,
 		 const struct sock *ssk, gfp_t gfp)
 {
@@ -2190,6 +2242,9 @@ void mptcp_event(enum mptcp_event_type type, const struct mptcp_sock *msk,
 		if (mptcp_event_sub_closed(skb, msk, ssk) < 0)
 			goto nla_put_failure;
 		break;
+	case MPTCP_EVENT_LISTENER_CREATED:
+	case MPTCP_EVENT_LISTENER_CLOSED:
+		break;
 	}
 
 	genlmsg_end(skb, nlh);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index dceeaaf3bab4..a1f9dddbdf2c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2358,6 +2358,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 			tcp_set_state(ssk, TCP_CLOSE);
 			mptcp_subflow_queue_clean(ssk);
 			inet_csk_listen_stop(ssk);
+			mptcp_event_pm_listener(ssk, MPTCP_EVENT_LISTENER_CLOSED);
 		}
 		__tcp_close(ssk, 0);
 
@@ -3674,6 +3675,8 @@ static int mptcp_listen(struct socket *sock, int backlog)
 	if (!err)
 		mptcp_copy_inaddrs(sock->sk, ssock->sk);
 
+	mptcp_event_pm_listener(ssock->sk, MPTCP_EVENT_LISTENER_CREATED);
+
 unlock:
 	release_sock(sock->sk);
 	return err;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 8f48f881adf8..d2c34360a3e3 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -839,6 +839,8 @@ void mptcp_event(enum mptcp_event_type type, const struct mptcp_sock *msk,
 		 const struct sock *ssk, gfp_t gfp);
 void mptcp_event_addr_announced(const struct sock *ssk, const struct mptcp_addr_info *info);
 void mptcp_event_addr_removed(const struct mptcp_sock *msk, u8 id);
+void mptcp_event_pm_listener(const struct sock *ssk,
+			     enum mptcp_event_type event);
 bool mptcp_userspace_pm_active(const struct mptcp_sock *msk);
 
 static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)
-- 
2.35.3

Re: mptcp: add pm listener events: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 2 failed test(s): selftest_mptcp_join selftest_userspace_pm 🔴:
  - Task: https://cirrus-ci.com/task/5474410546069504
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5474410546069504/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 3 failed test(s): packetdrill_add_addr selftest_mptcp_join selftest_userspace_pm - Critical: 1 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/6600310452912128
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6600310452912128/summary/summary.txt

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


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 v2 1/2] mptcp: use entry->lsk directly

[Mat Martineau]

On Wed, 26 Oct 2022, Geliang Tang wrote:

> This patch drops msk and ssock in mptcp_pm_nl_create_listen_socket(),
> use entry->lsk directly instead of ssock.
>

Hi Geliang -

The change looks good, but the old code looks buggy. Seems like it would 
be trying to re-bind the nmpc socket each time a listener socket was 
created. Is this a fix for mptcp-net in your opinion?


Thanks,

Mat

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/pm_netlink.c | 18 ++----------------
> 1 file changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 9813ed0fde9b..52a5847b9d74 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -992,8 +992,6 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
> {
> 	int addrlen = sizeof(struct sockaddr_in);
> 	struct sockaddr_storage addr;
> -	struct mptcp_sock *msk;
> -	struct socket *ssock;
> 	int backlog = 1024;
> 	int err;
>
> @@ -1002,30 +1000,18 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
> 	if (err)
> 		return err;
>
> -	msk = mptcp_sk(entry->lsk->sk);
> -	if (!msk) {
> -		err = -EINVAL;
> -		goto out;
> -	}
> -
> -	ssock = __mptcp_nmpc_socket(msk);
> -	if (!ssock) {
> -		err = -EINVAL;
> -		goto out;
> -	}
> -
> 	mptcp_info2sockaddr(&entry->addr, &addr, entry->addr.family);
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> 	if (entry->addr.family == AF_INET6)
> 		addrlen = sizeof(struct sockaddr_in6);
> #endif
> -	err = kernel_bind(ssock, (struct sockaddr *)&addr, addrlen);
> +	err = kernel_bind(entry->lsk, (struct sockaddr *)&addr, addrlen);
> 	if (err) {
> 		pr_warn("kernel_bind error, err=%d", err);
> 		goto out;
> 	}
>
> -	err = kernel_listen(ssock, backlog);
> +	err = kernel_listen(entry->lsk, backlog);
> 	if (err) {
> 		pr_warn("kernel_listen error, err=%d", err);
> 		goto out;
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v2 1/2] mptcp: use entry->lsk directly

[Geliang Tang]

On Thu, Oct 27, 2022 at 12:50:03PM -0700, Mat Martineau wrote:
> On Wed, 26 Oct 2022, Geliang Tang wrote:
> 
> > This patch drops msk and ssock in mptcp_pm_nl_create_listen_socket(),
> > use entry->lsk directly instead of ssock.
> > 
> 
> Hi Geliang -
> 
> The change looks good, but the old code looks buggy. Seems like it would be
> trying to re-bind the nmpc socket each time a listener socket was created.
> Is this a fix for mptcp-net in your opinion?

Yes, this should be a fix. Please add this into the -net section when
merging it.

Thanks,
-Geliang

> 
> 
> Thanks,
> 
> Mat
> 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> > net/mptcp/pm_netlink.c | 18 ++----------------
> > 1 file changed, 2 insertions(+), 16 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 9813ed0fde9b..52a5847b9d74 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -992,8 +992,6 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
> > {
> > 	int addrlen = sizeof(struct sockaddr_in);
> > 	struct sockaddr_storage addr;
> > -	struct mptcp_sock *msk;
> > -	struct socket *ssock;
> > 	int backlog = 1024;
> > 	int err;
> > 
> > @@ -1002,30 +1000,18 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
> > 	if (err)
> > 		return err;
> > 
> > -	msk = mptcp_sk(entry->lsk->sk);
> > -	if (!msk) {
> > -		err = -EINVAL;
> > -		goto out;
> > -	}
> > -
> > -	ssock = __mptcp_nmpc_socket(msk);
> > -	if (!ssock) {
> > -		err = -EINVAL;
> > -		goto out;
> > -	}
> > -
> > 	mptcp_info2sockaddr(&entry->addr, &addr, entry->addr.family);
> > #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > 	if (entry->addr.family == AF_INET6)
> > 		addrlen = sizeof(struct sockaddr_in6);
> > #endif
> > -	err = kernel_bind(ssock, (struct sockaddr *)&addr, addrlen);
> > +	err = kernel_bind(entry->lsk, (struct sockaddr *)&addr, addrlen);
> > 	if (err) {
> > 		pr_warn("kernel_bind error, err=%d", err);
> > 		goto out;
> > 	}
> > 
> > -	err = kernel_listen(ssock, backlog);
> > +	err = kernel_listen(entry->lsk, backlog);
> > 	if (err) {
> > 		pr_warn("kernel_listen error, err=%d", err);
> > 		goto out;
> > -- 
> > 2.35.3
> > 
> > 
> > 
> 
> --
> Mat Martineau
> Intel

Re: [PATCH mptcp-next v2 1/2] mptcp: use entry->lsk directly

[Matthieu Baerts]

Hi Geliang, Mat,

On 28/10/2022 13:12, Geliang Tang wrote:
> On Thu, Oct 27, 2022 at 12:50:03PM -0700, Mat Martineau wrote:
>> On Wed, 26 Oct 2022, Geliang Tang wrote:
>>
>>> This patch drops msk and ssock in mptcp_pm_nl_create_listen_socket(),
>>> use entry->lsk directly instead of ssock.
>>>
>>
>> Hi Geliang -
>>
>> The change looks good, but the old code looks buggy. Seems like it would be
>> trying to re-bind the nmpc socket each time a listener socket was created.
>> Is this a fix for mptcp-net in your opinion?
> 
> Yes, this should be a fix. Please add this into the -net section when
> merging it.

Only this patch 1/2 should go to -net I suppose and with this tag, right?

Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")

Also, I think the commit message should be improved to describe the
issue that is being fixed. Currently, it sounds like it is a simple
refactoring to me :)

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