[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: convert IPv4 address to IPv4-mapped

[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: convert IPv4 address to IPv4-mapped

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3780 bytes --]

On Wed, 16 Dec 2020, Geliang Tang wrote:

> Currently, PM doesn't create subflow with IPv4-mapped IPv6 socket. This
> patch converts the IPv4 address to IPv4-mapped IPv6 address to fix it.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> net/mptcp/pm_netlink.c | 25 ++++++++++++++++++++++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index cc4ca013a06b..987e83cdee11 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -133,6 +133,19 @@ static bool lookup_subflow_by_saddr(const struct list_head *list,
> 	return false;
> }
>
> +static void mptcp_pm_addr_convert_v4mapped(struct mptcp_sock *msk,
> +					   struct mptcp_addr_info *addr)
> +{
> +	struct mptcp_addr_info local;
> +
> +	local_address((struct sock_common *)msk, &local);
> +	if (ipv6_addr_v4mapped(&local.addr6) &&
> +	    addr->family == AF_INET) {
> +		ipv6_addr_set_v4mapped(addr->addr.s_addr, &addr->addr6);
> +		addr->family = AF_INET6;
> +	}
> +}
> +
> static struct mptcp_pm_addr_entry *
> select_local_address(const struct pm_nl_pernet *pernet,
> 		     struct mptcp_sock *msk)
> @@ -145,6 +158,8 @@ select_local_address(const struct pm_nl_pernet *pernet,
> 		if (!(entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
> 			continue;
>
> +		mptcp_pm_addr_convert_v4mapped(msk, &entry->addr);
> +

This is modifying the contents of pernet->local_addr_list, not a copy of 
the entry, when the msk has a v4mapped address. I'm not sure that's the 
way to fix this, and might leave some corner cases unaddressed.

Did you look at modifying addresses_equal() so it can detect equivalent 
IPv4 and v4mapped addresses?

> 		/* avoid any address already in use by subflows and
> 		 * pending join
> 		 */

There's some code just below this comment that would need to be updated - 
I'm not sure why the address family needs to be checked before calling 
lookup_subflow_by_saddr(), except as an optimization.

I will talk to the group about v4mapped address handling at the meeting 
tomorrow to see if there are other opinions on how this should be handled 
(I will check this email thread and 
https://github.com/multipath-tcp/mptcp_net-next/issues/122 for any updates 
first). I'll follow up on the github issue if needed.

Mat


> @@ -159,9 +174,11 @@ select_local_address(const struct pm_nl_pernet *pernet,
> }
>
> static struct mptcp_pm_addr_entry *
> -select_signal_address(struct pm_nl_pernet *pernet, unsigned int pos)
> +select_signal_address(struct pm_nl_pernet *pernet,
> +		      struct mptcp_sock *msk)
> {
> 	struct mptcp_pm_addr_entry *entry, *ret = NULL;
> +	unsigned int pos = msk->pm.add_addr_signaled;
> 	int i = 0;
>
> 	rcu_read_lock();
> @@ -173,6 +190,9 @@ select_signal_address(struct pm_nl_pernet *pernet, unsigned int pos)
> 	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
> 		if (!(entry->addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
> 			continue;
> +
> +		mptcp_pm_addr_convert_v4mapped(msk, &entry->addr);
> +
> 		if (i++ == pos) {
> 			ret = entry;
> 			break;
> @@ -346,8 +366,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>
> 	/* check first for announce */
> 	if (msk->pm.add_addr_signaled < msk->pm.add_addr_signal_max) {
> -		local = select_signal_address(pernet,
> -					      msk->pm.add_addr_signaled);
> +		local = select_signal_address(pernet, msk);
>
> 		if (local) {
> 			if (mptcp_pm_alloc_anno_list(msk, local)) {
> -- 
> 2.29.2
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>

--
Mat Martineau
Intel

[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: convert IPv4 address to IPv4-mapped

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]

Hi Geliang, Mat,

On 17/12/2020 01:23, Mat Martineau wrote:
> On Wed, 16 Dec 2020, Geliang Tang wrote:
> 
>> Currently, PM doesn't create subflow with IPv4-mapped IPv6 socket. This
>> patch converts the IPv4 address to IPv4-mapped IPv6 address to fix it.
>>
>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
>> ---
>> net/mptcp/pm_netlink.c | 25 ++++++++++++++++++++++---
>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index cc4ca013a06b..987e83cdee11 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -145,6 +158,8 @@ select_local_address(const struct pm_nl_pernet 
>> *pernet,
>>         if (!(entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
>>             continue;
>>
>> +        mptcp_pm_addr_convert_v4mapped(msk, &entry->addr);
>> +
> 
> This is modifying the contents of pernet->local_addr_list, not a copy of 
> the entry, when the msk has a v4mapped address. I'm not sure that's the 
> way to fix this, and might leave some corner cases unaddressed.
> 
> Did you look at modifying addresses_equal() so it can detect equivalent 
> IPv4 and v4mapped addresses?

Maybe something like that the patch I joined?

I didn't try much but maybe more what we want?

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-support-v4mappedv6.patch --]
[-- Type: text/x-patch, Size: 3897 bytes --]

From 36f5e1f9bade5329e82a85fb6330db57a165c04c Mon Sep 17 00:00:00 2001
From: Matthieu Baerts <matthieu.baerts@tessares.net>
Date: Fri, 18 Dec 2020 00:03:12 +0100
Subject: [PATCH] mptcp: support v4mappedv6

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/pm_netlink.c | 22 +++++++++++++---------
 net/mptcp/subflow.c    | 17 +++++++++++------
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 9b1f6298bbdb..6c89a9e0099b 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -60,15 +60,20 @@ static bool addresses_equal(const struct mptcp_addr_info *a,
 {
 	bool addr_equals = false;
 
-	if (a->family != b->family)
-		return false;
-
-	if (a->family == AF_INET)
-		addr_equals = a->addr.s_addr == b->addr.s_addr;
+	if (a->family == b->family) {
+		if (a->family == AF_INET)
+			addr_equals = a->addr.s_addr == b->addr.s_addr;
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
-	else
-		addr_equals = !ipv6_addr_cmp(&a->addr6, &b->addr6);
+		else
+			addr_equals = !ipv6_addr_cmp(&a->addr6, &b->addr6);
+	} else if (a->family == AF_INET6) {
+		if (b->family == AF_INET && ipv6_addr_v4mapped(&a->addr6))
+			addr_equals = a->addr6.s6_addr32[3] == b->addr.s_addr;
+	} else if (b->family == AF_INET6) {
+		if (a->family == AF_INET && ipv6_addr_v4mapped(&b->addr6))
+			addr_equals = a->addr.s_addr == b->addr6.s6_addr32[3];
 #endif
+	}
 
 	if (!addr_equals)
 		return false;
@@ -147,8 +152,7 @@ select_local_address(const struct pm_nl_pernet *pernet,
 		/* avoid any address already in use by subflows and
 		 * pending join
 		 */
-		if (entry->addr.family == ((struct sock *)msk)->sk_family &&
-		    !lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) {
+		if (!lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) {
 			ret = entry;
 			break;
 		}
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 278cbe3e539e..11cc0ef3ae3c 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1074,10 +1074,11 @@ void mptcpv6_handle_mapped(struct sock *sk, bool mapped)
 #endif
 
 static void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
-				struct sockaddr_storage *addr)
+				struct sockaddr_storage *addr,
+				unsigned short family)
 {
 	memset(addr, 0, sizeof(*addr));
-	addr->ss_family = info->family;
+	addr->ss_family = family;
 	if (addr->ss_family == AF_INET) {
 		struct sockaddr_in *in_addr = (struct sockaddr_in *)addr;
 
@@ -1088,7 +1089,11 @@ static void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
 	else if (addr->ss_family == AF_INET6) {
 		struct sockaddr_in6 *in6_addr = (struct sockaddr_in6 *)addr;
 
-		in6_addr->sin6_addr = info->addr6;
+		if (info->family == AF_INET)
+			ipv6_addr_set_v4mapped(info->addr.s_addr,
+					       &in6_addr->sin6_addr);
+		else
+			in6_addr->sin6_addr = info->addr6;
 		in6_addr->sin6_port = info->port;
 	}
 #endif
@@ -1132,11 +1137,11 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	subflow->remote_key = msk->remote_key;
 	subflow->local_key = msk->local_key;
 	subflow->token = msk->token;
-	mptcp_info2sockaddr(loc, &addr);
+	mptcp_info2sockaddr(loc, &addr, ssk->sk_family);
 
 	addrlen = sizeof(struct sockaddr_in);
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
-	if (loc->family == AF_INET6)
+	if (addr.ss_family == AF_INET6)
 		addrlen = sizeof(struct sockaddr_in6);
 #endif
 	ssk->sk_bound_dev_if = loc->ifindex;
@@ -1152,7 +1157,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	subflow->remote_id = remote_id;
 	subflow->request_join = 1;
 	subflow->request_bkup = !!(loc->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
-	mptcp_info2sockaddr(remote, &addr);
+	mptcp_info2sockaddr(remote, &addr, ssk->sk_family);
 
 	mptcp_add_pending_subflow(msk, subflow);
 	err = kernel_connect(sf, (struct sockaddr *)&addr, addrlen, O_NONBLOCK);
-- 
2.29.2

[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: convert IPv4 address to IPv4-mapped

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

On Fri, 18 Dec 2020, Matthieu Baerts wrote:

> Hi Geliang, Mat,
>
> On 17/12/2020 01:23, Mat Martineau wrote:
>> On Wed, 16 Dec 2020, Geliang Tang wrote:
>> 
>>> Currently, PM doesn't create subflow with IPv4-mapped IPv6 socket. This
>>> patch converts the IPv4 address to IPv4-mapped IPv6 address to fix it.
>>> 
>>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
>>> ---
>>> net/mptcp/pm_netlink.c | 25 ++++++++++++++++++++++---
>>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index cc4ca013a06b..987e83cdee11 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -145,6 +158,8 @@ select_local_address(const struct pm_nl_pernet 
>>> *pernet,
>>>         if (!(entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
>>>             continue;
>>> 
>>> +        mptcp_pm_addr_convert_v4mapped(msk, &entry->addr);
>>> +
>> 
>> This is modifying the contents of pernet->local_addr_list, not a copy of 
>> the entry, when the msk has a v4mapped address. I'm not sure that's the way 
>> to fix this, and might leave some corner cases unaddressed.
>> 
>> Did you look at modifying addresses_equal() so it can detect equivalent 
>> IPv4 and v4mapped addresses?
>
> Maybe something like that the patch I joined?
>
> I didn't try much but maybe more what we want?
>

I haven't tested it out, but yes, that's what I had in mind.

--
Mat Martineau
Intel

[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: convert IPv4 address to IPv4-mapped

[Geliang Tang]

[-- Attachment #1: Type: text/plain, Size: 8223 bytes --]

Hi Matt,

On Fri, Dec 18, 2020 at 12:06:51AM +0100, Matthieu Baerts wrote:
> Hi Geliang, Mat,
> 
> On 17/12/2020 01:23, Mat Martineau wrote:
> > On Wed, 16 Dec 2020, Geliang Tang wrote:
> > 
> > > Currently, PM doesn't create subflow with IPv4-mapped IPv6 socket. This
> > > patch converts the IPv4 address to IPv4-mapped IPv6 address to fix it.
> > > 
> > > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > > ---
> > > net/mptcp/pm_netlink.c | 25 ++++++++++++++++++++++---
> > > 1 file changed, 22 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index cc4ca013a06b..987e83cdee11 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > > @@ -145,6 +158,8 @@ select_local_address(const struct pm_nl_pernet
> > > *pernet,
> > >         if (!(entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
> > >             continue;
> > > 
> > > +        mptcp_pm_addr_convert_v4mapped(msk, &entry->addr);
> > > +
> > 
> > This is modifying the contents of pernet->local_addr_list, not a copy of
> > the entry, when the msk has a v4mapped address. I'm not sure that's the
> > way to fix this, and might leave some corner cases unaddressed.
> > 
> > Did you look at modifying addresses_equal() so it can detect equivalent
> > IPv4 and v4mapped addresses?
> 
> Maybe something like that the patch I joined?
> 
> I didn't try much but maybe more what we want?

Thanks for your help. Your patch is much better than mine. I added your
code in v3 with only a slightly change in addresses_equal. I added your
Co-developed-by tag in v3 if you don't mind:

@@ -66,12 +66,11 @@ static bool addresses_equal(const struct mptcp_addr_info *a,
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
                else
                        addr_equals = !ipv6_addr_cmp(&a->addr6, &b->addr6);
-       } else if (a->family == AF_INET6) {
-               if (b->family == AF_INET && ipv6_addr_v4mapped(&a->addr6))
-                       addr_equals = a->addr6.s6_addr32[3] == b->addr.s_addr;
-       } else if (b->family == AF_INET6) {
+       } else {
                if (a->family == AF_INET && ipv6_addr_v4mapped(&b->addr6))
                        addr_equals = a->addr.s_addr == b->addr6.s6_addr32[3];
+               if (b->family == AF_INET && ipv6_addr_v4mapped(&a->addr6))
+                       addr_equals = a->addr6.s6_addr32[3] == b->addr.s_addr;
 #endif
        }

And I added the following code in mptcp_info2sockaddr to deal with the
IPv4-mapped back to IPv4 case too:

--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1082,7 +1082,10 @@ static void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
        if (addr->ss_family == AF_INET) {
                struct sockaddr_in *in_addr = (struct sockaddr_in *)addr;

-               in_addr->sin_addr = info->addr;
+               if (info->family == AF_INET)
+                       in_addr->sin_addr = info->addr;
+               else
+                       in_addr->sin_addr.s_addr = info->addr6.s6_addr32[3];
                in_addr->sin_port = info->port;
        }
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)

Also in v3 I added two new IPv4-mapped back to IPv4 testcases:

+# signal subflow v6-map-v4
+reset
+ip netns exec $ns1 ./pm_nl_ctl limits 0 1
+ip netns exec $ns2 ./pm_nl_ctl limits 0 1
+ip netns exec $ns2 ./pm_nl_ctl add "::ffff:10.0.3.2" flags subflow
+run_tests $ns1 $ns2 10.0.1.1
+chk_join_nr "single subflow v6-map-v4" 1 1 1
+
+# signal address v6-map-v4
+reset
+ip netns exec $ns1 ./pm_nl_ctl limits 0 1
+ip netns exec $ns2 ./pm_nl_ctl limits 1 1
+ip netns exec $ns1 ./pm_nl_ctl add "::ffff:10.0.2.1" flags signal
+run_tests $ns1 $ns2 10.0.1.1
+chk_join_nr "signal address v6-map-v4" 1 1 1
+chk_add_nr 1 1

Thanks.

-Geliang

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

> >From 36f5e1f9bade5329e82a85fb6330db57a165c04c Mon Sep 17 00:00:00 2001
> From: Matthieu Baerts <matthieu.baerts(a)tessares.net>
> Date: Fri, 18 Dec 2020 00:03:12 +0100
> Subject: [PATCH] mptcp: support v4mappedv6
> 
> Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
> ---
>  net/mptcp/pm_netlink.c | 22 +++++++++++++---------
>  net/mptcp/subflow.c    | 17 +++++++++++------
>  2 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 9b1f6298bbdb..6c89a9e0099b 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -60,15 +60,20 @@ static bool addresses_equal(const struct mptcp_addr_info *a,
>  {
>  	bool addr_equals = false;
>  
> -	if (a->family != b->family)
> -		return false;
> -
> -	if (a->family == AF_INET)
> -		addr_equals = a->addr.s_addr == b->addr.s_addr;
> +	if (a->family == b->family) {
> +		if (a->family == AF_INET)
> +			addr_equals = a->addr.s_addr == b->addr.s_addr;
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -	else
> -		addr_equals = !ipv6_addr_cmp(&a->addr6, &b->addr6);
> +		else
> +			addr_equals = !ipv6_addr_cmp(&a->addr6, &b->addr6);
> +	} else if (a->family == AF_INET6) {
> +		if (b->family == AF_INET && ipv6_addr_v4mapped(&a->addr6))
> +			addr_equals = a->addr6.s6_addr32[3] == b->addr.s_addr;
> +	} else if (b->family == AF_INET6) {
> +		if (a->family == AF_INET && ipv6_addr_v4mapped(&b->addr6))
> +			addr_equals = a->addr.s_addr == b->addr6.s6_addr32[3];
>  #endif
> +	}
>  
>  	if (!addr_equals)
>  		return false;
> @@ -147,8 +152,7 @@ select_local_address(const struct pm_nl_pernet *pernet,
>  		/* avoid any address already in use by subflows and
>  		 * pending join
>  		 */
> -		if (entry->addr.family == ((struct sock *)msk)->sk_family &&
> -		    !lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) {
> +		if (!lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) {
>  			ret = entry;
>  			break;
>  		}
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 278cbe3e539e..11cc0ef3ae3c 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1074,10 +1074,11 @@ void mptcpv6_handle_mapped(struct sock *sk, bool mapped)
>  #endif
>  
>  static void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
> -				struct sockaddr_storage *addr)
> +				struct sockaddr_storage *addr,
> +				unsigned short family)
>  {
>  	memset(addr, 0, sizeof(*addr));
> -	addr->ss_family = info->family;
> +	addr->ss_family = family;
>  	if (addr->ss_family == AF_INET) {
>  		struct sockaddr_in *in_addr = (struct sockaddr_in *)addr;
>  
> @@ -1088,7 +1089,11 @@ static void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
>  	else if (addr->ss_family == AF_INET6) {
>  		struct sockaddr_in6 *in6_addr = (struct sockaddr_in6 *)addr;
>  
> -		in6_addr->sin6_addr = info->addr6;
> +		if (info->family == AF_INET)
> +			ipv6_addr_set_v4mapped(info->addr.s_addr,
> +					       &in6_addr->sin6_addr);
> +		else
> +			in6_addr->sin6_addr = info->addr6;
>  		in6_addr->sin6_port = info->port;
>  	}
>  #endif
> @@ -1132,11 +1137,11 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
>  	subflow->remote_key = msk->remote_key;
>  	subflow->local_key = msk->local_key;
>  	subflow->token = msk->token;
> -	mptcp_info2sockaddr(loc, &addr);
> +	mptcp_info2sockaddr(loc, &addr, ssk->sk_family);
>  
>  	addrlen = sizeof(struct sockaddr_in);
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -	if (loc->family == AF_INET6)
> +	if (addr.ss_family == AF_INET6)
>  		addrlen = sizeof(struct sockaddr_in6);
>  #endif
>  	ssk->sk_bound_dev_if = loc->ifindex;
> @@ -1152,7 +1157,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
>  	subflow->remote_id = remote_id;
>  	subflow->request_join = 1;
>  	subflow->request_bkup = !!(loc->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> -	mptcp_info2sockaddr(remote, &addr);
> +	mptcp_info2sockaddr(remote, &addr, ssk->sk_family);
>  
>  	mptcp_add_pending_subflow(msk, subflow);
>  	err = kernel_connect(sf, (struct sockaddr *)&addr, addrlen, O_NONBLOCK);
> -- 
> 2.29.2
> 

[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: convert IPv4 address to IPv4-mapped

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 4228 bytes --]

Hi Geliang,

On 18/12/2020 12:27, Geliang Tang wrote:
> Hi Matt,
> 
> On Fri, Dec 18, 2020 at 12:06:51AM +0100, Matthieu Baerts wrote:
>> Hi Geliang, Mat,
>>
>> On 17/12/2020 01:23, Mat Martineau wrote:
>>> On Wed, 16 Dec 2020, Geliang Tang wrote:
>>>
>>>> Currently, PM doesn't create subflow with IPv4-mapped IPv6 socket. This
>>>> patch converts the IPv4 address to IPv4-mapped IPv6 address to fix it.
>>>>
>>>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
>>>> ---
>>>> net/mptcp/pm_netlink.c | 25 ++++++++++++++++++++++---
>>>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>> index cc4ca013a06b..987e83cdee11 100644
>>>> --- a/net/mptcp/pm_netlink.c
>>>> +++ b/net/mptcp/pm_netlink.c
>>>> @@ -145,6 +158,8 @@ select_local_address(const struct pm_nl_pernet
>>>> *pernet,
>>>>          if (!(entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
>>>>              continue;
>>>>
>>>> +        mptcp_pm_addr_convert_v4mapped(msk, &entry->addr);
>>>> +
>>>
>>> This is modifying the contents of pernet->local_addr_list, not a copy of
>>> the entry, when the msk has a v4mapped address. I'm not sure that's the
>>> way to fix this, and might leave some corner cases unaddressed.
>>>
>>> Did you look at modifying addresses_equal() so it can detect equivalent
>>> IPv4 and v4mapped addresses?
>>
>> Maybe something like that the patch I joined?
>>
>> I didn't try much but maybe more what we want?
> 
> Thanks for your help. Your patch is much better than mine. I added your
> code in v3 with only a slightly change in addresses_equal. I added your
> Co-developed-by tag in v3 if you don't mind:

Of course I don't mind, that's the proper tag when a code is modified by 
different people!

> @@ -66,12 +66,11 @@ static bool addresses_equal(const struct mptcp_addr_info *a,
>   #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>                  else
>                          addr_equals = !ipv6_addr_cmp(&a->addr6, &b->addr6);
> -       } else if (a->family == AF_INET6) {
> -               if (b->family == AF_INET && ipv6_addr_v4mapped(&a->addr6))
> -                       addr_equals = a->addr6.s6_addr32[3] == b->addr.s_addr;
> -       } else if (b->family == AF_INET6) {
> +       } else {
>                  if (a->family == AF_INET && ipv6_addr_v4mapped(&b->addr6))
>                          addr_equals = a->addr.s_addr == b->addr6.s6_addr32[3];
> +               if (b->family == AF_INET && ipv6_addr_v4mapped(&a->addr6))
> +                       addr_equals = a->addr6.s6_addr32[3] == b->addr.s_addr;

I will need to check to verify if we can safely assume that if the 
families are not the same, the only possibility is one in v4 and the 
other one in v6. If we cannot assume that, we cannot really call 
ipv6_addr_v4mapped() without checking if it is v6. Could we have there a 
non initialised one (family == 0)? Or something else?

>   #endif
>          }
> 
> And I added the following code in mptcp_info2sockaddr to deal with the
> IPv4-mapped back to IPv4 case too:

I see, good point!
But now I wonder if we need to support v4mapped addresses on the PM 
side. This seems to increase the complexity because the PM would then 
have to check if the local address is v4, v4 mapped in a v6 or a real 
v6. Just to avoid asking to connect from a (real) v6 to v4 or the opposite.

> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1082,7 +1082,10 @@ static void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
>          if (addr->ss_family == AF_INET) {
>                  struct sockaddr_in *in_addr = (struct sockaddr_in *)addr;
> 
> -               in_addr->sin_addr = info->addr;
> +               if (info->family == AF_INET)
> +                       in_addr->sin_addr = info->addr;
> +               else
> +                       in_addr->sin_addr.s_addr = info->addr6.s6_addr32[3];

Link to my previous comment: we might need to be careful that if "info" 
has a real IPv6 address, we will not do what we want here.

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