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

[PATCH 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 ;)

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                        | 57 ++++++++++---------
 net/mptcp/protocol.c                          |  2 +-
 net/mptcp/protocol.h                          |  3 +-
 net/mptcp/subflow.c                           |  9 +--
 .../testing/selftests/net/mptcp/mptcp_join.sh | 51 ++++++++++++++---
 5 files changed, 80 insertions(+), 42 deletions(-)

-- 
2.38.1

[PATCH 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 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 with no constraint.

This patch changes the in kernel PM logic to create subflows matching
the currently selected source (or destination) address, 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 | 57 ++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index f2a43e13bacd..1d4ba6716a98 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,25 @@ static bool lookup_address_in_vec(const struct mptcp_addr_info *addrs, unsigned
 	return false;
 }
 
+static bool addr_families_match(const struct mptcp_addr_info *a,
+				const struct mptcp_addr_info *b)
+{
+	if (a->family == b->family)
+		return true;
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+	if ((a->family == AF_INET && ipv6_addr_v4mapped(&b->addr6)) ||
+	    (b->family == AF_INET && ipv6_addr_v4mapped(&a->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 +447,9 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
 		if (deny_id0)
 			return 0;
 
+		if (!addr_families_match(local, &remote))
+			return 0;
+
 		msk->pm.subflows++;
 		addrs[i++] = remote;
 	} else {
@@ -453,6 +460,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(local, &addrs[i]))
+				continue;
+
 			if (!lookup_address_in_vec(addrs, i, &addrs[i]) &&
 			    msk->pm.subflows < subflows_max) {
 				msk->pm.subflows++;
@@ -600,10 +610,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,9 +635,9 @@ 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;
 	struct mptcp_pm_addr_entry *entry;
 	struct mptcp_addr_info local;
 	struct pm_nl_pernet *pernet;
@@ -642,15 +652,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(&entry->addr, remote))
+			continue;
 
 		if (msk->pm.subflows < subflows_max) {
 			msk->pm.subflows++;
@@ -664,7 +667,7 @@ 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;
 
 		msk->pm.subflows++;
 		addrs[i++] = local;
@@ -703,7 +706,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 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 | 51 +++++++++++++++----
 1 file changed, 42 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..6ef4787ad244 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,45 @@ v4mapped_tests()
 	fi
 }
 
+mixed_tests()
+{
+	# subflow IPv4-mapped to IPv4-mapped
+	if reset "simult IPv4 and IPv6 subflows, first is IPv4"; 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 1 1 1
+	fi
+
+	# subflow IPv4-mapped to IPv4-mapped
+	if reset "simult IPv4 and IPv6 subflows, first is IPv6"; 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
+
+	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
+
+	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 +3152,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/4957635586293760
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4957635586293760/summary/summary.txt

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

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

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

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


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

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

For more details:

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


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

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

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

[Matthieu Baerts]

Hi Paolo,

On 23/12/2022 13:51, Paolo Abeni wrote:
> 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

Thank you very much for having looked at that!

I just have a couple of questions in the different patches :)

> Merry Xmas ;)

Thank you for this Xmas gift :-)
And Merry Xmas as well!

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

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

[Matthieu Baerts]

Hi Paolo,

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

(...)

> @@ -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);

Mmh, I don't know if we can do that: it means we no longer inherit the
'sk_family'. With the modification, it means if the initial socket is
created in v4 only (AF_INET), we will still be able to create subflows
in IPv6, right?

Somehow, I like the idea because with today's Internet, it is normal for
a client to have both IPv4 and IPv6 addresses. Then we avoid people
creating AF_INET sockets and complaining they cannot have additional
IPv6 subflows.

But on the other hand, there is no more way to force having v4 only
subflows, no?
And I guess listening sockets will have to be created with AF_INET6 to
accept additional v6 subflows.

WDYT?
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

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

[Matthieu Baerts]

Hi Paolo,

Thank you for having looked at these patches!

On 23/12/2022 13:51, 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 with no constraint.
> 
> This patch changes the in kernel PM logic to create subflows matching
> the currently selected source (or destination) address, 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 | 57 ++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index f2a43e13bacd..1d4ba6716a98 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c

(...)

> @@ -600,10 +610,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);

I see that you removed the 'if (nr)' condition: was there a bug before?
If no addresses have been filled, no new subflows will be created below
and this ID is still available.

>  		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);

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

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

[Matthieu Baerts]

Hi Paolo,

On 23/12/2022 13:51, Paolo Abeni wrote:
> 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 | 51 +++++++++++++++----
>  1 file changed, 42 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..6ef4787ad244 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,45 @@ v4mapped_tests()
>  	fi
>  }
>  
> +mixed_tests()
> +{
> +	# subflow IPv4-mapped to IPv4-mapped

(small detail: the comment is not adapted. I can remove it when applying
the patch if no other modifications are needed. Same just below, the
next test)

> +	if reset "simult IPv4 and IPv6 subflows, first is IPv4"; 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

(linked to my question from patch 1/3)

If I'm not mistaken, by default, mptcp_connect will create an MPTCP
socket with AF_INET family if the initial destination is an IPv4
address, no?
Here should we not try to connect to ::ffff:10.0.1.1 instead (or pass -6
option? Maybe not enough)?

Also, I guess we should have an extra test to make sure a v4 only
sockets is not allowing extra subflows in v6, no?

> +		chk_join_nr 1 1 1
> +	fi
> +
> +	# subflow IPv4-mapped to IPv4-mapped
> +	if reset "simult IPv4 and IPv6 subflows, first is IPv6"; 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

Linked to the previous question: that makes me think we properly don't
propagate the IPV6_V6ONLY socket option (sk_ipv6only) to new subflows
(just before the connect). Now that we can connect to v4 addresses, it
might be an issue. Easy to fix with this I guess:

--------------------- 8< ---------------------
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 582ed93bcc8a..9986681aaf40 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -1255,6 +1255,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
>         ssk->sk_priority = sk->sk_priority;
>         ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
>         ssk->sk_incoming_cpu = sk->sk_incoming_cpu;
> +       ssk->sk_ipv6only = sk->sk_ipv6only;
>         __ip_sock_set_tos(ssk, inet_sk(sk)->tos);
>  
>         if (sk->sk_userlocks & tx_rx_locks) {
--------------------- 8< ---------------------

(+ a test checking that for v6 only socket as well I suppose)

> +		chk_join_nr 1 1 1
> +	fi
> +
> +	if reset "simult IPv4 and IPv6 subflows, fullmesh 1x1"; then
> +		pm_nl_set_limits $ns1 0 4
> +		pm_nl_set_limits $ns2 1 4

Does the limit here mean 1 subflow or 1 extra subflow? (it might answer
my question below)

> +		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

Why do you not have an extra subflow in IPv6 between the second IPv6 of
ns2 (subflow,fullmesh) and the main IPV6 of the server on ns1?

> +	fi
> +
> +	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

Why do you have so many subflows here? The client has one IPv6 and the
server 2, no?

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

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

[Paolo Abeni]

On Fri, 2022-12-23 at 15:35 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 23/12/2022 13:51, Paolo Abeni wrote:
> > Let the caller specify the to-be-created subflow family. No
> > functional change intended, will be leveraged by the next
> > patch.
> 
> (...)
> 
> > @@ -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);
> 
> Mmh, I don't know if we can do that: it means we no longer inherit the
> 'sk_family'. With the modification, it means if the initial socket is
> created in v4 only (AF_INET), we will still be able to create subflows
> in IPv6, right?

Yes. That was my understanding of issues/269 goal.

> Somehow, I like the idea because with today's Internet, it is normal for
> a client to have both IPv4 and IPv6 addresses. Then we avoid people
> creating AF_INET sockets and complaining they cannot have additional
> IPv6 subflows.
> 
> But on the other hand, there is no more way to force having v4 only
> subflows, no?
> And I guess listening sockets will have to be created with AF_INET6 to
> accept additional v6 subflows.
> 
> WDYT?

I'm not sure I understand. Do you mean that we should allow mixed
ipv4/ipv6 subflows only if mptcp family is ipv6? that is doable, I
think, just slightly different from what is implemented here.

Cheers,

Paolo

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

[Matthieu Baerts]

On 23/12/2022 17:01, Paolo Abeni wrote:
> On Fri, 2022-12-23 at 15:35 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 23/12/2022 13:51, Paolo Abeni wrote:
>>> Let the caller specify the to-be-created subflow family. No
>>> functional change intended, will be leveraged by the next
>>> patch.
>>
>> (...)
>>
>>> @@ -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);
>>
>> Mmh, I don't know if we can do that: it means we no longer inherit the
>> 'sk_family'. With the modification, it means if the initial socket is
>> created in v4 only (AF_INET), we will still be able to create subflows
>> in IPv6, right?
> 
> Yes. That was my understanding of issues/269 goal.

I didn't think about that when creating issue 269.

>> Somehow, I like the idea because with today's Internet, it is normal for
>> a client to have both IPv4 and IPv6 addresses. Then we avoid people
>> creating AF_INET sockets and complaining they cannot have additional
>> IPv6 subflows.
>>
>> But on the other hand, there is no more way to force having v4 only
>> subflows, no?
>> And I guess listening sockets will have to be created with AF_INET6 to
>> accept additional v6 subflows.
>>
>> WDYT?
> 
> I'm not sure I understand. Do you mean that we should allow mixed
> ipv4/ipv6 subflows only if mptcp family is ipv6? that is doable, I
> think, just slightly different from what is implemented here.

Maybe we don't need to change, we just need to decide which behaviour we
want.

Is it OK to have a mix of v4 and v6 subflows if the socket has been
created with AF_INET family? In this case, the family is just for the
initial subflow and not the next ones.

That's maybe fine but it might be strange that for the server side,
AF_INET6 is required to accept both IPv4 and IPv6 subflows.

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

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

[Paolo Abeni]

On Fri, 2022-12-23 at 15:35 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 23/12/2022 13:51, Paolo Abeni wrote:
> > 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 | 51 +++++++++++++++----
> >  1 file changed, 42 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..6ef4787ad244 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,45 @@ v4mapped_tests()
> >  	fi
> >  }
> >  
> > +mixed_tests()
> > +{
> > +	# subflow IPv4-mapped to IPv4-mapped
> 
> (small detail: the comment is not adapted. I can remove it when applying
> the patch if no other modifications are needed. Same just below, the
> next test)
> 
> > +	if reset "simult IPv4 and IPv6 subflows, first is IPv4"; 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
> 
> (linked to my question from patch 1/3)
> 
> If I'm not mistaken, by default, mptcp_connect will create an MPTCP
> socket with AF_INET family if the initial destination is an IPv4
> address, no?
> Here should we not try to connect to ::ffff:10.0.1.1 instead (or pass -6
> option? Maybe not enough)?
> 
> Also, I guess we should have an extra test to make sure a v4 only
> sockets is not allowing extra subflows in v6, no?
> 
> > +		chk_join_nr 1 1 1
> > +	fi
> > +
> > +	# subflow IPv4-mapped to IPv4-mapped
> > +	if reset "simult IPv4 and IPv6 subflows, first is IPv6"; 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
> 
> Linked to the previous question: that makes me think we properly don't
> propagate the IPV6_V6ONLY socket option (sk_ipv6only) to new subflows
> (just before the connect). Now that we can connect to v4 addresses, it
> might be an issue. Easy to fix with this I guess:
> 
> --------------------- 8< ---------------------
> > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> > index 582ed93bcc8a..9986681aaf40 100644
> > --- a/net/mptcp/sockopt.c
> > +++ b/net/mptcp/sockopt.c
> > @@ -1255,6 +1255,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
> >         ssk->sk_priority = sk->sk_priority;
> >         ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
> >         ssk->sk_incoming_cpu = sk->sk_incoming_cpu;
> > +       ssk->sk_ipv6only = sk->sk_ipv6only;
> >         __ip_sock_set_tos(ssk, inet_sk(sk)->tos);
> >  
> >         if (sk->sk_userlocks & tx_rx_locks) {
> --------------------- 8< ---------------------
> 
> (+ a test checking that for v6 only socket as well I suppose)

This is an unrelated patch. Additionally, I think sk_ipv6only on
subflow other then the first one is not needed, as we already match
carefully the sk family and address family (we create ipv6 subflow for
ipv6 addr)

> > +		chk_join_nr 1 1 1
> > +	fi
> > +
> > +	if reset "simult IPv4 and IPv6 subflows, fullmesh 1x1"; then
> > +		pm_nl_set_limits $ns1 0 4
> > +		pm_nl_set_limits $ns2 1 4
> 
> Does the limit here mean 1 subflow or 1 extra subflow? (it might answer
> my question below)
> 
> > +		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
> 
> Why do you not have an extra subflow in IPv6 between the second IPv6 of
> ns2 (subflow,fullmesh) and the main IPV6 of the server on ns1?

becayse there is no second ipv6 address in ns2:  dead:beef:2::2 is used
as source by the first subflow (to connect towards dead:beef:2::1).

> > +	fi
> > +
> > +	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
> 
> Why do you have so many subflows here? The client has one IPv6 and the
> server 2, no?

Here the client has 2 IPv6 addresses: dead:beef:1::2 and the one added
by the 'fullmesh_1' argument (dead:beef:3::2, IIRC).

/P

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

[Paolo Abeni]

On Fri, 2022-12-23 at 15:35 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> Thank you for having looked at these patches!
> 
> On 23/12/2022 13:51, 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 with no constraint.
> > 
> > This patch changes the in kernel PM logic to create subflows matching
> > the currently selected source (or destination) address, 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 | 57 ++++++++++++++++++++++--------------------
> >  1 file changed, 30 insertions(+), 27 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index f2a43e13bacd..1d4ba6716a98 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> 
> (...)
> 
> > @@ -600,10 +610,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);
> 
> I see that you removed the 'if (nr)' condition: was there a bug before?
> If no addresses have been filled, no new subflows will be created below
> and this ID is still available.

The previous behavior was uncorrect, I think. It's cleaner if we always
consider consumed an endpoint once we tried to process it, but I can't
easily think of a scenario demonstrating a real "bug". I think we can
squash the change in here.

/P

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

[Paolo Abeni]

On Fri, 2022-12-23 at 17:08 +0100, Matthieu Baerts wrote:
> On 23/12/2022 17:01, Paolo Abeni wrote:
> 
> > Do you mean that we should allow mixed
> > ipv4/ipv6 subflows only if mptcp family is ipv6? that is doable, I
> > think, just slightly different from what is implemented here.
> 
> Maybe we don't need to change, we just need to decide which behaviour we
> want.
> 
> Is it OK to have a mix of v4 and v6 subflows if the socket has been
> created with AF_INET family? In this case, the family is just for the
> initial subflow and not the next ones.
> 
> That's maybe fine but it might be strange that for the server side,
> AF_INET6 is required to accept both IPv4 and IPv6 subflows.

Ok, adapting to that does not look like a big change. I'll try to cook
it.

Thanks,

Paolo

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

[Matthieu Baerts]

Hi Paolo,

Thank you for the replies!

On 23/12/2022 19:29, Paolo Abeni wrote:
> On Fri, 2022-12-23 at 15:35 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 23/12/2022 13:51, Paolo Abeni wrote:
>>> 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 | 51 +++++++++++++++----
>>>  1 file changed, 42 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..6ef4787ad244 100755
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh

(...)

>>> +		chk_join_nr 1 1 1
>>> +	fi
>>> +
>>> +	# subflow IPv4-mapped to IPv4-mapped
>>> +	if reset "simult IPv4 and IPv6 subflows, first is IPv6"; 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
>>
>> Linked to the previous question: that makes me think we properly don't
>> propagate the IPV6_V6ONLY socket option (sk_ipv6only) to new subflows
>> (just before the connect). Now that we can connect to v4 addresses, it
>> might be an issue. Easy to fix with this I guess:
>>
>> --------------------- 8< ---------------------
>>> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
>>> index 582ed93bcc8a..9986681aaf40 100644
>>> --- a/net/mptcp/sockopt.c
>>> +++ b/net/mptcp/sockopt.c
>>> @@ -1255,6 +1255,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
>>>         ssk->sk_priority = sk->sk_priority;
>>>         ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
>>>         ssk->sk_incoming_cpu = sk->sk_incoming_cpu;
>>> +       ssk->sk_ipv6only = sk->sk_ipv6only;
>>>         __ip_sock_set_tos(ssk, inet_sk(sk)->tos);
>>>  
>>>         if (sk->sk_userlocks & tx_rx_locks) {
>> --------------------- 8< ---------------------
>>
>> (+ a test checking that for v6 only socket as well I suppose)
> 
> This is an unrelated patch. Additionally, I think sk_ipv6only on
> subflow other then the first one is not needed, as we already match
> carefully the sk family and address family (we create ipv6 subflow for
> ipv6 addr)

I'm sorry, I'm not sure to understand. If someone wants to have only v6
connections, it has to create an AF_INET6 socket and set IPV6_V6ONLY
option. Why not reflecting that to the different subflows to have
subflow in v6 only?

But I agree it is not directly related.

>>> +		chk_join_nr 1 1 1
>>> +	fi
>>> +
>>> +	if reset "simult IPv4 and IPv6 subflows, fullmesh 1x1"; then
>>> +		pm_nl_set_limits $ns1 0 4
>>> +		pm_nl_set_limits $ns2 1 4
>>
>> Does the limit here mean 1 subflow or 1 extra subflow? (it might answer
>> my question below)
>>
>>> +		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
>>
>> Why do you not have an extra subflow in IPv6 between the second IPv6 of
>> ns2 (subflow,fullmesh) and the main IPV6 of the server on ns1?
> 
> becayse there is no second ipv6 address in ns2:  dead:beef:2::2 is used
> as source by the first subflow (to connect towards dead:beef:2::1).

Ah yes indeed, I don't know why I had another topology in mind...

>>> +	fi
>>> +
>>> +	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
>>
>> Why do you have so many subflows here? The client has one IPv6 and the
>> server 2, no?
> 
> Here the client has 2 IPv6 addresses: dead:beef:1::2 and the one added
> by the 'fullmesh_1' argument (dead:beef:3::2, IIRC).

Indeed, thank you. There will then be 3 subflows in v6 and 1 in v4.

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

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

[Matthieu Baerts]

On 26/12/2022 14:09, Matthieu Baerts wrote:
> Hi Paolo,
> 
> Thank you for the replies!
> 
> On 23/12/2022 19:29, Paolo Abeni wrote:
>> On Fri, 2022-12-23 at 15:35 +0100, Matthieu Baerts wrote:
>>> Hi Paolo,
>>>
>>> On 23/12/2022 13:51, Paolo Abeni wrote:
>>>> 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 | 51 +++++++++++++++----
>>>>  1 file changed, 42 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..6ef4787ad244 100755
>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> 
> (...)
> 
>>>> +		chk_join_nr 1 1 1
>>>> +	fi
>>>> +
>>>> +	# subflow IPv4-mapped to IPv4-mapped
>>>> +	if reset "simult IPv4 and IPv6 subflows, first is IPv6"; 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
>>>
>>> Linked to the previous question: that makes me think we properly don't
>>> propagate the IPV6_V6ONLY socket option (sk_ipv6only) to new subflows
>>> (just before the connect). Now that we can connect to v4 addresses, it
>>> might be an issue. Easy to fix with this I guess:
>>>
>>> --------------------- 8< ---------------------
>>>> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
>>>> index 582ed93bcc8a..9986681aaf40 100644
>>>> --- a/net/mptcp/sockopt.c
>>>> +++ b/net/mptcp/sockopt.c
>>>> @@ -1255,6 +1255,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
>>>>         ssk->sk_priority = sk->sk_priority;
>>>>         ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
>>>>         ssk->sk_incoming_cpu = sk->sk_incoming_cpu;
>>>> +       ssk->sk_ipv6only = sk->sk_ipv6only;
>>>>         __ip_sock_set_tos(ssk, inet_sk(sk)->tos);
>>>>  
>>>>         if (sk->sk_userlocks & tx_rx_locks) {
>>> --------------------- 8< ---------------------
>>>
>>> (+ a test checking that for v6 only socket as well I suppose)
>>
>> This is an unrelated patch. Additionally, I think sk_ipv6only on
>> subflow other then the first one is not needed, as we already match
>> carefully the sk family and address family (we create ipv6 subflow for
>> ipv6 addr)
> 
> I'm sorry, I'm not sure to understand. If someone wants to have only v6
> connections, it has to create an AF_INET6 socket and set IPV6_V6ONLY
> option. Why not reflecting that to the different subflows to have
> subflow in v6 only?
> 
> But I agree it is not directly related.
I just noticed that in your v2, you added a check in pm_netlink.h. But I
guess it is fine to also apply the patch, just to be sure the check for
pm_netlink.h is not bypassed somehow (by the userspace PM, a future one,
with BPF, etc.), no?

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