[PATCH v5 mptcp-next 0/4] mptcp: more self-tests improvements

[PATCH v5 mptcp-next 0/4] mptcp: more self-tests improvements

[Paolo Abeni]

This iteration tries to address the feedback from Mat on v4.

patch 1/4 is new and tries to address the problem behind issues/225 with
stricted RFC compliance
patch 2 and 3 clean-up endpoint management as per past discussion
patch 4 introduce new self-tests, as deserved the previous patch

Paolo Abeni (4):
  mptcp: more careful RM_ADDR generation
  mptcp: introduce implicit endpoints
  mptcp: strict local address ID selection.
  selftests: mptcp: add implicit endpoint test case

 include/uapi/linux/mptcp.h                    |   1 +
 net/mptcp/pm_netlink.c                        |  84 ++++++------
 net/mptcp/protocol.c                          |   3 +
 net/mptcp/protocol.h                          |   3 +-
 net/mptcp/subflow.c                           |  67 ++++++++--
 .../testing/selftests/net/mptcp/mptcp_join.sh | 121 +++++++++++++++++-
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c |   7 +
 7 files changed, 234 insertions(+), 52 deletions(-)

-- 
2.34.1

[PATCH v5 mptcp-next 1/4] mptcp: more careful RM_ADDR generation

[Paolo Abeni]

The in-kernel MPTCP path manager, when processing the MPTCP_PM_CMD_FLUSH_ADDR
command, generates RM_ADDR events for each known local address. While that
is allowed by the RFC, it makes unpredictable the exact number of RM_ADDR
generated when both ends flush the PM addresses.

This change restricts the RM_ADDR generation to previously explicitly
announced addresses, and adjust the expected results in a bunch of related
self-tests.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c                          | 10 ++++------
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  6 +++---
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 56f5603c10f2..619746611110 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1458,14 +1458,12 @@ static void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
 
 	list_for_each_entry(entry, rm_list, list) {
 		if (lookup_subflow_by_saddr(&msk->conn_list, &entry->addr) &&
-		    alist.nr < MPTCP_RM_IDS_MAX &&
-		    slist.nr < MPTCP_RM_IDS_MAX) {
-			alist.ids[alist.nr++] = entry->addr.id;
+		    slist.nr < MPTCP_RM_IDS_MAX)
 			slist.ids[slist.nr++] = entry->addr.id;
-		} else if (remove_anno_list_by_saddr(msk, &entry->addr) &&
-			 alist.nr < MPTCP_RM_IDS_MAX) {
+
+		if (remove_anno_list_by_saddr(msk, &entry->addr) &&
+		    alist.nr < MPTCP_RM_IDS_MAX)
 			alist.ids[alist.nr++] = entry->addr.id;
-		}
 	}
 
 	if (alist.nr) {
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index b48b11714817..6f16337409e3 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1577,7 +1577,7 @@ remove_tests()
 	run_tests $ns1 $ns2 10.0.1.1 0 -8 -8 slow
 	chk_join_nr "flush subflows and signal" 3 3 3
 	chk_add_nr 1 1
-	chk_rm_nr 2 2
+	chk_rm_nr 1 1 invert
 
 	# subflows flush
 	reset
@@ -1588,7 +1588,7 @@ remove_tests()
 	pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow
 	run_tests $ns1 $ns2 10.0.1.1 0 -8 -8 slow
 	chk_join_nr "flush subflows" 3 3 3
-	chk_rm_nr 3 3
+	chk_rm_nr 0 3
 
 	# addresses flush
 	reset
@@ -1884,7 +1884,7 @@ add_addr_ports_tests()
 	run_tests $ns1 $ns2 10.0.1.1 0 -8 -2 slow
 	chk_join_nr "flush subflows and signal with port" 3 3 3
 	chk_add_nr 1 1
-	chk_rm_nr 2 2
+	chk_rm_nr 1 1 invert
 
 	# multiple addresses with port
 	reset
-- 
2.34.1

[PATCH v5 mptcp-next 2/4] mptcp: introduce implicit endpoints

[Paolo Abeni]

In some edge scenarios, an MPTCP subflows can use a local address
mapped by a "implicit" endpoint created by the in-kernel path manager.

Such endpoints presence can be confusing, as it's creation is hard
to track and will prevent the later endpoint creation from the user-space
using the same address.

Define a new endpoint flag to mark implicit endpoints and allow the
user-space to replace implicit them with user-provided data at endpoint
creation time.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v4 -> v5:
 - fixed a few book-keeping issues detected by the paired self-test
---
 include/uapi/linux/mptcp.h                    |  1 +
 net/mptcp/pm_netlink.c                        | 61 +++++++++++++------
 .../testing/selftests/net/mptcp/mptcp_join.sh |  4 +-
 3 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index f106a3941cdf..9690efedb5fa 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -81,6 +81,7 @@ enum {
 #define MPTCP_PM_ADDR_FLAG_SUBFLOW			(1 << 1)
 #define MPTCP_PM_ADDR_FLAG_BACKUP			(1 << 2)
 #define MPTCP_PM_ADDR_FLAG_FULLMESH			(1 << 3)
+#define MPTCP_PM_ADDR_FLAG_IMPLICIT			(1 << 4)
 
 enum {
 	MPTCP_PM_CMD_UNSPEC,
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 619746611110..3bbc5f9b1983 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -877,10 +877,18 @@ static bool address_use_port(struct mptcp_pm_addr_entry *entry)
 		MPTCP_PM_ADDR_FLAG_SIGNAL;
 }
 
+/* caller must ensure the RCU grace period is already elapsed */
+static void __mptcp_pm_release_addr_entry(struct mptcp_pm_addr_entry *entry)
+{
+	if (entry->lsk)
+		sock_release(entry->lsk);
+	kfree(entry);
+}
+
 static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 					     struct mptcp_pm_addr_entry *entry)
 {
-	struct mptcp_pm_addr_entry *cur;
+	struct mptcp_pm_addr_entry *cur, *del_entry = NULL;
 	unsigned int addr_max;
 	int ret = -EINVAL;
 
@@ -901,8 +909,22 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 	list_for_each_entry(cur, &pernet->local_addr_list, list) {
 		if (addresses_equal(&cur->addr, &entry->addr,
 				    address_use_port(entry) &&
-				    address_use_port(cur)))
-			goto out;
+				    address_use_port(cur))) {
+			/* allow replacing the exiting endpoint only if such
+			 * endpoint is an implicit one and the user-space
+			 * did not provide an endpoint id
+			 */
+			if (!(cur->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT))
+				goto out;
+			if (entry->addr.id)
+				goto out;
+
+			pernet->addrs--;
+			entry->addr.id = cur->addr.id;
+			list_del_rcu(&cur->list);
+			del_entry = cur;
+			break;
+		}
 	}
 
 	if (!entry->addr.id) {
@@ -938,6 +960,12 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 
 out:
 	spin_unlock_bh(&pernet->lock);
+
+	/* just replaced an existing entry, free it */
+	if (del_entry) {
+		synchronize_rcu();
+		__mptcp_pm_release_addr_entry(del_entry);
+	}
 	return ret;
 }
 
@@ -1036,7 +1064,7 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 	entry->addr.id = 0;
 	entry->addr.port = 0;
 	entry->ifindex = 0;
-	entry->flags = 0;
+	entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
 	entry->lsk = NULL;
 	ret = mptcp_pm_nl_append_new_local_addr(pernet, entry);
 	if (ret < 0)
@@ -1238,6 +1266,11 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	}
 
+	if (addr.flags & MPTCP_PM_ADDR_FLAG_IMPLICIT) {
+		GENL_SET_ERR_MSG(info, "can't create IMPLICIT endpoint");
+		return -EINVAL;
+	}
+
 	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry) {
 		GENL_SET_ERR_MSG(info, "can't allocate addr");
@@ -1322,11 +1355,12 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
 }
 
 static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
-						   struct mptcp_addr_info *addr)
+						   const struct mptcp_pm_addr_entry *entry)
 {
-	struct mptcp_sock *msk;
-	long s_slot = 0, s_num = 0;
+	const struct mptcp_addr_info *addr = &entry->addr;
 	struct mptcp_rm_list list = { .nr = 0 };
+	long s_slot = 0, s_num = 0;
+	struct mptcp_sock *msk;
 
 	pr_debug("remove_id=%d", addr->id);
 
@@ -1346,7 +1380,8 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 
 		lock_sock(sk);
 		remove_subflow = lookup_subflow_by_saddr(&msk->conn_list, addr);
-		mptcp_pm_remove_anno_addr(msk, addr, remove_subflow);
+		mptcp_pm_remove_anno_addr(msk, addr, remove_subflow &&
+					  !(entry->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT));
 		if (remove_subflow)
 			mptcp_pm_remove_subflow(msk, &list);
 		release_sock(sk);
@@ -1359,14 +1394,6 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 	return 0;
 }
 
-/* caller must ensure the RCU grace period is already elapsed */
-static void __mptcp_pm_release_addr_entry(struct mptcp_pm_addr_entry *entry)
-{
-	if (entry->lsk)
-		sock_release(entry->lsk);
-	kfree(entry);
-}
-
 static int mptcp_nl_remove_id_zero_address(struct net *net,
 					   struct mptcp_addr_info *addr)
 {
@@ -1443,7 +1470,7 @@ static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
 	__clear_bit(entry->addr.id, pernet->id_bitmap);
 	spin_unlock_bh(&pernet->lock);
 
-	mptcp_nl_remove_subflow_and_signal_addr(sock_net(skb->sk), &entry->addr);
+	mptcp_nl_remove_subflow_and_signal_addr(sock_net(skb->sk), entry);
 	synchronize_rcu();
 	__mptcp_pm_release_addr_entry(entry);
 
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 6f16337409e3..913866d5d570 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1819,7 +1819,7 @@ backup_tests()
 	run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow backup
 	chk_join_nr "single address, backup" 1 1 1
 	chk_add_nr 1 1
-	chk_prio_nr 1 0
+	chk_prio_nr 1 1
 
 	# single address with port, backup
 	reset
@@ -1829,7 +1829,7 @@ backup_tests()
 	run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow backup
 	chk_join_nr "single address with port, backup" 1 1 1
 	chk_add_nr 1 1
-	chk_prio_nr 1 0
+	chk_prio_nr 1 1
 }
 
 add_addr_ports_tests()
-- 
2.34.1

[PATCH v5 mptcp-next 3/4] mptcp: strict local address ID selection.

[Paolo Abeni]

The address ID selection for MPJ subflows created in response
to incoming ADD_ADDR option is currently unreliable: it happens
at MPJ socket creation time, when the local address could be
unknown.

Additionally, if the no local endpoint is available for the local
address, a new dummy endpoint is created, confusing the user-land.

This change refactor the code to move the address ID seleciton inside
the rebuild_header() helper, when the local address eventually
selected by the route lookup is finally known. If the address used
is not mapped by any endpoint - and thus can't be advertised/removed
pick the id 0 instead of allocate a new endpoint.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 v3 -> v4:
- really create endpoints in mptcp_pm_get_local_id() - Mat

 v2 -> v3:
- keep creating dummy endpoint

 v1 -> v2:
- hopefully fix build issue with ipv6 disabled
- avoid looking-up multiple times the local_id for req sockets
- factor-out an helper for local_id initialization

RFC -> v1:
- don't bail if ID lookup fails, use 0 instead
---
 net/mptcp/pm_netlink.c | 13 --------
 net/mptcp/protocol.c   |  3 ++
 net/mptcp/protocol.h   |  3 +-
 net/mptcp/subflow.c    | 67 ++++++++++++++++++++++++++++++++++++------
 4 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 3bbc5f9b1983..a0e7d5b7e22f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -83,16 +83,6 @@ static bool addresses_equal(const struct mptcp_addr_info *a,
 	return a->port == b->port;
 }
 
-static bool address_zero(const struct mptcp_addr_info *addr)
-{
-	struct mptcp_addr_info zero;
-
-	memset(&zero, 0, sizeof(zero));
-	zero.family = addr->family;
-
-	return addresses_equal(addr, &zero, true);
-}
-
 static void local_address(const struct sock_common *skc,
 			  struct mptcp_addr_info *addr)
 {
@@ -1039,9 +1029,6 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 	if (addresses_equal(&msk_local, &skc_local, false))
 		return 0;
 
-	if (address_zero(&skc_local))
-		return 0;
-
 	pernet = net_generic(sock_net((struct sock *)msk), pm_nl_pernet_id);
 
 	rcu_read_lock();
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 4599bde215b2..bf5af6bf8756 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -117,6 +117,9 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
 	list_add(&subflow->node, &msk->conn_list);
 	sock_hold(ssock->sk);
 	subflow->request_mptcp = 1;
+
+	/* This is the first subflow, always with id 0 */
+	subflow->local_id_valid = 1;
 	mptcp_sock_graft(msk->first, sk->sk_socket);
 
 	return 0;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 18ca0248c084..c8bada4537e2 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -453,7 +453,8 @@ struct mptcp_subflow_context {
 		rx_eof : 1,
 		can_ack : 1,        /* only after processing the remote a key */
 		disposable : 1,	    /* ctx can be free at ulp release time */
-		stale : 1;	    /* unable to snd/rcv data, do not use for xmit */
+		stale : 1,	    /* unable to snd/rcv data, do not use for xmit */
+		local_id_valid : 1; /* local_id is correctly initialized */
 	enum mptcp_data_avail data_avail;
 	u32	remote_nonce;
 	u64	thmac;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index e727d838da0e..c05c19f92532 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -481,6 +481,51 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 	mptcp_subflow_reset(sk);
 }
 
+static void subflow_set_local_id(struct mptcp_subflow_context *subflow, int local_id)
+{
+	subflow->local_id = local_id;
+	subflow->local_id_valid = 1;
+}
+
+static int subflow_chk_local_id(struct sock *sk)
+{
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+	int err;
+
+	if (likely(subflow->local_id_valid))
+		return 0;
+
+	err = mptcp_pm_get_local_id(msk, (struct sock_common *)sk);
+	if (err < 0)
+		return err;
+
+	subflow_set_local_id(subflow, err);
+	return 0;
+}
+
+static int subflow_rebuild_header(struct sock *sk)
+{
+	int err = subflow_chk_local_id(sk);
+
+	if (unlikely(err < 0))
+		return err;
+
+	return inet_sk_rebuild_header(sk);
+}
+
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+static int subflow_v6_rebuild_header(struct sock *sk)
+{
+	int err = subflow_chk_local_id(sk);
+
+	if (unlikely(err < 0))
+		return err;
+
+	return inet6_sk_rebuild_header(sk);
+}
+#endif
+
 struct request_sock_ops mptcp_subflow_request_sock_ops;
 static struct tcp_request_sock_ops subflow_request_sock_ipv4_ops __ro_after_init;
 
@@ -1403,13 +1448,8 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 		get_random_bytes(&subflow->local_nonce, sizeof(u32));
 	} while (!subflow->local_nonce);
 
-	if (!local_id) {
-		err = mptcp_pm_get_local_id(msk, (struct sock_common *)ssk);
-		if (err < 0)
-			goto failed;
-
-		local_id = err;
-	}
+	if (local_id)
+		subflow_set_local_id(subflow, local_id);
 
 	mptcp_pm_get_flags_and_ifindex_by_id(sock_net(sk), local_id,
 					     &flags, &ifindex);
@@ -1434,7 +1474,6 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	pr_debug("msk=%p remote_token=%u local_id=%d remote_id=%d", msk,
 		 remote_token, local_id, remote_id);
 	subflow->remote_token = remote_token;
-	subflow->local_id = local_id;
 	subflow->remote_id = remote_id;
 	subflow->request_join = 1;
 	subflow->request_bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
@@ -1734,15 +1773,22 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		new_ctx->token = subflow_req->token;
 		new_ctx->ssn_offset = subflow_req->ssn_offset;
 		new_ctx->idsn = subflow_req->idsn;
+
+		/* this is the first subflow, id is always 0 */
+		new_ctx->local_id_valid = 1;
 	} else if (subflow_req->mp_join) {
 		new_ctx->ssn_offset = subflow_req->ssn_offset;
 		new_ctx->mp_join = 1;
 		new_ctx->fully_established = 1;
 		new_ctx->backup = subflow_req->backup;
-		new_ctx->local_id = subflow_req->local_id;
 		new_ctx->remote_id = subflow_req->remote_id;
 		new_ctx->token = subflow_req->token;
 		new_ctx->thmac = subflow_req->thmac;
+
+		/* the subflow req id is valid, fetched via subflow_check_req()
+		 * and subflow_token_join_request()
+		 */
+		subflow_set_local_id(new_ctx, subflow_req->local_id);
 	}
 }
 
@@ -1795,6 +1841,7 @@ void __init mptcp_subflow_init(void)
 	subflow_specific.conn_request = subflow_v4_conn_request;
 	subflow_specific.syn_recv_sock = subflow_syn_recv_sock;
 	subflow_specific.sk_rx_dst_set = subflow_finish_connect;
+	subflow_specific.rebuild_header = subflow_rebuild_header;
 
 	tcp_prot_override = tcp_prot;
 	tcp_prot_override.release_cb = tcp_release_cb_override;
@@ -1807,6 +1854,7 @@ void __init mptcp_subflow_init(void)
 	subflow_v6_specific.conn_request = subflow_v6_conn_request;
 	subflow_v6_specific.syn_recv_sock = subflow_syn_recv_sock;
 	subflow_v6_specific.sk_rx_dst_set = subflow_finish_connect;
+	subflow_v6_specific.rebuild_header = subflow_v6_rebuild_header;
 
 	subflow_v6m_specific = subflow_v6_specific;
 	subflow_v6m_specific.queue_xmit = ipv4_specific.queue_xmit;
@@ -1814,6 +1862,7 @@ void __init mptcp_subflow_init(void)
 	subflow_v6m_specific.net_header_len = ipv4_specific.net_header_len;
 	subflow_v6m_specific.mtu_reduced = ipv4_specific.mtu_reduced;
 	subflow_v6m_specific.net_frag_header_len = 0;
+	subflow_v6m_specific.rebuild_header = subflow_rebuild_header;
 
 	tcpv6_prot_override = tcpv6_prot;
 	tcpv6_prot_override.release_cb = tcp_release_cb_override;
-- 
2.34.1

[PATCH v5 mptcp-next 4/4] selftests: mptcp: add implicit endpoint test case

[Paolo Abeni]

Ensure implicit endpoint are created when expected and
that the user-space can update them

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 111 +++++++++++++++++-
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c |   7 ++
 2 files changed, 117 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 913866d5d570..9414a1c62fe0 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -413,6 +413,76 @@ pm_nl_change_endpoint()
 	fi
 }
 
+pm_nl_check_endpoint()
+{
+	local line expected_line
+	local msg="$1"
+	local addr=$3
+	local ns=$2
+	local _flags=""
+	local flags
+	local _port
+	local port
+	local dev
+	local _id
+	local id
+
+	TEST_COUNT=$((TEST_COUNT + 1))
+	printf "%03u %-40s" "$TEST_COUNT" "$msg"
+
+	shift 3
+	while [ -n "$1" ]; do
+		if [ $1 = "flags" ]; then
+			_flags=$2
+			[ ! -z $_flags ]; flags="flags $_flags"
+			shift
+		elif [ $1 = "dev" ]; then
+			[ ! -z $2 ]; dev="dev $1"
+			shift
+		elif [ $1 = "id" ]; then
+			_id=$2
+			[ ! -z $_id ]; id="id $_id"
+			shift
+		elif [ $1 = "port" ]; then
+			_port=$2
+			[ ! -z $_port ]; port=" port $_port"
+			shift
+		fi
+
+		shift
+	done
+
+	if [ -z "$id" ]; then
+		echo "[skip] bad test - missing endpoint id"
+		return
+	fi
+
+	if [ $ip_mptcp -eq 1 ]; then
+		line=$(ip -n $ns mptcp endpoint show $id)
+		# the dump order is: address id flags port dev
+		expected_line="$addr"
+		[ -n "$addr" ] && expected_line="$expected_line $addr"
+		expected_line="$expected_line $id"
+		[ -n "$_flags" ] && expected_line="$expected_line ${_flags//","/" "}"
+		[ -n "$dev" ] && expected_line="$expected_line $dev"
+		[ -n "$port" ] && expected_line="$expected_line $port"
+	else
+		line=$(ip netns exec $ns ./pm_nl_ctl get $_id)
+		# the dump order is: id flags dev address port
+		expected_line="$id"
+		[ -n "$flags" ] && expected_line="$expected_line $flags"
+		[ -n "$dev" ] && expected_line="$expected_line $dev"
+		[ -n "$addr" ] && expected_line="$expected_line $addr"
+		[ -n "$_port" ] && expected_line="$expected_line $_port"
+	fi
+	if [ "$line" = "$expected_line" ]; then
+		echo "[ ok ]"
+	else
+		echo "[fail] expected '$expected_line' found '$line'"
+		ret=1
+	fi
+}
+
 do_transfer()
 {
 	listener_ns="$1"
@@ -2203,6 +2273,41 @@ userspace_tests()
 	chk_rm_nr 0 0
 }
 
+wait_mpj()
+{
+	local ns="${1}"
+	local old_cnt=$(ip netns exec ${ns} nstat -as | grep MPJoinAckRx | awk '{print $2}')
+
+	for i in $(seq 10); do
+		cnt=$(ip netns exec ${ns} nstat -as | grep MPJoinAckRx | awk '{print $2}')
+		[ "$cnt" = "${old_cnt}" ] || break
+		sleep 0.1
+	done
+}
+
+implicit_tests()
+{
+	# userspace pm type prevents add_addr
+	reset
+	pm_nl_set_limits $ns1 2 2
+	pm_nl_set_limits $ns2 2 2
+	pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+	run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow &
+
+	wait_mpj $ns1
+	pm_nl_check_endpoint "implicit EP creation" \
+		$ns2 10.0.2.2 id 1 flags implicit
+
+	pm_nl_add_endpoint $ns2 10.0.2.2 id 33
+	pm_nl_check_endpoint "implicit EP ID change is prevented" \
+		$ns2 10.0.2.2 id 1 flags implicit
+
+	pm_nl_add_endpoint $ns2 10.0.2.2 flags signal
+	pm_nl_check_endpoint "implicit EP modification is allowed" \
+		$ns2 10.0.2.2 id 1 flags signal
+	wait
+}
+
 all_tests()
 {
 	subflows_tests
@@ -2221,6 +2326,7 @@ all_tests()
 	deny_join_id0_tests
 	fullmesh_tests
 	userspace_tests
+	implicit_tests
 }
 
 # [$1: error message]
@@ -2279,7 +2385,7 @@ if [ $do_all_tests -eq 1 ]; then
 	exit $ret
 fi
 
-while getopts 'fesltra64bpkdmuchCSi' opt; do
+while getopts 'fesltra64bpkdmuchCSiI' opt; do
 	case $opt in
 		f)
 			subflows_tests
@@ -2335,6 +2441,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do
 			;;
 		i)
 			;;
+		I)
+			implicit_tests
+			;;
 		h)
 			usage
 			;;
diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 22a5ec1e128e..a75a68ad652e 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -436,6 +436,13 @@ static void print_addr(struct rtattr *attrs, int len)
 					printf(",");
 			}
 
+			if (flags & MPTCP_PM_ADDR_FLAG_IMPLICIT) {
+				printf("implicit");
+				flags &= ~MPTCP_PM_ADDR_FLAG_IMPLICIT;
+				if (flags)
+					printf(",");
+			}
+
 			/* bump unknown flags, if any */
 			if (flags)
 				printf("0x%x", flags);
-- 
2.34.1

Re: [PATCH v5 mptcp-next 0/4] mptcp: more self-tests improvements

[Mat Martineau]

On Thu, 17 Feb 2022, Paolo Abeni wrote:

> This iteration tries to address the feedback from Mat on v4.
>
> patch 1/4 is new and tries to address the problem behind issues/225 with
> stricted RFC compliance
> patch 2 and 3 clean-up endpoint management as per past discussion
> patch 4 introduce new self-tests, as deserved the previous patch
>

Hi Paolo -

These changes and tests are working well on my system and fits with what 
we discussed on the previous revisions - thanks!

I was taking another look at the RFC and saw this paragraph:

    The MP_JOIN option includes an "Address ID".  This is an identifier
    generated by the sender of the option, used to identify the source
    address of this packet, even if the IP header has been changed in
    transit by a middlebox.  The numeric value of this field is generated
    by the sender and must map uniquely to a source IP address for the
    sending host.  The Address ID allows address removal (Section 3.4.2)
    without needing to know what the source address at the receiver is,
    thus allowing address removal through NATs.  The Address ID also
    allows correlation between new subflow setup attempts and address
    signaling (Section 3.4.1), to prevent setting up duplicate subflows
    on the same path, if an MP_JOIN and ADD_ADDR are sent at the same
    time.

That made me wonder if we are supposed to treat this address ID as a type 
of advertisement which would require us to not reuse id 0? I think the 
answer is "no" because nothing in the REMOVE_ADDR section (3.4.2) seems to 
apply to joins, and it's also pretty permissive about not sending 
REMOVE_ADDR. We also won't be sending REMOVE_ADDR for id 0, so the new 
code seems safe in that regard.

Hopefully you agree with the above and I'm just catching up to what you 
understood about the RFC already :)

That's a long way to say, I think this patch set looks ok for the export 
branch:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>


> Paolo Abeni (4):
>  mptcp: more careful RM_ADDR generation
>  mptcp: introduce implicit endpoints
>  mptcp: strict local address ID selection.
>  selftests: mptcp: add implicit endpoint test case
>
> include/uapi/linux/mptcp.h                    |   1 +
> net/mptcp/pm_netlink.c                        |  84 ++++++------
> net/mptcp/protocol.c                          |   3 +
> net/mptcp/protocol.h                          |   3 +-
> net/mptcp/subflow.c                           |  67 ++++++++--
> .../testing/selftests/net/mptcp/mptcp_join.sh | 121 +++++++++++++++++-
> tools/testing/selftests/net/mptcp/pm_nl_ctl.c |   7 +
> 7 files changed, 234 insertions(+), 52 deletions(-)
>
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH v5 mptcp-next 0/4] mptcp: more self-tests improvements

[Paolo Abeni]

On Thu, 2022-02-17 at 17:35 -0800, Mat Martineau wrote:
> On Thu, 17 Feb 2022, Paolo Abeni wrote:
> 
> > This iteration tries to address the feedback from Mat on v4.
> > 
> > patch 1/4 is new and tries to address the problem behind issues/225 with
> > stricted RFC compliance
> > patch 2 and 3 clean-up endpoint management as per past discussion
> > patch 4 introduce new self-tests, as deserved the previous patch
> > 
> 
> Hi Paolo -
> 
> These changes and tests are working well on my system and fits with what 
> we discussed on the previous revisions - thanks!
> 
> I was taking another look at the RFC and saw this paragraph:
> 
>     The MP_JOIN option includes an "Address ID".  This is an identifier
>     generated by the sender of the option, used to identify the source
>     address of this packet, even if the IP header has been changed in
>     transit by a middlebox.  The numeric value of this field is generated
>     by the sender and must map uniquely to a source IP address for the
>     sending host.  The Address ID allows address removal (Section 3.4.2)
>     without needing to know what the source address at the receiver is,
>     thus allowing address removal through NATs.  The Address ID also
>     allows correlation between new subflow setup attempts and address
>     signaling (Section 3.4.1), to prevent setting up duplicate subflows
>     on the same path, if an MP_JOIN and ADD_ADDR are sent at the same
>     time.
> 
> That made me wonder if we are supposed to treat this address ID as a type 
> of advertisement which would require us to not reuse id 0? I think the 
> answer is "no" because nothing in the REMOVE_ADDR section (3.4.2) seems to 
> apply to joins, and it's also pretty permissive about not sending 
> REMOVE_ADDR. We also won't be sending REMOVE_ADDR for id 0, so the new 
> code seems safe in that regard.

Double checking if I parsed the above correctly. The main
point/question is: which ID should use additional MPJ subflows with the
same source address of the initial MPC subflow? My answer/what the
current code is doing prior and after this patch is 'id 0'. I read the
above as this is also your answer.

Please correct me if I misinterpret something!

> 
> Hopefully you agree with the above and I'm just catching up to what you 
> understood about the RFC already :)
> 
> That's a long way to say, I think this patch set looks ok for the export 
> branch:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Thanks!

/P

Re: [PATCH v5 mptcp-next 0/4] mptcp: more self-tests improvements

[Mat Martineau]

On Fri, 18 Feb 2022, Paolo Abeni wrote:

> On Thu, 2022-02-17 at 17:35 -0800, Mat Martineau wrote:
>> On Thu, 17 Feb 2022, Paolo Abeni wrote:
>>
>>> This iteration tries to address the feedback from Mat on v4.
>>>
>>> patch 1/4 is new and tries to address the problem behind issues/225 with
>>> stricted RFC compliance
>>> patch 2 and 3 clean-up endpoint management as per past discussion
>>> patch 4 introduce new self-tests, as deserved the previous patch
>>>
>>
>> Hi Paolo -
>>
>> These changes and tests are working well on my system and fits with what
>> we discussed on the previous revisions - thanks!
>>
>> I was taking another look at the RFC and saw this paragraph:
>>
>>     The MP_JOIN option includes an "Address ID".  This is an identifier
>>     generated by the sender of the option, used to identify the source
>>     address of this packet, even if the IP header has been changed in
>>     transit by a middlebox.  The numeric value of this field is generated
>>     by the sender and must map uniquely to a source IP address for the
>>     sending host.  The Address ID allows address removal (Section 3.4.2)
>>     without needing to know what the source address at the receiver is,
>>     thus allowing address removal through NATs.  The Address ID also
>>     allows correlation between new subflow setup attempts and address
>>     signaling (Section 3.4.1), to prevent setting up duplicate subflows
>>     on the same path, if an MP_JOIN and ADD_ADDR are sent at the same
>>     time.
>>
>> That made me wonder if we are supposed to treat this address ID as a type
>> of advertisement which would require us to not reuse id 0? I think the
>> answer is "no" because nothing in the REMOVE_ADDR section (3.4.2) seems to
>> apply to joins, and it's also pretty permissive about not sending
>> REMOVE_ADDR. We also won't be sending REMOVE_ADDR for id 0, so the new
>> code seems safe in that regard.
>
> Double checking if I parsed the above correctly. The main
> point/question is: which ID should use additional MPJ subflows with the
> same source address of the initial MPC subflow? My answer/what the
> current code is doing prior and after this patch is 'id 0'. I read the
> above as this is also your answer.
>
> Please correct me if I misinterpret something!
>

That's part of it. I also wasn't sure if the MPJ syn could also be sent 
with addr id 0 if the kernel picked some other interface for sending that 
syn that had not been configured in the path manager - a corner case that 
I thought had been mentioned earlier in discussion of this patch series.

If the outgoing MPJ only has addr id 0 if the MPC source addr is used then 
that's definitely not any kind of problem.

--
Mat Martineau
Intel

Re: [PATCH v5 mptcp-next 0/4] mptcp: more self-tests improvements

[Matthieu Baerts]

Hi Paolo, Mat,

On 17/02/2022 22:44, Paolo Abeni wrote:
> This iteration tries to address the feedback from Mat on v4.
> 
> patch 1/4 is new and tries to address the problem behind issues/225 with
> stricted RFC compliance
> patch 2 and 3 clean-up endpoint management as per past discussion
> patch 4 introduce new self-tests, as deserved the previous patch

Thank you for the patches and reviews!

Now in our tree (feat. for net-next) with Mat's RvB tags and minor
modifications on the last patch:

- 1c0e3277fab9: mptcp: more careful RM_ADDR generation
- 1908304713ea: mptcp: introduce implicit endpoints
- 25124126a6c0: mptcp: strict local address ID selection
- 7532760df8bc: selftests: mptcp: add implicit endpoint test case
  - With minor modifications (missing local + help usage)
  - https://paste.opendev.org/show/812824
- Results: 78bc7e06c12d..785fa0377037

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220219T171438
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

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

Re: [PATCH v5 mptcp-next 0/4] mptcp: more self-tests improvements

[Paolo Abeni]

On Fri, 2022-02-18 at 12:33 -0800, Mat Martineau wrote:
> On Fri, 18 Feb 2022, Paolo Abeni wrote:
> 
> > On Thu, 2022-02-17 at 17:35 -0800, Mat Martineau wrote:
> > > On Thu, 17 Feb 2022, Paolo Abeni wrote:
> > > 
> > > > This iteration tries to address the feedback from Mat on v4.
> > > > 
> > > > patch 1/4 is new and tries to address the problem behind issues/225 with
> > > > stricted RFC compliance
> > > > patch 2 and 3 clean-up endpoint management as per past discussion
> > > > patch 4 introduce new self-tests, as deserved the previous patch
> > > > 
> > > 
> > > Hi Paolo -
> > > 
> > > These changes and tests are working well on my system and fits with what
> > > we discussed on the previous revisions - thanks!
> > > 
> > > I was taking another look at the RFC and saw this paragraph:
> > > 
> > >     The MP_JOIN option includes an "Address ID".  This is an identifier
> > >     generated by the sender of the option, used to identify the source
> > >     address of this packet, even if the IP header has been changed in
> > >     transit by a middlebox.  The numeric value of this field is generated
> > >     by the sender and must map uniquely to a source IP address for the
> > >     sending host.  The Address ID allows address removal (Section 3.4.2)
> > >     without needing to know what the source address at the receiver is,
> > >     thus allowing address removal through NATs.  The Address ID also
> > >     allows correlation between new subflow setup attempts and address
> > >     signaling (Section 3.4.1), to prevent setting up duplicate subflows
> > >     on the same path, if an MP_JOIN and ADD_ADDR are sent at the same
> > >     time.
> > > 
> > > That made me wonder if we are supposed to treat this address ID as a type
> > > of advertisement which would require us to not reuse id 0? I think the
> > > answer is "no" because nothing in the REMOVE_ADDR section (3.4.2) seems to
> > > apply to joins, and it's also pretty permissive about not sending
> > > REMOVE_ADDR. We also won't be sending REMOVE_ADDR for id 0, so the new
> > > code seems safe in that regard.
> > 
> > Double checking if I parsed the above correctly. The main
> > point/question is: which ID should use additional MPJ subflows with the
> > same source address of the initial MPC subflow? My answer/what the
> > current code is doing prior and after this patch is 'id 0'. I read the
> > above as this is also your answer.
> > 
> > Please correct me if I misinterpret something!
> > 
> 
> That's part of it. I also wasn't sure if the MPJ syn could also be sent 
> with addr id 0 if the kernel picked some other interface for sending that 
> syn that had not been configured in the path manager - a corner case that 
> I thought had been mentioned earlier in discussion of this patch series.

I think now it's much clearer to me, thanks!

> If the outgoing MPJ only has addr id 0 if the MPC source addr is used then 
> that's definitely not any kind of problem.

That is addressed by patch 3/4: prior to such patch, MPJ subflows using
local addresses not mapped by user-defined endpoint would select the 0
ID.

After that patch such subflows will create a new IMPLICIT endpoint at
rebuild_header time will use an unique ID number.

Thanks,

Paolo