[PATCH v2 mptcp-next 0/3] mptcp: add support for mixed v4/v6

[PATCH v2 mptcp-next 0/3] mptcp: add support for mixed v4/v6

[Paolo Abeni]

The core MPTCP implementation is just a few bits of to support such
feature, we just need to allow specifying the subflow family explicitly
(patch 1/3) and remove the artificial constraint currently enforcing
no mixed subflow in place (patch 2/3)

Some self-tests added on patch 3/3

Merry Xmas ;)

v1 -> v2:
 - only allow mixing family for ipv6 mptcp socket with no IPV6_ONLY
   option (all the functional changes in 2/3 plus self-tests adaptation)

Paolo Abeni (3):
  mptcp: explicitly specify sock family at subflow creation time
  mptcp: let the in kernel PM use mixed ipv4 and ipv6 addresses
  selftests: mptcp: add test-cases for mixed v4/v6 subflows

 net/mptcp/pm_netlink.c                        | 68 ++++++++++++-------
 net/mptcp/protocol.c                          |  2 +-
 net/mptcp/protocol.h                          |  3 +-
 net/mptcp/subflow.c                           |  9 +--
 .../testing/selftests/net/mptcp/mptcp_join.sh | 53 ++++++++++++---
 5 files changed, 94 insertions(+), 41 deletions(-)

-- 
2.38.1

[PATCH v2 mptcp-next 1/3] mptcp: explicitly specify sock family at subflow creation time

[Paolo Abeni]

Let the caller specify the to-be-created subflow family. No
functional change intended, will be leveraged by the next
patch.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 2 +-
 net/mptcp/protocol.h | 3 ++-
 net/mptcp/subflow.c  | 9 +++++----
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 5e813580c394..f08ef9ca242a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -98,7 +98,7 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
 	struct socket *ssock;
 	int err;
 
-	err = mptcp_subflow_create_socket(sk, &ssock);
+	err = mptcp_subflow_create_socket(sk, sk->sk_family, &ssock);
 	if (err)
 		return err;
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3cd3270407b0..fcce8f03c1b5 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -643,7 +643,8 @@ bool mptcp_addresses_equal(const struct mptcp_addr_info *a,
 /* called with sk socket lock held */
 int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 			    const struct mptcp_addr_info *remote);
-int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock);
+int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
+				struct socket **new_sock);
 void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
 			 struct sockaddr_storage *addr,
 			 unsigned short family);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index bd387d4b5a38..ec54413fb31f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1547,7 +1547,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	if (!mptcp_is_fully_established(sk))
 		goto err_out;
 
-	err = mptcp_subflow_create_socket(sk, &sf);
+	err = mptcp_subflow_create_socket(sk, loc->family, &sf);
 	if (err)
 		goto err_out;
 
@@ -1660,7 +1660,9 @@ static void mptcp_subflow_ops_undo_override(struct sock *ssk)
 #endif
 		ssk->sk_prot = &tcp_prot;
 }
-int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock)
+
+int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
+				struct socket **new_sock)
 {
 	struct mptcp_subflow_context *subflow;
 	struct net *net = sock_net(sk);
@@ -1673,8 +1675,7 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock)
 	if (unlikely(!sk->sk_socket))
 		return -EINVAL;
 
-	err = sock_create_kern(net, sk->sk_family, SOCK_STREAM, IPPROTO_TCP,
-			       &sf);
+	err = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP, &sf);
 	if (err)
 		return err;
 
-- 
2.38.1

[PATCH v2 mptcp-next 2/3] mptcp: let the in kernel PM use mixed ipv4 and ipv6 addresses

[Paolo Abeni]

Currently the in kernel PM arbitrary enforces that created subflow's
family must match the main MPTCP socket. The RFC allow instead for
mixing IPv4 and IPv6 subflows.

This patch changes the in kernel PM logic to create subflows matching
the currently selected source (or destination) address. IPv4 sockets
can pick only IPv4 addresses, while IPv6 sockets not restricted to
V6ONLY can pick either IPv4 and IPv6 addresses, effectively
allowing the creation of IPv4 and IPv6 subflows under the same msk.

While at that, factor out a new helper for address family matching,
taking care of ipv4 vs ipv4-mapped-ipv6.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/269
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c | 68 ++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 26 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index f2a43e13bacd..23c4e74312f7 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -152,7 +152,6 @@ static struct mptcp_pm_addr_entry *
 select_local_address(const struct pm_nl_pernet *pernet,
 		     const struct mptcp_sock *msk)
 {
-	const struct sock *sk = (const struct sock *)msk;
 	struct mptcp_pm_addr_entry *entry, *ret = NULL;
 
 	msk_owned_by_me(msk);
@@ -165,16 +164,6 @@ select_local_address(const struct pm_nl_pernet *pernet,
 		if (!test_bit(entry->addr.id, msk->pm.id_avail_bitmap))
 			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;
-		}
-
 		ret = entry;
 		break;
 	}
@@ -420,10 +409,34 @@ static bool lookup_address_in_vec(const struct mptcp_addr_info *addrs, unsigned
 	return false;
 }
 
+/* if sk is ipv4 or ipv6_only allows only same-family local and remote addresses,
+ * otherwise allow any matching local/remote pair
+ */
+static bool addr_families_match(const struct sock *sk,
+				const struct mptcp_addr_info *loc,
+				const struct mptcp_addr_info *remote)
+{
+	if (sk->sk_family == loc->family && sk->sk_family == remote->family)
+		return true;
+
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+	if (sk->sk_family == AF_INET || sk->sk_ipv6only)
+		return false;
+
+	if (loc->family == remote->family ||
+	    (loc->family == AF_INET && ipv6_addr_v4mapped(&remote->addr6)) ||
+	    (remote->family == AF_INET && ipv6_addr_v4mapped(&loc->addr6)))
+		return true;
+#endif
+	return false;
+}
+
 /* Fill all the remote addresses into the array addrs[],
  * and return the array size.
  */
-static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullmesh,
+static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk,
+					      struct mptcp_addr_info *local,
+					      bool fullmesh,
 					      struct mptcp_addr_info *addrs)
 {
 	bool deny_id0 = READ_ONCE(msk->pm.remote_deny_join_id0);
@@ -443,6 +456,9 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
 		if (deny_id0)
 			return 0;
 
+		if (!addr_families_match(sk, local, &remote))
+			return 0;
+
 		msk->pm.subflows++;
 		addrs[i++] = remote;
 	} else {
@@ -453,6 +469,9 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
 			if (deny_id0 && !addrs[i].id)
 				continue;
 
+			if (!addr_families_match(sk, local, &addrs[i]))
+				continue;
+
 			if (!lookup_address_in_vec(addrs, i, &addrs[i]) &&
 			    msk->pm.subflows < subflows_max) {
 				msk->pm.subflows++;
@@ -600,10 +619,10 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 		fullmesh = !!(local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH);
 
 		msk->pm.local_addr_used++;
-		nr = fill_remote_addresses_vec(msk, fullmesh, addrs);
-		if (nr)
-			__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
+		nr = fill_remote_addresses_vec(msk, &local->addr, fullmesh, addrs);
+		__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
 		spin_unlock_bh(&msk->pm.lock);
+
 		for (i = 0; i < nr; i++)
 			__mptcp_subflow_connect(sk, &local->addr, &addrs[i]);
 		spin_lock_bh(&msk->pm.lock);
@@ -625,6 +644,7 @@ static void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk)
  * and return the array size.
  */
 static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
+					     struct mptcp_addr_info *remote,
 					     struct mptcp_addr_info *addrs)
 {
 	struct sock *sk = (struct sock *)msk;
@@ -642,15 +662,8 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
 		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 (!addr_families_match(sk, &entry->addr, remote))
+			continue;
 
 		if (msk->pm.subflows < subflows_max) {
 			msk->pm.subflows++;
@@ -664,7 +677,10 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
 	 */
 	if (!i) {
 		memset(&local, 0, sizeof(local));
-		local.family = msk->pm.remote.family;
+		local.family = remote->family;
+
+		if (!addr_families_match(sk, &local, remote))
+			return 0;
 
 		msk->pm.subflows++;
 		addrs[i++] = local;
@@ -703,7 +719,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 	/* connect to the specified remote address, using whatever
 	 * local address the routing configuration will pick.
 	 */
-	nr = fill_local_addresses_vec(msk, addrs);
+	nr = fill_local_addresses_vec(msk, &remote, addrs);
 
 	msk->pm.add_addr_accepted++;
 	if (msk->pm.add_addr_accepted >= add_addr_accept_max ||
-- 
2.38.1

[PATCH v2 mptcp-next 3/3] selftests: mptcp: add test-cases for mixed v4/v6 subflows

[Paolo Abeni]

Note that we can't guess anymore the listener family based on the
client target address; use always IPv6.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 53 +++++++++++++++----
 1 file changed, 44 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index d11d3d566608..387abdcec011 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -774,24 +774,17 @@ do_transfer()
 		addr_nr_ns2=${addr_nr_ns2:9}
 	fi
 
-	local local_addr
-	if is_v6 "${connect_addr}"; then
-		local_addr="::"
-	else
-		local_addr="0.0.0.0"
-	fi
-
 	extra_srv_args="$extra_args $extra_srv_args"
 	if [ "$test_link_fail" -gt 1 ];then
 		timeout ${timeout_test} \
 			ip netns exec ${listener_ns} \
 				./mptcp_connect -t ${timeout_poll} -l -p $port -s ${srv_proto} \
-					$extra_srv_args ${local_addr} < "$sinfail" > "$sout" &
+					$extra_srv_args "::" < "$sinfail" > "$sout" &
 	else
 		timeout ${timeout_test} \
 			ip netns exec ${listener_ns} \
 				./mptcp_connect -t ${timeout_poll} -l -p $port -s ${srv_proto} \
-					$extra_srv_args ${local_addr} < "$sin" > "$sout" &
+					$extra_srv_args "::" < "$sin" > "$sout" &
 	fi
 	local spid=$!
 
@@ -2448,6 +2441,47 @@ v4mapped_tests()
 	fi
 }
 
+mixed_tests()
+{
+	if reset "IPv4 sockets do not use IPv6 addresses"; then
+		pm_nl_set_limits $ns1 0 1
+		pm_nl_set_limits $ns2 1 1
+		pm_nl_add_endpoint $ns1 dead:beef:2::1 flags signal
+		run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow
+		chk_join_nr 0 0 0
+	fi
+
+	# Need an IPv6 mptcp socket to allow subflows of both families
+	if reset "simult IPv4 and IPv6 subflows"; then
+		pm_nl_set_limits $ns1 0 1
+		pm_nl_set_limits $ns2 1 1
+		pm_nl_add_endpoint $ns1 10.0.1.1 flags signal
+		run_tests $ns1 $ns2 dead:beef:2::1 0 0 0 slow
+		chk_join_nr 1 1 1
+	fi
+
+	# cross families subflows will not be created even in fullmesh mode
+	if reset "simult IPv4 and IPv6 subflows, fullmesh 1x1"; then
+		pm_nl_set_limits $ns1 0 4
+		pm_nl_set_limits $ns2 1 4
+		pm_nl_add_endpoint $ns2 dead:beef:2::2 flags subflow,fullmesh
+		pm_nl_add_endpoint $ns1 10.0.1.1 flags signal
+		run_tests $ns1 $ns2 dead:beef:2::1 0 0 0 slow
+		chk_join_nr 1 1 1
+	fi
+
+	# fullmesh still tries to create all the possibly subflows with
+	# matching family
+	if reset "simult IPv4 and IPv6 subflows, fullmesh 2x2"; then
+		pm_nl_set_limits $ns1 0 4
+		pm_nl_set_limits $ns2 2 4
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		pm_nl_add_endpoint $ns1 dead:beef:2::1 flags signal
+		run_tests $ns1 $ns2 dead:beef:1::1 0 0 fullmesh_1 slow
+		chk_join_nr 4 4 4
+	fi
+}
+
 backup_tests()
 {
 	# single subflow, backup
@@ -3120,6 +3154,7 @@ all_tests_sorted=(
 	a@add_tests
 	6@ipv6_tests
 	4@v4mapped_tests
+	M@mixed_tests
 	b@backup_tests
 	p@add_addr_ports_tests
 	k@syncookies_tests
-- 
2.38.1

Re: selftests: mptcp: add test-cases for mixed v4/v6 subflows: Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6349295318728704
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6349295318728704/summary/summary.txt

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

- KVM Validation: Script error! ❓:
  - :
  - Task: https://cirrus-ci.com/task/5504870388596736
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5504870388596736/summary/summary.txt

- KVM Validation: Script error! ❓:
  - :
  - Task: https://cirrus-ci.com/task/6067820342018048
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6067820342018048/summary/summary.txt

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


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 v2 mptcp-next 2/3] mptcp: let the in kernel PM use mixed ipv4 and ipv6 addresses

[Matthieu Baerts]

Hi Paolo,

On 23/12/2022 20:00, Paolo Abeni wrote:
> Currently the in kernel PM arbitrary enforces that created subflow's
> family must match the main MPTCP socket. The RFC allow instead for
> mixing IPv4 and IPv6 subflows.
> 
> This patch changes the in kernel PM logic to create subflows matching
> the currently selected source (or destination) address. IPv4 sockets
> can pick only IPv4 addresses, while IPv6 sockets not restricted to
> V6ONLY can pick either IPv4 and IPv6 addresses, effectively
> allowing the creation of IPv4 and IPv6 subflows under the same msk.
> 
> While at that, factor out a new helper for address family matching,
> taking care of ipv4 vs ipv4-mapped-ipv6.

Thank you for the v2!

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/269
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/pm_netlink.c | 68 ++++++++++++++++++++++++++----------------
>  1 file changed, 42 insertions(+), 26 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index f2a43e13bacd..23c4e74312f7 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c

(...)

> @@ -420,10 +409,34 @@ static bool lookup_address_in_vec(const struct mptcp_addr_info *addrs, unsigned
>  	return false;
>  }
>  
> +/* if sk is ipv4 or ipv6_only allows only same-family local and remote addresses,
> + * otherwise allow any matching local/remote pair
> + */
> +static bool addr_families_match(const struct sock *sk,
> +				const struct mptcp_addr_info *loc,
> +				const struct mptcp_addr_info *remote)
> +{
> +	if (sk->sk_family == loc->family && sk->sk_family == remote->family)

Do we need to handle this case: MPTCP socket is in v6, local and remote
are also in v6 but one or the two of them are in fact v4 mapped in v6.
If the two are v4 mapped, we need to checked for sk_ipv6only as well
which increase the number of checks to do :-/

> +		return true;
> +
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +	if (sk->sk_family == AF_INET || sk->sk_ipv6only)
> +		return false;

This breaks one test in mptcp_join.sh selftests (55: single subflow
v6-map-v4). This test creates an AF_INET MPTCP socket and adds a subflow
endpoint using a v6 mapped form address: "::ffff:10.0.3.2".

If we want to imitate TCP, we should treat v4 mapped addresses in v6 as
v4 and then allow AF_INET6 if it is in fact a v4.

I suggest to do this modification:

----------------- 8< -----------------
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 23c4e74312f7..22dfbaf988a5 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -414,21 +414,32 @@ static bool lookup_address_in_vec(const struct mptcp_addr_info *addrs, unsigned
>   */
>  static bool addr_families_match(const struct sock *sk,
>                                 const struct mptcp_addr_info *loc,
> -                               const struct mptcp_addr_info *remote)
> +                               const struct mptcp_addr_info *rem)
>  {
> -       if (sk->sk_family == loc->family && sk->sk_family == remote->family)
> -               return true;
> +       bool mptcp_is_v4 = sk->sk_family == AF_INET;
>  
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -       if (sk->sk_family == AF_INET || sk->sk_ipv6only)
> +       bool loc_is_v4 = loc->family == AF_INET || ipv6_addr_v4mapped(&loc->addr6);
> +       bool rem_is_v4 = rem->family == AF_INET || ipv6_addr_v4mapped(&rem->addr6);
> +
> +       /* v4 only */
> +       if (mptcp_is_v4) {
> +               if (loc_is_v4 && rem_is_v4)
> +                       return true;
>                 return false;
> +       }
>  
> -       if (loc->family == remote->family ||
> -           (loc->family == AF_INET && ipv6_addr_v4mapped(&remote->addr6)) ||
> -           (remote->family == AF_INET && ipv6_addr_v4mapped(&loc->addr6)))
> -               return true;
> +       /* v6 only */
> +       if (sk->sk_ipv6only) {
> +               if (!loc_is_v4 && !rem_is_v4)
> +                       return true;
> +               return false;
> +       }
> +
> +       return loc_is_v4 == rem_is_v4;
> +#else
> +       return mptcp_is_v4 && loc->family == AF_INET && rem->family && AF_INET;
>  #endif
> -       return false;
>  }
>  
>  /* Fill all the remote addresses into the array addrs[],
----------------- 8< -----------------

Note that if the kernel is compiled without CONFIG_MPTCP_IPV6, we don't
really need to check if the MPTCP socket is in v4 but I keep it here to
continue to use 'sk' and avoid a warning from the compiler.

> +
> +	if (loc->family == remote->family ||
> +	    (loc->family == AF_INET && ipv6_addr_v4mapped(&remote->addr6)) ||
> +	    (remote->family == AF_INET && ipv6_addr_v4mapped(&loc->addr6)))
> +		return true;
> +#endif
> +	return false;
> +}
> +
>  /* Fill all the remote addresses into the array addrs[],
>   * and return the array size.
>   */
> -static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullmesh,
> +static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk,
> +					      struct mptcp_addr_info *local,
> +					      bool fullmesh,
>  					      struct mptcp_addr_info *addrs)
>  {
>  	bool deny_id0 = READ_ONCE(msk->pm.remote_deny_join_id0);
> @@ -443,6 +456,9 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
>  		if (deny_id0)
>  			return 0;
>  
> +		if (!addr_families_match(sk, local, &remote))
> +			return 0;
> +
>  		msk->pm.subflows++;
>  		addrs[i++] = remote;
>  	} else {
> @@ -453,6 +469,9 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
>  			if (deny_id0 && !addrs[i].id)
>  				continue;
>  
> +			if (!addr_families_match(sk, local, &addrs[i]))
> +				continue;
> +
>  			if (!lookup_address_in_vec(addrs, i, &addrs[i]) &&
>  			    msk->pm.subflows < subflows_max) {
>  				msk->pm.subflows++;
> @@ -600,10 +619,10 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>  		fullmesh = !!(local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH);
>  
>  		msk->pm.local_addr_used++;
> -		nr = fill_remote_addresses_vec(msk, fullmesh, addrs);
> -		if (nr)
> -			__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
> +		nr = fill_remote_addresses_vec(msk, &local->addr, fullmesh, addrs);
> +		__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
>  		spin_unlock_bh(&msk->pm.lock);
> +
>  		for (i = 0; i < nr; i++)
>  			__mptcp_subflow_connect(sk, &local->addr, &addrs[i]);
>  		spin_lock_bh(&msk->pm.lock);
> @@ -625,6 +644,7 @@ static void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk)
>   * and return the array size.
>   */
>  static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
> +					     struct mptcp_addr_info *remote,
>  					     struct mptcp_addr_info *addrs)
>  {
>  	struct sock *sk = (struct sock *)msk;
> @@ -642,15 +662,8 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>  		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 (!addr_families_match(sk, &entry->addr, remote))
> +			continue;
>  
>  		if (msk->pm.subflows < subflows_max) {
>  			msk->pm.subflows++;
> @@ -664,7 +677,10 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>  	 */
>  	if (!i) {
>  		memset(&local, 0, sizeof(local));
> -		local.family = msk->pm.remote.family;
> +		local.family = remote->family;

Here, you need to take into account v4mapped addresses not to break
mptcp_join selftest (56 signal address v6-map-v4):

----------------- 8< -----------------
> @@ -649,7 +655,6 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>  {
>         struct sock *sk = (struct sock *)msk;
>         struct mptcp_pm_addr_entry *entry;
> -       struct mptcp_addr_info local;
>         struct pm_nl_pernet *pernet;
>         unsigned int subflows_max;
>         int i = 0;
> @@ -676,8 +681,12 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>          * 'IPADDRANY' local address
>          */
>         if (!i) {
> +               struct mptcp_addr_info local;
> +
>                 memset(&local, 0, sizeof(local));
> -               local.family = remote->family;
> +               local.family = remote->family == AF_INET6 &&
> +                              ipv6_addr_v4mapped(&remote->addr6) ? AF_INET :
> +                              remote->family;
>  
>                 if (!addr_families_match(sk, &local, remote))
>                         return 0;
----------------- 8< -----------------

> +
> +		if (!addr_families_match(sk, &local, remote))
> +			return 0;

Now that this function can return 0 ... (see below)

>  
>  		msk->pm.subflows++;
>  		addrs[i++] = local;
> @@ -703,7 +719,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  	/* connect to the specified remote address, using whatever
>  	 * local address the routing configuration will pick.
>  	 */
> -	nr = fill_local_addresses_vec(msk, addrs);
> +	nr = fill_local_addresses_vec(msk, &remote, addrs);

... does it still make sense to increment add_addr_accepted counter here
below if nr == 0? We got an ADD_ADDR but we cannot do anything with it
so we didn't really "accepted" it. WDYT?

>  
>  	msk->pm.add_addr_accepted++;
>  	if (msk->pm.add_addr_accepted >= add_addr_accept_max ||

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

Re: [PATCH v2 mptcp-next 1/3] mptcp: explicitly specify sock family at subflow creation time

[Matthieu Baerts]

Hi Paolo,

On 23/12/2022 20:00, Paolo Abeni wrote:
> Let the caller specify the to-be-created subflow family. No
> functional change intended, will be leveraged by the next
> patch.

If I'm not mistaken, there is a functional change ... (see below)

(...)

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index bd387d4b5a38..ec54413fb31f 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1547,7 +1547,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
>  	if (!mptcp_is_fully_established(sk))
>  		goto err_out;
>  
> -	err = mptcp_subflow_create_socket(sk, &sf);
> +	err = mptcp_subflow_create_socket(sk, loc->family, &sf);

This seems to allow the userspace PM to create subflows for different
family than the one of the MPTCP socket.

A detail but maybe this should be mentioned in the commit message?

(and I suppose the userspace PM should be modified to check subflows can
be created but that's for an additional commit)

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