4:41 p.m.
Fix issues found by Mat in the "IPv6 prerequisite" patch series
when running against the multipath.org implementation.
Peter Krystad (3):
mptcp: fix mistake in path manager interface
mptcp: Save address family when creating socket
mptcp: Use correct address family when creating subflows
net/mptcp/basic.c | 28 ++++++++++++++++------------
net/mptcp/pm.c | 4 ++--
net/mptcp/protocol.c | 1 +
net/mptcp/protocol.h | 1 +
4 files changed, 20 insertions(+), 14 deletions(-)
--
2.17.2
4:41 p.m.
New subject: [PATCH 1/3] mptcp: fix mistake in path manager interface
Do not de-reference NULL pointer immediately after confirming it
is NULL.
squashto: Implement path manager interface commands
Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
---
net/mptcp/pm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index f521bf0c5f69..d45406bc3f6c 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -95,7 +95,7 @@ int mptcp_pm_create_subflow(u32 token, u8 remote_id, struct in_addr
*addr)
local.sin_family = AF_INET;
local.sin_port = 0;
- if (!addr)
+ if (addr)
local.sin_addr = *addr;
else
local.sin_addr.s_addr = htonl(INADDR_ANY);
@@ -133,7 +133,7 @@ int mptcp_pm_create_subflow6(u32 token, u8 remote_id, struct in6_addr
*addr)
local.sin6_family = AF_INET6;
local.sin6_port = 0;
- if (!addr)
+ if (addr)
local.sin6_addr = *addr;
else
local.sin6_addr = in6addr_any;
--
2.17.2
4:57 a.m.
New subject: [PATCH 1/3] mptcp: fix mistake in path manager interface
Hi Peter,
On 22/10/2019 01:41, Peter Krystad wrote:
Do not de-reference NULL pointer immediately after confirming it
is NULL.
squashto: Implement path manager interface commands
Thank you for the patch! I just applied this one but not the other ones
due to the other discussions started on the last one.
- 166607d720d7: "squashed" patch 1/3 in "mptcp: Implement path manager
interface commands"
- 088eb1a59555..ecb276617e3b: results
Tests have the same issue as what has been saw this night:
00:06:43.648 # ns2 MPTCP -> ns3 (10.0.2.2:10027) MPTCP [ OK ]
00:06:44.698 # ns2 MPTCP -> ns3 (10.0.2.2:10028) TCP [ OK ]
00:06:45.940 # ns2 TCP -> ns3 (10.0.2.2:10029) MPTCP [ OK ]
00:06:46.992 # ns2 MPTCP -> ns3 (10.0.3.2:10030) MPTCP
./mptcp_connect.sh: line 16: 797 Terminated ip netns exec
${listener_ns} ./mptcp_connect -t $timeout -l -p $port -s ${srv_proto}
0.0.0.0 < "$sin" > "$sout"
00:06:47.076 # ./mptcp_connect.sh: line 16: 798 Terminated
sleep 1
00:06:47.108 not ok 1 selftests: mptcp: mptcp_connect.sh # TIMEOUT
Maybe due to a modification in net-next.
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
4:41 p.m.
New subject: [PATCH 2/3] mptcp: Save address family when creating socket
For IPv6 the family will be needed when creating additional
subflow sockets.
squashto: mptcp: Associate MPTCP context with TCP socket
Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
---
net/mptcp/protocol.c | 1 +
net/mptcp/protocol.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c7755cde9810..e7b40987d1b6 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1091,6 +1091,7 @@ static struct socket *mptcp_socket_create_get(struct mptcp_sock
*msk)
}
msk->subflow = ssock;
+ msk->family = ssock->sk->sk_family;
subflow = mptcp_subflow_ctx(msk->subflow->sk);
subflow->request_mptcp = 1; /* @@ if MPTCP enabled */
subflow->request_version = 0; /* currently only v0 supported */
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 791f2c19cfb8..8fe5f9383d38 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -133,6 +133,7 @@ struct mptcp_sock {
u32 token;
unsigned long flags;
u16 dport;
+ sa_family_t family;
struct work_struct rtx_work;
struct list_head conn_list;
struct list_head rtx_queue;
--
2.17.2
4:41 p.m.
New subject: [PATCH 3/3] mptcp: Use correct address family when creating subflows
Use the address family used when the initial MPTCP socket
was created when making additional subflow sockets.
squashto: Implement basic path manager
Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
---
net/mptcp/basic.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/net/mptcp/basic.c b/net/mptcp/basic.c
index d2fd0560c136..2557cb63fd61 100644
--- a/net/mptcp/basic.c
+++ b/net/mptcp/basic.c
@@ -209,19 +209,23 @@ static void create_subflow_worker(struct work_struct *work)
pernet = net_generic(sock_net((struct sock *)msk), basic_pernet_id);
- if (pernet->has_announce_v4)
- mptcp_pm_create_subflow(pm->token, pm->remote_id,
- &pernet->announce_v4_addr);
-#if IS_ENABLED(CONFIG_IPV6)
- else if (pernet->has_announce_v6)
- mptcp_pm_create_subflow6(pm->token, pm->remote_id,
- &pernet->announce_v6_addr);
-#endif
- else if (pm->local_family == AF_INET)
- mptcp_pm_create_subflow(pm->token, pm->remote_id, NULL);
+ if (msk->family == AF_INET) {
+ if (pernet->has_announce_v4)
+ mptcp_pm_create_subflow(pm->token, pm->remote_id,
+ &pernet->announce_v4_addr);
+ else
+ mptcp_pm_create_subflow(pm->token, pm->remote_id,
+ NULL);
+ }
#if IS_ENABLED(CONFIG_IPV6)
- else
- mptcp_pm_create_subflow6(pm->token, pm->remote_id, NULL);
+ else if (msk->family == AF_INET6) {
+ if (pernet->has_announce_v6)
+ mptcp_pm_create_subflow6(pm->token, pm->remote_id,
+ &pernet->announce_v6_addr);
+ else
+ mptcp_pm_create_subflow6(pm->token, pm->remote_id,
+ NULL);
+ }
#endif
sock_put((struct sock *)msk);
--
2.17.2
1:35 a.m.
New subject: [PATCH 3/3] mptcp: Use correct address family when creating subflows
Hi,
On Mon, 2019-10-21 at 16:41 -0700, Peter Krystad wrote:
Use the address family used when the initial MPTCP socket
was created when making additional subflow sockets.
squashto: Implement basic path manager
Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
---
net/mptcp/basic.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/net/mptcp/basic.c b/net/mptcp/basic.c
index d2fd0560c136..2557cb63fd61 100644
--- a/net/mptcp/basic.c
+++ b/net/mptcp/basic.c
@@ -209,19 +209,23 @@ static void create_subflow_worker(struct work_struct *work)
pernet = net_generic(sock_net((struct sock *)msk), basic_pernet_id);
- if (pernet->has_announce_v4)
- mptcp_pm_create_subflow(pm->token, pm->remote_id,
- &pernet->announce_v4_addr);
-#if IS_ENABLED(CONFIG_IPV6)
- else if (pernet->has_announce_v6)
- mptcp_pm_create_subflow6(pm->token, pm->remote_id,
- &pernet->announce_v6_addr);
-#endif
- else if (pm->local_family == AF_INET)
- mptcp_pm_create_subflow(pm->token, pm->remote_id, NULL);
+ if (msk->family == AF_INET) {
+ if (pernet->has_announce_v4)
+ mptcp_pm_create_subflow(pm->token, pm->remote_id,
+ &pernet->announce_v4_addr);
+ else
+ mptcp_pm_create_subflow(pm->token, pm->remote_id,
+ NULL);
+ }
Here I'm likely a bit lost, but what if the peer sends an ADD_ADDR v4,
and the local msk socket is a v6 (or vice versa)?
I think checking vs the msk family is possibly not required here (IIRC
the RFC code, it looks like an MPTCP client could manage correctly
subflows with different inet families - but we need to check/match the
local address family (has_announce_v4/has_announce_v6) vs the remote
address/ADD_ADDR family. Otherwise we could try to connect to a
corrupted/truncated/uncomplete address.
Additionally, I think we need to check/match the local additional
address family (has_announce_v4/has_announce_v6) vs the msk family in
announce_addr_worker(), so that the sever always announce addresses
that is able to accept[1].
Thanks,
Paolo
[1] for IPv6 listener such check could be fairly more complex - unbound
listener could accept any ipv4 address, bound ones could check for
ipv4-mapped ipv6 address, etc... I think is better use to the simpler
approach here.
4:12 a.m.
New subject: [PATCH 3/3] mptcp: Use correct address family when creating subflows
Hi Paolo, Peter,
On 22/10/2019 10:35, Paolo Abeni wrote:
Hi,
On Mon, 2019-10-21 at 16:41 -0700, Peter Krystad wrote:
> Use the address family used when the initial MPTCP socket
> was created when making additional subflow sockets.
>
> squashto: Implement basic path manager
>
> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> ---
> net/mptcp/basic.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/net/mptcp/basic.c b/net/mptcp/basic.c
> index d2fd0560c136..2557cb63fd61 100644
> --- a/net/mptcp/basic.c
> +++ b/net/mptcp/basic.c
> @@ -209,19 +209,23 @@ static void create_subflow_worker(struct work_struct *work)
>
> pernet = net_generic(sock_net((struct sock *)msk), basic_pernet_id);
>
> - if (pernet->has_announce_v4)
> - mptcp_pm_create_subflow(pm->token, pm->remote_id,
> - &pernet->announce_v4_addr);
> -#if IS_ENABLED(CONFIG_IPV6)
> - else if (pernet->has_announce_v6)
> - mptcp_pm_create_subflow6(pm->token, pm->remote_id,
> - &pernet->announce_v6_addr);
> -#endif
> - else if (pm->local_family == AF_INET)
> - mptcp_pm_create_subflow(pm->token, pm->remote_id, NULL);
> + if (msk->family == AF_INET) {
> + if (pernet->has_announce_v4)
> + mptcp_pm_create_subflow(pm->token, pm->remote_id,
> + &pernet->announce_v4_addr);
> + else
> + mptcp_pm_create_subflow(pm->token, pm->remote_id,
> + NULL);
> + }
Here I'm likely a bit lost, but what if the peer sends an ADD_ADDR v4,
and the local msk socket is a v6 (or vice versa)?
On one hand, if the socket is v6 only, it would make sense not to
establish a v4 subflow or announce a v4 IP. Same if the socket is v4 only.
But on the other hand, when an app creates both v4-only and v6-only
sockets (like many webservers do), it is annoying because you cannot
combine v4 and v6. In this case the PM doesn't announce the addresses
for the other family because it will not accept addresses from this
other family. We respect what the app asked but I don't know if we want
to "ignore" v4-only/v6-only for other subflows.
I think checking vs the msk family is possibly not required here
(IIRC
the RFC code, it looks like an MPTCP client could manage correctly
subflows with different inet families - but we need to check/match the
local address family (has_announce_v4/has_announce_v6) vs the remote
address/ADD_ADDR family. Otherwise we could try to connect to a
corrupted/truncated/uncomplete address.
Regarding the code here, I guess we should check for sockets allowing
both v4 and v6. And possibly use the v4 addressed mapped in v6.
Additionally, I think we need to check/match the local additional
address family (has_announce_v4/has_announce_v6) vs the msk family in
announce_addr_worker(), so that the sever always announce addresses
that is able to accept[1].
Yes but only if the socket is v4-only or v6-only.
Thanks,
Paolo
[1] for IPv6 listener such check could be fairly more complex - unbound
listener could accept any ipv4 address, bound ones could check for
ipv4-mapped ipv6 address, etc... I think is better use to the simpler
approach here.
So accept a mix of v4 and v6 later? It would make sense but it will need
to be documented somewhere with a TODO I guess :)
Cheers,
Matt
PS: Paolo: is it OK for you if I apply the first patch of the series?
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
5:35 a.m.
New subject: [PATCH 3/3] mptcp: Use correct address family when creating subflows
On Tue, 2019-10-22 at 13:12 +0200, Matthieu Baerts wrote:
On 22/10/2019 10:35, Paolo Abeni wrote:
> but what if the peer sends an ADD_ADDR v4,
> and the local msk socket is a v6 (or vice versa)?
Sorry, I was not clear enough. With the posted code, when msk is v6,
pernet->has_announce_v6 is 1 and ADD_ADDR is ipv4 (pm->remote_family ==
AF_INET), I think mptcp_pm_create_subflow6() will end-up using
uninitialized bytes for mptcp_subflow_connect().
To avoid that, we should check that the local and the remote address
used to establish the connection belong to the same family.
So accept a mix of v4 and v6 later?
The idea would be to keep the code as simple as possible. The listening
socket in the current status can't accept easily a mix of ipv4 and ipv6
subflow, I think.
On the flip side, should be quite straigh-forward support mixed family
for client sockets; that was my overall proposal here.
It would make sense but it will need
to be documented somewhere with a TODO I guess :)
Agreed.
PS: Paolo: is it OK for you if I apply the first patch of the series?
too late, but... "yes"
Cheers,
Paolo
5:52 a.m.
New subject: [PATCH 3/3] mptcp: Use correct address family when creating subflows
Hi Paolo,
On 22/10/2019 14:35, Paolo Abeni wrote:
On Tue, 2019-10-22 at 13:12 +0200, Matthieu Baerts wrote:
> On 22/10/2019 10:35, Paolo Abeni wrote:
>> but what if the peer sends an ADD_ADDR v4,
>> and the local msk socket is a v6 (or vice versa)?
Sorry, I was not clear enough. With the posted code, when msk is v6,
pernet->has_announce_v6 is 1 and ADD_ADDR is ipv4 (pm->remote_family ==
AF_INET), I think mptcp_pm_create_subflow6() will end-up using
uninitialized bytes for mptcp_subflow_connect().
To avoid that, we should check that the local and the remote address
used to establish the connection belong to the same family.
I see, thank you for the clarification! Indeed, there is no check when
doing this, good catch!
remote.sin_family = msk->pm.remote_family
remote.sin6_family = msk->pm.remote_family
> So accept a mix of v4 and v6 later?
The idea would be to keep the code as simple as possible. The listening
socket in the current status can't accept easily a mix of ipv4 and ipv6
subflow, I think.
On the flip side, should be quite straigh-forward support mixed family
for client sockets; that was my overall proposal here.
Sounds like a good idea!
> It would make sense but it will need
> to be documented somewhere with a TODO I guess :)
Agreed.
> PS: Paolo: is it OK for you if I apply the first patch of the series?
too late, but... "yes"
Sorry, the fix was straightforward, I went ahead without waiting :)
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
2:28 p.m.
New subject: [PATCH 3/3] mptcp: Use correct address family when creating subflows
Thank-you for the review Paolo.
On Tue, 2019-10-22 at 10:35 +0200, Paolo Abeni wrote:
Hi,
On Mon, 2019-10-21 at 16:41 -0700, Peter Krystad wrote:
> Use the address family used when the initial MPTCP socket
> was created when making additional subflow sockets.
>
> squashto: Implement basic path manager
>
> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> ---
> net/mptcp/basic.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/net/mptcp/basic.c b/net/mptcp/basic.c
> index d2fd0560c136..2557cb63fd61 100644
> --- a/net/mptcp/basic.c
> +++ b/net/mptcp/basic.c
> @@ -209,19 +209,23 @@ static void create_subflow_worker(struct work_struct *work)
>
> pernet = net_generic(sock_net((struct sock *)msk), basic_pernet_id);
>
> - if (pernet->has_announce_v4)
> - mptcp_pm_create_subflow(pm->token, pm->remote_id,
> - &pernet->announce_v4_addr);
> -#if IS_ENABLED(CONFIG_IPV6)
> - else if (pernet->has_announce_v6)
> - mptcp_pm_create_subflow6(pm->token, pm->remote_id,
> - &pernet->announce_v6_addr);
> -#endif
> - else if (pm->local_family == AF_INET)
> - mptcp_pm_create_subflow(pm->token, pm->remote_id, NULL);
> + if (msk->family == AF_INET) {
> + if (pernet->has_announce_v4)
> + mptcp_pm_create_subflow(pm->token, pm->remote_id,
> + &pernet->announce_v4_addr);
> + else
> + mptcp_pm_create_subflow(pm->token, pm->remote_id,
> + NULL);
> + }
Here I'm likely a bit lost, but what if the peer sends an ADD_ADDR v4,
and the local msk socket is a v6 (or vice versa)?
I think checking vs the msk family is possibly not required here (IIRC
the RFC code, it looks like an MPTCP client could manage correctly
subflows with different inet families - but we need to check/match the
local address family (has_announce_v4/has_announce_v6) vs the remote
address/ADD_ADDR family. Otherwise we could try to connect to a
corrupted/truncated/uncomplete address.
You're right, checking the msk->family is not required, but checking against
the remote address family is. So client may start both types, but we won't be
able to test that against our own implementation, for now.
Additionally, I think we need to check/match the local additional
address family (has_announce_v4/has_announce_v6) vs the msk family in
announce_addr_worker(), so that the sever always announce addresses
that is able to accept[1].
Agreed, I'll add this.
Peter.
Thanks,
Paolo
[1] for IPv6 listener such check could be fairly more complex - unbound
listener could accept any ipv4 address, bound ones could check for
ipv4-mapped ipv6 address, etc... I think is better use to the simpler
approach here.
981
days inactive
983
days old
9 comments
3 participants
participants (3)
-
Matthieu Baerts -
Paolo Abeni -
Peter Krystad