[MPTCP][PATCH v3 mptcp-next 0/5] fullmesh path manager support

[MPTCP][PATCH v3 mptcp-next 0/5] fullmesh path manager support

[Geliang Tang]

From: Geliang Tang <geliangtang@xiaomi.com>

v3:
 - the in-kernel fullmesh path manager has been dropped from this
   patchset, only keep the fullmesh flag support code.

v2:
 - Implement the fullmesh mode as an extension to the netlink PM, not a
   standalone PM as Paolo suggested.
 - drop duplicate code.
 - add a new per endpoint flag MPTCP_PM_ADDR_FLAG_FULLMESH.

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

Geliang Tang (5):
  mptcp: remote addresses fullmesh
  mptcp: local addresses fullmesh
  selftests: mptcp: set and print the fullmesh flag
  selftests: mptcp: add fullmesh testcases
  selftests: mptcp: delete uncontinuous removing ids

 include/uapi/linux/mptcp.h                    |  1 +
 net/mptcp/pm_netlink.c                        | 81 +++++++++++++++++--
 .../testing/selftests/net/mptcp/mptcp_join.sh | 59 +++++++++++---
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 11 ++-
 4 files changed, 136 insertions(+), 16 deletions(-)

-- 
2.31.1

[MPTCP][PATCH v3 mptcp-next 1/5] mptcp: remote addresses fullmesh

[Geliang Tang]

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added and managed a new per endpoint flag, named
MPTCP_PM_ADDR_FLAG_FULLMESH.

In mptcp_pm_create_subflow_or_signal_addr(), if such flag is set, instead
of:

        remote_address((struct sock_common *)sk, &remote);

fill a temporary allocated array of all known remote address. After
releaseing the pm lock loop on such array and create a subflow for each
remote address from the given local.

Note that the we could still use an array even for non 'fullmesh'
endpoint: with a single entry corresponding to the primary MPC subflow
remote address.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 include/uapi/linux/mptcp.h |  1 +
 net/mptcp/pm_netlink.c     | 46 +++++++++++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 7b05f7102321..f66038b9551f 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -73,6 +73,7 @@ enum {
 #define MPTCP_PM_ADDR_FLAG_SIGNAL			(1 << 0)
 #define MPTCP_PM_ADDR_FLAG_SUBFLOW			(1 << 1)
 #define MPTCP_PM_ADDR_FLAG_BACKUP			(1 << 2)
+#define MPTCP_PM_ADDR_FLAG_FULLMESH			(1 << 3)
 
 enum {
 	MPTCP_PM_CMD_UNSPEC,
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index ba0e1d71504d..f57db5b9a50f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -158,6 +158,27 @@ static bool lookup_subflow_by_daddr(const struct list_head *list,
 	return false;
 }
 
+static bool lookup_subflow_by_addrs(const struct list_head *list,
+				    struct mptcp_addr_info *saddr,
+				    struct mptcp_addr_info *daddr)
+{
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_addr_info local, remote;
+	struct sock_common *skc;
+
+	list_for_each_entry(subflow, list, node) {
+		skc = (struct sock_common *)mptcp_subflow_tcp_sock(subflow);
+
+		local_address(skc, &local);
+		remote_address(skc, &remote);
+		if (addresses_equal(&local, saddr, saddr->port) &&
+		    addresses_equal(&remote, daddr, daddr->port))
+			return true;
+	}
+
+	return false;
+}
+
 static struct mptcp_pm_addr_entry *
 select_local_address(const struct pm_nl_pernet *pernet,
 		     struct mptcp_sock *msk)
@@ -455,15 +476,34 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 	    !READ_ONCE(msk->pm.remote_deny_join_id0)) {
 		local = select_local_address(pernet, msk);
 		if (local) {
+			struct mptcp_addr_info addrs[MPTCP_PM_ADDR_MAX];
 			struct mptcp_addr_info remote = { 0 };
+			int i, n = 0;
 
 			msk->pm.local_addr_used++;
 			msk->pm.subflows++;
 			check_work_pending(msk);
-			remote_address((struct sock_common *)sk, &remote);
+			if (!(local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH)) {
+				remote_address((struct sock_common *)sk, &remote);
+				addrs[n++] = remote;
+			} else {
+				struct mptcp_subflow_context *subflow;
+
+				mptcp_for_each_subflow(msk, subflow) {
+					struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+					remote_address((struct sock_common *)ssk, &remote);
+					if (!lookup_subflow_by_addrs(&msk->conn_list,
+								     &local->addr, &remote)) {
+						addrs[n++] = remote;
+					}
+				}
+			}
 			spin_unlock_bh(&msk->pm.lock);
-			__mptcp_subflow_connect(sk, &local->addr, &remote,
-						local->flags, local->ifindex);
+			for (i = 0; i < n; i++) {
+				__mptcp_subflow_connect(sk, &local->addr, &addrs[i],
+							local->flags, local->ifindex);
+			}
 			spin_lock_bh(&msk->pm.lock);
 			return;
 		}
-- 
2.31.1

[MPTCP][PATCH v3 mptcp-next 2/5] mptcp: local addresses fullmesh

[Geliang Tang]

From: Geliang Tang <geliangtang@xiaomi.com>

In mptcp_pm_nl_add_addr_received(), fill a temporary allocate array of
all local address corresponding to the fullmesh endpoint. If such array
is empty, keep the current behavior.

Elsewhere loop on such array and create a subflow for each local address
towards the given remote address

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 net/mptcp/pm_netlink.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index f57db5b9a50f..a03e8fd3a584 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -526,12 +526,17 @@ static void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk)
 
 static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 {
+	struct mptcp_pm_addr_entry entries[MPTCP_PM_ADDR_MAX];
 	struct sock *sk = (struct sock *)msk;
+	struct mptcp_pm_addr_entry *entry;
 	unsigned int add_addr_accept_max;
+	struct mptcp_pm_addr_entry local;
 	struct mptcp_addr_info remote;
-	struct mptcp_addr_info local;
+	struct pm_nl_pernet *pernet;
 	unsigned int subflows_max;
+	int i, n = 0;
 
+	pernet = net_generic(sock_net(sk), pm_nl_pernet_id);
 	add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
 	subflows_max = mptcp_pm_get_subflows_max(msk);
 
@@ -555,10 +560,34 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 	if (!remote.port)
 		remote.port = sk->sk_dport;
 	memset(&local, 0, sizeof(local));
-	local.family = remote.family;
+	local.addr.family = remote.family;
 
+	entries[n++] = local;
+	rcu_read_lock();
+	__mptcp_flush_join_list(msk);
+	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
+		if (!(entry->flags & MPTCP_PM_ADDR_FLAG_FULLMESH))
+			continue;
+
+		if (entry->addr.family != sk->sk_family) {
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+			if ((entry->addr.family == AF_INET &&
+			     !ipv6_addr_v4mapped(&sk->sk_v6_daddr)) ||
+			    (sk->sk_family == AF_INET &&
+			     !ipv6_addr_v4mapped(&entry->addr.addr6)))
+#endif
+				continue;
+		}
+
+		if (!lookup_subflow_by_addrs(&msk->conn_list, &entry->addr, &remote))
+			entries[n++] = *entry;
+	}
+	rcu_read_unlock();
 	spin_unlock_bh(&msk->pm.lock);
-	__mptcp_subflow_connect(sk, &local, &remote, 0, 0);
+	for (i = 0; i < n; i++) {
+		__mptcp_subflow_connect(sk, &entries[i].addr, &remote,
+					entries[i].flags, entries[i].ifindex);
+	}
 	spin_lock_bh(&msk->pm.lock);
 
 add_addr_echo:
-- 
2.31.1

[MPTCP][PATCH v3 mptcp-next 3/5] selftests: mptcp: set and print the fullmesh flag

[Geliang Tang]

From: Geliang Tang <geliangtang@xiaomi.com>

This patch dealt with the MPTCP_PM_ADDR_FLAG_FULLMESH flag in add_addr()
and print_addr(), to set and print out the fullmesh flag.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 115decfdc1ef..9a5a3b3d20f6 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -25,7 +25,7 @@
 static void syntax(char *argv[])
 {
 	fprintf(stderr, "%s add|get|set|del|flush|dump|accept [<args>]\n", argv[0]);
-	fprintf(stderr, "\tadd [flags signal|subflow|backup] [id <nr>] [dev <name>] <ip>\n");
+	fprintf(stderr, "\tadd [flags signal|subflow|backup|fullmesh] [id <nr>] [dev <name>] <ip>\n");
 	fprintf(stderr, "\tdel <id> [<ip>]\n");
 	fprintf(stderr, "\tget <id>\n");
 	fprintf(stderr, "\tset <ip> [flags backup|nobackup]\n");
@@ -236,6 +236,8 @@ int add_addr(int fd, int pm_family, int argc, char *argv[])
 					flags |= MPTCP_PM_ADDR_FLAG_SIGNAL;
 				else if (!strcmp(tok, "backup"))
 					flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
+				else if (!strcmp(tok, "fullmesh"))
+					flags |= MPTCP_PM_ADDR_FLAG_FULLMESH;
 				else
 					error(1, errno,
 					      "unknown flag %s", argv[arg]);
@@ -422,6 +424,13 @@ static void print_addr(struct rtattr *attrs, int len)
 					printf(",");
 			}
 
+			if (flags & MPTCP_PM_ADDR_FLAG_FULLMESH) {
+				printf("fullmesh");
+				flags &= ~MPTCP_PM_ADDR_FLAG_FULLMESH;
+				if (flags)
+					printf(",");
+			}
+
 			/* bump unknown flags, if any */
 			if (flags)
 				printf("0x%x", flags);
-- 
2.31.1

[MPTCP][PATCH v3 mptcp-next 4/5] selftests: mptcp: add fullmesh testcases

[Geliang Tang]

From: Geliang Tang <geliangtang@xiaomi.com>

This patch added the testcases for the fullmesh address flag of the path
manager.

Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 49 +++++++++++++++++--
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 937e861e9490..887b94eab64e 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -325,7 +325,13 @@ do_transfer()
 	cpid=$!
 
 	if [ $addr_nr_ns1 -gt 0 ]; then
-		let add_nr_ns1=addr_nr_ns1
+		if [ $addr_nr_ns1 -gt 10 ]; then
+			let add_nr_ns1=addr_nr_ns1-10
+			flags=signal,fullmesh
+		else
+			let add_nr_ns1=addr_nr_ns1
+			flags=signal
+		fi
 		counter=2
 		sleep 1
 		while [ $add_nr_ns1 -gt 0 ]; do
@@ -335,7 +341,7 @@ do_transfer()
 			else
 				addr="10.0.$counter.1"
 			fi
-			ip netns exec $ns1 ./pm_nl_ctl add $addr flags signal
+			ip netns exec $ns1 ./pm_nl_ctl add $addr flags $flags
 			let counter+=1
 			let add_nr_ns1-=1
 		done
@@ -367,7 +373,13 @@ do_transfer()
 	fi
 
 	if [ $addr_nr_ns2 -gt 0 ]; then
-		let add_nr_ns2=addr_nr_ns2
+		if [ $addr_nr_ns2 -gt 10 ]; then
+			let add_nr_ns2=addr_nr_ns2-10
+			flags=subflow,fullmesh
+		else
+			let add_nr_ns2=addr_nr_ns2
+			flags=subflow
+		fi
 		counter=3
 		sleep 1
 		while [ $add_nr_ns2 -gt 0 ]; do
@@ -377,7 +389,7 @@ do_transfer()
 			else
 				addr="10.0.$counter.2"
 			fi
-			ip netns exec $ns2 ./pm_nl_ctl add $addr flags subflow
+			ip netns exec $ns2 ./pm_nl_ctl add $addr flags $flags
 			let counter+=1
 			let add_nr_ns2-=1
 		done
@@ -1697,6 +1709,28 @@ deny_join_id0_tests()
 	chk_join_nr "subflow and address allow join id0 2" 1 1 1
 }
 
+fullmesh_tests()
+{
+	# fullmesh 1
+	reset
+	ip netns exec $ns1 ./pm_nl_ctl limits 8 8
+	ip netns exec $ns2 ./pm_nl_ctl limits 8 8
+	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow,fullmesh
+	run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow
+	chk_join_nr "signal address fullmesh 1" 3 3 3
+	chk_add_nr 1 1
+
+	# fullmesh 2
+	reset
+	ip netns exec $ns1 ./pm_nl_ctl limits 8 8
+	ip netns exec $ns2 ./pm_nl_ctl limits 8 8
+	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
+	run_tests $ns1 $ns2 10.0.1.1 0 0 11 slow
+	chk_join_nr "signal address fullmesh 2" 3 3 3
+	chk_add_nr 1 1
+}
+
 all_tests()
 {
 	subflows_tests
@@ -1712,6 +1746,7 @@ all_tests()
 	syncookies_tests
 	checksum_tests
 	deny_join_id0_tests
+	fullmesh_tests
 }
 
 usage()
@@ -1730,6 +1765,7 @@ usage()
 	echo "  -k syncookies_tests"
 	echo "  -S checksum_tests"
 	echo "  -d deny_join_id0_tests"
+	echo "  -m fullmesh_tests"
 	echo "  -c capture pcap files"
 	echo "  -C enable data checksum"
 	echo "  -h help"
@@ -1765,7 +1801,7 @@ if [ $do_all_tests -eq 1 ]; then
 	exit $ret
 fi
 
-while getopts 'fsltra64bpkdchCS' opt; do
+while getopts 'fsltra64bpkdmchCS' opt; do
 	case $opt in
 		f)
 			subflows_tests
@@ -1806,6 +1842,9 @@ while getopts 'fsltra64bpkdchCS' opt; do
 		d)
 			deny_join_id0_tests
 			;;
+		m)
+			fullmesh_tests
+			;;
 		c)
 			;;
 		C)
-- 
2.31.1

[MPTCP][PATCH v3 mptcp-next 5/5] selftests: mptcp: delete uncontinuous removing ids

[Geliang Tang]

From: Geliang Tang <geliangtang@xiaomi.com>

The removing addresses testcases can only deal with the continuous ids.
This patch added the uncontinuous removing ids support.

Fixes: f87744ad42446 ("selftests: mptcp: set addr id for removing testcases")
Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 887b94eab64e..4d2d5a6843c0 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -350,17 +350,18 @@ do_transfer()
 		let rm_nr_ns1=-addr_nr_ns1
 		if [ $rm_nr_ns1 -lt 8 ]; then
 			counter=1
+			pos=1
 			dump=(`ip netns exec ${listener_ns} ./pm_nl_ctl dump`)
 			if [ ${#dump[@]} -gt 0 ]; then
-				id=${dump[1]}
 				sleep 1
 
 				while [ $counter -le $rm_nr_ns1 ]
 				do
+					id=${dump[$pos]}
 					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
 					sleep 1
 					let counter+=1
-					let id+=1
+					let pos+=5
 				done
 			fi
 		elif [ $rm_nr_ns1 -eq 8 ]; then
@@ -398,17 +399,18 @@ do_transfer()
 		let rm_nr_ns2=-addr_nr_ns2
 		if [ $rm_nr_ns2 -lt 8 ]; then
 			counter=1
+			pos=1
 			dump=(`ip netns exec ${connector_ns} ./pm_nl_ctl dump`)
 			if [ ${#dump[@]} -gt 0 ]; then
-				id=${dump[1]}
 				sleep 1
 
 				while [ $counter -le $rm_nr_ns2 ]
 				do
+					id=${dump[$pos]}
 					ip netns exec ${connector_ns} ./pm_nl_ctl del $id
 					sleep 1
 					let counter+=1
-					let id+=1
+					let pos+=5
 				done
 			fi
 		elif [ $rm_nr_ns2 -eq 8 ]; then
-- 
2.31.1

Re: [MPTCP][PATCH v3 mptcp-next 3/5] selftests: mptcp: set and print the fullmesh flag

[Paolo Abeni]

On Mon, 2021-07-26 at 12:12 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch dealt with the MPTCP_PM_ADDR_FLAG_FULLMESH flag in add_addr()
> and print_addr(), to set and print out the fullmesh flag.

I'm unsure we should enforce no 'SIGNAL' flag is set on the relevant
endpoint. Likely not required...

/P

Re: [MPTCP][PATCH v3 mptcp-next 3/5] selftests: mptcp: set and print the fullmesh flag

[Paolo Abeni]

On Mon, 2021-07-26 at 10:25 +0200, Paolo Abeni wrote:
> On Mon, 2021-07-26 at 12:12 +0800, Geliang Tang wrote:
> > From: Geliang Tang <geliangtang@xiaomi.com>
> > 
> > This patch dealt with the MPTCP_PM_ADDR_FLAG_FULLMESH flag in add_addr()
> > and print_addr(), to set and print out the fullmesh flag.
> 
> I'm unsure we should enforce no 'SIGNAL' flag is set on the relevant
> endpoint. Likely not required...

Whoops this comment really matters for patch 1/5, sorry for the
confusion!

/P

Re: [MPTCP][PATCH v3 mptcp-next 2/5] mptcp: local addresses fullmesh

[Paolo Abeni]

On Mon, 2021-07-26 at 12:12 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> In mptcp_pm_nl_add_addr_received(), fill a temporary allocate array of
> all local address corresponding to the fullmesh endpoint. If such array
> is empty, keep the current behavior.
> 
> Elsewhere loop on such array and create a subflow for each local address
> towards the given remote address
> 
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/pm_netlink.c | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index f57db5b9a50f..a03e8fd3a584 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -526,12 +526,17 @@ static void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk)
>  
>  static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  {
> +	struct mptcp_pm_addr_entry entries[MPTCP_PM_ADDR_MAX];
>  	struct sock *sk = (struct sock *)msk;
> +	struct mptcp_pm_addr_entry *entry;
>  	unsigned int add_addr_accept_max;
> +	struct mptcp_pm_addr_entry local;
>  	struct mptcp_addr_info remote;
> -	struct mptcp_addr_info local;
> +	struct pm_nl_pernet *pernet;
>  	unsigned int subflows_max;
> +	int i, n = 0;
>  
> +	pernet = net_generic(sock_net(sk), pm_nl_pernet_id);
>  	add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
>  	subflows_max = mptcp_pm_get_subflows_max(msk);
>  
> @@ -555,10 +560,34 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  	if (!remote.port)
>  		remote.port = sk->sk_dport;
>  	memset(&local, 0, sizeof(local));
> -	local.family = remote.family;
> +	local.addr.family = remote.family;
>  
> +	entries[n++] = local;

If I read correctly, I think this entry should be added only if the
full-mesh selection below find no other entries. Otherwise, e.g. in the
simple scenario with 2 locals address, 3 subflows will be created.

> +	rcu_read_lock();
> +	__mptcp_flush_join_list(msk);
> +	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
> +		if (!(entry->flags & MPTCP_PM_ADDR_FLAG_FULLMESH))
> +			continue;
> +
> +		if (entry->addr.family != sk->sk_family) {
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +			if ((entry->addr.family == AF_INET &&
> +			     !ipv6_addr_v4mapped(&sk->sk_v6_daddr)) ||
> +			    (sk->sk_family == AF_INET &&
> +			     !ipv6_addr_v4mapped(&entry->addr.addr6)))
> +#endif
> +				continue;
> +		}
> +
> +		if (!lookup_subflow_by_addrs(&msk->conn_list, &entry->addr, &remote))
> +			entries[n++] = *entry;

I guess the loop should stop when:

 n + <number of already existing subflows> = mptcp_pm_get_subflows_max()

Even the previous patch need a similar check.

Additional minor nit, what about encapsualting the above chunk in a new
helper - fill_source_addresses_vec() or something similar? It gets the
current pm and net as input arguments, the address vector as an output
arg, and returns the number of addresses filled into the array
(enforcing the above constraint).

This also applies to the previous patch.

Cheers,

Paolo

Re: [MPTCP][PATCH v3 mptcp-next 2/5] mptcp: local addresses fullmesh

[Geliang Tang]

Paolo Abeni <pabeni@redhat.com> 于2021年7月26日周一 下午4:24写道：
>
> On Mon, 2021-07-26 at 12:12 +0800, Geliang Tang wrote:
> > From: Geliang Tang <geliangtang@xiaomi.com>
> >
> > In mptcp_pm_nl_add_addr_received(), fill a temporary allocate array of
> > all local address corresponding to the fullmesh endpoint. If such array
> > is empty, keep the current behavior.
> >
> > Elsewhere loop on such array and create a subflow for each local address
> > towards the given remote address
> >
> > Suggested-by: Paolo Abeni <pabeni@redhat.com>
> > Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> > ---
> >  net/mptcp/pm_netlink.c | 35 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index f57db5b9a50f..a03e8fd3a584 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -526,12 +526,17 @@ static void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk)
> >
> >  static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
> >  {
> > +     struct mptcp_pm_addr_entry entries[MPTCP_PM_ADDR_MAX];
> >       struct sock *sk = (struct sock *)msk;
> > +     struct mptcp_pm_addr_entry *entry;
> >       unsigned int add_addr_accept_max;
> > +     struct mptcp_pm_addr_entry local;
> >       struct mptcp_addr_info remote;
> > -     struct mptcp_addr_info local;
> > +     struct pm_nl_pernet *pernet;
> >       unsigned int subflows_max;
> > +     int i, n = 0;
> >
> > +     pernet = net_generic(sock_net(sk), pm_nl_pernet_id);
> >       add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
> >       subflows_max = mptcp_pm_get_subflows_max(msk);
> >
> > @@ -555,10 +560,34 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
> >       if (!remote.port)
> >               remote.port = sk->sk_dport;
> >       memset(&local, 0, sizeof(local));
> > -     local.family = remote.family;
> > +     local.addr.family = remote.family;
> >
> > +     entries[n++] = local;
>
> If I read correctly, I think this entry should be added only if the
> full-mesh selection below find no other entries. Otherwise, e.g. in the
> simple scenario with 2 locals address, 3 subflows will be created.

local is the id 0 address, this address should always be used, and it's
not on the local_addr_list. So I think we should put local in the array
all the time, no matter fullmesh or not.

Do you agree?

Thanks,
-Geliang


>
> > +     rcu_read_lock();
> > +     __mptcp_flush_join_list(msk);
> > +     list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
> > +             if (!(entry->flags & MPTCP_PM_ADDR_FLAG_FULLMESH))
> > +                     continue;
> > +
> > +             if (entry->addr.family != sk->sk_family) {
> > +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > +                     if ((entry->addr.family == AF_INET &&
> > +                          !ipv6_addr_v4mapped(&sk->sk_v6_daddr)) ||
> > +                         (sk->sk_family == AF_INET &&
> > +                          !ipv6_addr_v4mapped(&entry->addr.addr6)))
> > +#endif
> > +                             continue;
> > +             }
> > +
> > +             if (!lookup_subflow_by_addrs(&msk->conn_list, &entry->addr, &remote))
> > +                     entries[n++] = *entry;
>
> I guess the loop should stop when:
>
>  n + <number of already existing subflows> = mptcp_pm_get_subflows_max()
>
> Even the previous patch need a similar check.
>
> Additional minor nit, what about encapsualting the above chunk in a new
> helper - fill_source_addresses_vec() or something similar? It gets the
> current pm and net as input arguments, the address vector as an output
> arg, and returns the number of addresses filled into the array
> (enforcing the above constraint).
>
> This also applies to the previous patch.
>
> Cheers,
>
> Paolo
>

Re: [MPTCP][PATCH v3 mptcp-next 2/5] mptcp: local addresses fullmesh

[Paolo Abeni]

On Mon, 2021-07-26 at 16:32 +0800, Geliang Tang wrote:
> Paolo Abeni <pabeni@redhat.com> 于2021年7月26日周一 下午4:24写道：
> > On Mon, 2021-07-26 at 12:12 +0800, Geliang Tang wrote:
> > > From: Geliang Tang <geliangtang@xiaomi.com>
> > > 
> > > In mptcp_pm_nl_add_addr_received(), fill a temporary allocate array of
> > > all local address corresponding to the fullmesh endpoint. If such array
> > > is empty, keep the current behavior.
> > > 
> > > Elsewhere loop on such array and create a subflow for each local address
> > > towards the given remote address
> > > 
> > > Suggested-by: Paolo Abeni <pabeni@redhat.com>
> > > Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> > > ---
> > >  net/mptcp/pm_netlink.c | 35 ++++++++++++++++++++++++++++++++---
> > >  1 file changed, 32 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index f57db5b9a50f..a03e8fd3a584 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > > @@ -526,12 +526,17 @@ static void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk)
> > > 
> > >  static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
> > >  {
> > > +     struct mptcp_pm_addr_entry entries[MPTCP_PM_ADDR_MAX];
> > >       struct sock *sk = (struct sock *)msk;
> > > +     struct mptcp_pm_addr_entry *entry;
> > >       unsigned int add_addr_accept_max;
> > > +     struct mptcp_pm_addr_entry local;
> > >       struct mptcp_addr_info remote;
> > > -     struct mptcp_addr_info local;
> > > +     struct pm_nl_pernet *pernet;
> > >       unsigned int subflows_max;
> > > +     int i, n = 0;
> > > 
> > > +     pernet = net_generic(sock_net(sk), pm_nl_pernet_id);
> > >       add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
> > >       subflows_max = mptcp_pm_get_subflows_max(msk);
> > > 
> > > @@ -555,10 +560,34 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
> > >       if (!remote.port)
> > >               remote.port = sk->sk_dport;
> > >       memset(&local, 0, sizeof(local));
> > > -     local.family = remote.family;
> > > +     local.addr.family = remote.family;
> > > 
> > > +     entries[n++] = local;
> > 
> > If I read correctly, I think this entry should be added only if the
> > full-mesh selection below find no other entries. Otherwise, e.g. in the
> > simple scenario with 2 locals address, 3 subflows will be created.
> 
> local is the id 0 address, this address should always be used, and it's
> not on the local_addr_list. 

Uhm... if the user-space configures correctly the mptcp endpoints,
every relevant address should be in local_addr_list.

e.g.

The client has 2 IPs:

192.168.255.2/24 dev eth0
192.168.254.2/24 dev eth1

The user-space should create 2 endpoints with full-mesh flags, one for
each of the above IPs, at addresses creation time.

When the mptcp socket is established, it's first subflow will use as
local address one of such IPs/endpoints.

If mptcp_pm_nl_add_addr_received() adds to the 'entries' array an
'IPADDRANY' address, and than the 2 addresses above, the client will
end-up creating 3 subflows towards the newly signaled address, instead
of the expected 2.

WDYT?

Paolo