All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH mptcp-net] mptcp: fix connect timeout handling
@ 2023-05-05 19:34 Paolo Abeni
  2023-05-05 20:39 ` mptcp: fix connect timeout handling: Tests Results MPTCP CI
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Paolo Abeni @ 2023-05-05 19:34 UTC (permalink / raw)
  To: mptcp; +Cc: Ondrej Mosnacek

Ondrej reported a functional issue WRT timeout handling on connect
with a nice reproducer.

The problem is that the current mptcp connect waits for both the
MPTCP socket level timeout, and the first subflow socket timeout.
The latter is not influenced/touched by the exposed setsockopt().

Overall the above makes the SO_SNDTIMEO a no-op on connect.

Since mptcp_connect is invoked via inet_stream_connect and the
latter properly handle the MPTCP level timeout, we can address the
issue making the nested subflow level connect always unblocking.

This also allow simplifying a bit the code, dropping an ugly hack
to handle the fastopen and custom proto_ops connect.

The issues predates the blamed commit below, but the current resolution
requires the infrastructure introduced there.

Fixes: 54f1944ed6d2 ("mptcp: factor out mptcp_connect()")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/399
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note: there is needed follow-up correctly propagating the SYN_SENT
-> CLOSE state transition from the first subflow to the msk socket in
case of connect error, but that is somewhat independed/pre-existent.

Sending out early for testing.
---
 net/mptcp/protocol.c | 29 +++++++----------------------
 net/mptcp/protocol.h |  1 -
 2 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f5842f93c891..4747058a8b6b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1734,7 +1734,6 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
 
 	lock_sock(ssk);
 	msg->msg_flags |= MSG_DONTWAIT;
-	msk->connect_flags = O_NONBLOCK;
 	msk->fastopening = 1;
 	ret = tcp_sendmsg_fastopen(ssk, msg, copied_syn, len, NULL);
 	msk->fastopening = 0;
@@ -3672,9 +3671,9 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	 * acquired the subflow socket lock, too.
 	 */
 	if (msk->fastopening)
-		err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
+		err = __inet_stream_connect(ssock, uaddr, addr_len, O_NONBLOCK, 1);
 	else
-		err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
+		err = inet_stream_connect(ssock, uaddr, addr_len, O_NONBLOCK);
 	inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
 
 	/* on successful connect, the msk state will be moved to established by
@@ -3687,12 +3686,10 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 
 	mptcp_copy_inaddrs(sk, ssock->sk);
 
-	/* unblocking connect, mptcp-level inet_stream_connect will error out
-	 * without changing the socket state, update it here.
+	/* silence EINPROGRESS and let the caller inet_stream_connect
+	 * handle the connection in progress
 	 */
-	if (err == -EINPROGRESS)
-		sk->sk_socket->state = ssock->state;
-	return err;
+	return 0;
 }
 
 static struct proto mptcp_prot = {
@@ -3751,18 +3748,6 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	return err;
 }
 
-static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
-				int addr_len, int flags)
-{
-	int ret;
-
-	lock_sock(sock->sk);
-	mptcp_sk(sock->sk)->connect_flags = flags;
-	ret = __inet_stream_connect(sock, uaddr, addr_len, flags, 0);
-	release_sock(sock->sk);
-	return ret;
-}
-
 static int mptcp_listen(struct socket *sock, int backlog)
 {
 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
@@ -3917,7 +3902,7 @@ static const struct proto_ops mptcp_stream_ops = {
 	.owner		   = THIS_MODULE,
 	.release	   = inet_release,
 	.bind		   = mptcp_bind,
-	.connect	   = mptcp_stream_connect,
+	.connect	   = inet_stream_connect,
 	.socketpair	   = sock_no_socketpair,
 	.accept		   = mptcp_stream_accept,
 	.getname	   = inet_getname,
@@ -4012,7 +3997,7 @@ static const struct proto_ops mptcp_v6_stream_ops = {
 	.owner		   = THIS_MODULE,
 	.release	   = inet6_release,
 	.bind		   = mptcp_bind,
-	.connect	   = mptcp_stream_connect,
+	.connect	   = inet_stream_connect,
 	.socketpair	   = sock_no_socketpair,
 	.accept		   = mptcp_stream_accept,
 	.getname	   = inet6_getname,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 8d9d87e2f840..39613bba1c46 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -300,7 +300,6 @@ struct mptcp_sock {
 			nodelay:1,
 			fastopening:1,
 			in_accept_queue:1;
-	int		connect_flags;
 	struct work_struct work;
 	struct sk_buff  *ooo_last_skb;
 	struct rb_root  out_of_order_queue;
-- 
2.40.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: mptcp: fix connect timeout handling: Tests Results
  2023-05-05 19:34 [PATCH mptcp-net] mptcp: fix connect timeout handling Paolo Abeni
@ 2023-05-05 20:39 ` MPTCP CI
  2023-05-08  9:32 ` [PATCH mptcp-net] mptcp: fix connect timeout handling Paolo Abeni
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: MPTCP CI @ 2023-05-05 20:39 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: mptcp

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/5039504482893824
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5039504482893824/summary/summary.txt

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

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

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

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


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)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH mptcp-net] mptcp: fix connect timeout handling
  2023-05-05 19:34 [PATCH mptcp-net] mptcp: fix connect timeout handling Paolo Abeni
  2023-05-05 20:39 ` mptcp: fix connect timeout handling: Tests Results MPTCP CI
@ 2023-05-08  9:32 ` Paolo Abeni
  2023-05-12 19:24 ` Mat Martineau
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Paolo Abeni @ 2023-05-08  9:32 UTC (permalink / raw)
  To: mptcp; +Cc: Ondrej Mosnacek

On Fri, 2023-05-05 at 21:34 +0200, Paolo Abeni wrote:
> Note: there is needed follow-up correctly propagating the SYN_SENT
> -> CLOSE state transition from the first subflow to the msk socket in
> case of connect error, but that is somewhat independed/pre-existent.

Re-reading the relevant code, there is no need for such follow-up:
commit 1249db44a102 ("mptcp: be careful on subflow status propagation
on errors") already take care of it.

Cheers,

Paolo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH mptcp-net] mptcp: fix connect timeout handling
  2023-05-05 19:34 [PATCH mptcp-net] mptcp: fix connect timeout handling Paolo Abeni
  2023-05-05 20:39 ` mptcp: fix connect timeout handling: Tests Results MPTCP CI
  2023-05-08  9:32 ` [PATCH mptcp-net] mptcp: fix connect timeout handling Paolo Abeni
@ 2023-05-12 19:24 ` Mat Martineau
  2023-05-15  8:16   ` Paolo Abeni
  2023-05-12 21:10 ` mptcp: fix connect timeout handling: Tests Results MPTCP CI
  2023-05-15 15:58 ` [PATCH mptcp-net] mptcp: fix connect timeout handling Matthieu Baerts
  4 siblings, 1 reply; 9+ messages in thread
From: Mat Martineau @ 2023-05-12 19:24 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: mptcp, Ondrej Mosnacek

On Fri, 5 May 2023, Paolo Abeni wrote:

> Ondrej reported a functional issue WRT timeout handling on connect
> with a nice reproducer.
>
> The problem is that the current mptcp connect waits for both the
> MPTCP socket level timeout, and the first subflow socket timeout.
> The latter is not influenced/touched by the exposed setsockopt().
>
> Overall the above makes the SO_SNDTIMEO a no-op on connect.
>
> Since mptcp_connect is invoked via inet_stream_connect and the
> latter properly handle the MPTCP level timeout, we can address the
> issue making the nested subflow level connect always unblocking.
>
> This also allow simplifying a bit the code, dropping an ugly hack
> to handle the fastopen and custom proto_ops connect.
>
> The issues predates the blamed commit below, but the current resolution
> requires the infrastructure introduced there.
>
> Fixes: 54f1944ed6d2 ("mptcp: factor out mptcp_connect()")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/399
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> Note: there is needed follow-up correctly propagating the SYN_SENT
> -> CLOSE state transition from the first subflow to the msk socket in
> case of connect error, but that is somewhat independed/pre-existent.
>
> Sending out early for testing.

Hi Paolo -

Patch LGTM, and appreciate the cleanup of msk->connect_flags:

Reviewed-by: Mat Martineau <martineau@kernel.org>

I think it would be good to also add a selftest in net-next to check the 
connect timeout (set to a very short time), but that doesn't need to block 
this -net change.

Thanks,
Mat

> ---
> net/mptcp/protocol.c | 29 +++++++----------------------
> net/mptcp/protocol.h |  1 -
> 2 files changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f5842f93c891..4747058a8b6b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1734,7 +1734,6 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>
> 	lock_sock(ssk);
> 	msg->msg_flags |= MSG_DONTWAIT;
> -	msk->connect_flags = O_NONBLOCK;
> 	msk->fastopening = 1;
> 	ret = tcp_sendmsg_fastopen(ssk, msg, copied_syn, len, NULL);
> 	msk->fastopening = 0;
> @@ -3672,9 +3671,9 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
> 	 * acquired the subflow socket lock, too.
> 	 */
> 	if (msk->fastopening)
> -		err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
> +		err = __inet_stream_connect(ssock, uaddr, addr_len, O_NONBLOCK, 1);
> 	else
> -		err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
> +		err = inet_stream_connect(ssock, uaddr, addr_len, O_NONBLOCK);
> 	inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
>
> 	/* on successful connect, the msk state will be moved to established by
> @@ -3687,12 +3686,10 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>
> 	mptcp_copy_inaddrs(sk, ssock->sk);
>
> -	/* unblocking connect, mptcp-level inet_stream_connect will error out
> -	 * without changing the socket state, update it here.
> +	/* silence EINPROGRESS and let the caller inet_stream_connect
> +	 * handle the connection in progress
> 	 */
> -	if (err == -EINPROGRESS)
> -		sk->sk_socket->state = ssock->state;
> -	return err;
> +	return 0;
> }
>
> static struct proto mptcp_prot = {
> @@ -3751,18 +3748,6 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> 	return err;
> }
>
> -static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> -				int addr_len, int flags)
> -{
> -	int ret;
> -
> -	lock_sock(sock->sk);
> -	mptcp_sk(sock->sk)->connect_flags = flags;
> -	ret = __inet_stream_connect(sock, uaddr, addr_len, flags, 0);
> -	release_sock(sock->sk);
> -	return ret;
> -}
> -
> static int mptcp_listen(struct socket *sock, int backlog)
> {
> 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
> @@ -3917,7 +3902,7 @@ static const struct proto_ops mptcp_stream_ops = {
> 	.owner		   = THIS_MODULE,
> 	.release	   = inet_release,
> 	.bind		   = mptcp_bind,
> -	.connect	   = mptcp_stream_connect,
> +	.connect	   = inet_stream_connect,
> 	.socketpair	   = sock_no_socketpair,
> 	.accept		   = mptcp_stream_accept,
> 	.getname	   = inet_getname,
> @@ -4012,7 +3997,7 @@ static const struct proto_ops mptcp_v6_stream_ops = {
> 	.owner		   = THIS_MODULE,
> 	.release	   = inet6_release,
> 	.bind		   = mptcp_bind,
> -	.connect	   = mptcp_stream_connect,
> +	.connect	   = inet_stream_connect,
> 	.socketpair	   = sock_no_socketpair,
> 	.accept		   = mptcp_stream_accept,
> 	.getname	   = inet6_getname,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 8d9d87e2f840..39613bba1c46 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -300,7 +300,6 @@ struct mptcp_sock {
> 			nodelay:1,
> 			fastopening:1,
> 			in_accept_queue:1;
> -	int		connect_flags;
> 	struct work_struct work;
> 	struct sk_buff  *ooo_last_skb;
> 	struct rb_root  out_of_order_queue;
> -- 
> 2.40.0
>
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: mptcp: fix connect timeout handling: Tests Results
  2023-05-05 19:34 [PATCH mptcp-net] mptcp: fix connect timeout handling Paolo Abeni
                   ` (2 preceding siblings ...)
  2023-05-12 19:24 ` Mat Martineau
@ 2023-05-12 21:10 ` MPTCP CI
  2023-05-15 15:58 ` [PATCH mptcp-net] mptcp: fix connect timeout handling Matthieu Baerts
  4 siblings, 0 replies; 9+ messages in thread
From: MPTCP CI @ 2023-05-12 21:10 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: mptcp

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):
  - Unstable: 1 failed test(s): packetdrill_fastopen 🔴:
  - Task: https://cirrus-ci.com/task/4891499171676160
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4891499171676160/summary/summary.txt

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

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 4 failed test(s): packetdrill_add_addr packetdrill_fastopen packetdrill_syscalls selftest_diag 🔴:
  - Task: https://cirrus-ci.com/task/6661833889677312
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6661833889677312/summary/summary.txt

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

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


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)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH mptcp-net] mptcp: fix connect timeout handling
  2023-05-12 19:24 ` Mat Martineau
@ 2023-05-15  8:16   ` Paolo Abeni
  2023-05-15 15:46     ` Matthieu Baerts
  0 siblings, 1 reply; 9+ messages in thread
From: Paolo Abeni @ 2023-05-15  8:16 UTC (permalink / raw)
  To: Mat Martineau; +Cc: mptcp, Ondrej Mosnacek

On Fri, 2023-05-12 at 12:24 -0700, Mat Martineau wrote:
> On Fri, 5 May 2023, Paolo Abeni wrote:
> 
> > Ondrej reported a functional issue WRT timeout handling on connect
> > with a nice reproducer.
> > 
> > The problem is that the current mptcp connect waits for both the
> > MPTCP socket level timeout, and the first subflow socket timeout.
> > The latter is not influenced/touched by the exposed setsockopt().
> > 
> > Overall the above makes the SO_SNDTIMEO a no-op on connect.
> > 
> > Since mptcp_connect is invoked via inet_stream_connect and the
> > latter properly handle the MPTCP level timeout, we can address the
> > issue making the nested subflow level connect always unblocking.
> > 
> > This also allow simplifying a bit the code, dropping an ugly hack
> > to handle the fastopen and custom proto_ops connect.
> > 
> > The issues predates the blamed commit below, but the current resolution
> > requires the infrastructure introduced there.
> > 
> > Fixes: 54f1944ed6d2 ("mptcp: factor out mptcp_connect()")
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/399
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > Note: there is needed follow-up correctly propagating the SYN_SENT
> > -> CLOSE state transition from the first subflow to the msk socket in
> > case of connect error, but that is somewhat independed/pre-existent.
> > 
> > Sending out early for testing.
> 
> Hi Paolo -
> 
> Patch LGTM, and appreciate the cleanup of msk->connect_flags:
> 
> Reviewed-by: Mat Martineau <martineau@kernel.org>
> 
> I think it would be good to also add a selftest in net-next to check the 
> connect timeout (set to a very short time), but that doesn't need to block 
> this -net change.

Agreed. Perhaps a pktdrill script would could be simpler option?!?

Thanks!

Paolo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH mptcp-net] mptcp: fix connect timeout handling
  2023-05-15  8:16   ` Paolo Abeni
@ 2023-05-15 15:46     ` Matthieu Baerts
  2023-05-15 17:19       ` Paolo Abeni
  0 siblings, 1 reply; 9+ messages in thread
From: Matthieu Baerts @ 2023-05-15 15:46 UTC (permalink / raw)
  To: Paolo Abeni, Mat Martineau; +Cc: mptcp, Ondrej Mosnacek

Hi Paolo, Mat,

On 15/05/2023 10:16, Paolo Abeni wrote:
> On Fri, 2023-05-12 at 12:24 -0700, Mat Martineau wrote:
>> On Fri, 5 May 2023, Paolo Abeni wrote:
>>
>>> Ondrej reported a functional issue WRT timeout handling on connect
>>> with a nice reproducer.
>>>
>>> The problem is that the current mptcp connect waits for both the
>>> MPTCP socket level timeout, and the first subflow socket timeout.
>>> The latter is not influenced/touched by the exposed setsockopt().
>>>
>>> Overall the above makes the SO_SNDTIMEO a no-op on connect.
>>>
>>> Since mptcp_connect is invoked via inet_stream_connect and the
>>> latter properly handle the MPTCP level timeout, we can address the
>>> issue making the nested subflow level connect always unblocking.
>>>
>>> This also allow simplifying a bit the code, dropping an ugly hack
>>> to handle the fastopen and custom proto_ops connect.
>>>
>>> The issues predates the blamed commit below, but the current resolution
>>> requires the infrastructure introduced there.
>>>
>>> Fixes: 54f1944ed6d2 ("mptcp: factor out mptcp_connect()")
>>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/399
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> Note: there is needed follow-up correctly propagating the SYN_SENT
>>> -> CLOSE state transition from the first subflow to the msk socket in
>>> case of connect error, but that is somewhat independed/pre-existent.
>>>
>>> Sending out early for testing.
>>
>> Hi Paolo -
>>
>> Patch LGTM, and appreciate the cleanup of msk->connect_flags:
>>
>> Reviewed-by: Mat Martineau <martineau@kernel.org>
>>
>> I think it would be good to also add a selftest in net-next to check the 
>> connect timeout (set to a very short time), but that doesn't need to block 
>> this -net change.
> 
> Agreed. Perhaps a pktdrill script would could be simpler option?!?

Good idea, that seems more appropriated, especially with the timeout part.

Just to avoid deadlocks: is there someone working on that? :)

But as Mat said, this is not blocking, I can apply the patch.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH mptcp-net] mptcp: fix connect timeout handling
  2023-05-05 19:34 [PATCH mptcp-net] mptcp: fix connect timeout handling Paolo Abeni
                   ` (3 preceding siblings ...)
  2023-05-12 21:10 ` mptcp: fix connect timeout handling: Tests Results MPTCP CI
@ 2023-05-15 15:58 ` Matthieu Baerts
  4 siblings, 0 replies; 9+ messages in thread
From: Matthieu Baerts @ 2023-05-15 15:58 UTC (permalink / raw)
  To: Paolo Abeni, Mat Martineau; +Cc: Ondrej Mosnacek, mptcp

Hi Paolo, Mat,

On 05/05/2023 21:34, Paolo Abeni wrote:
> Ondrej reported a functional issue WRT timeout handling on connect
> with a nice reproducer.
> 
> The problem is that the current mptcp connect waits for both the
> MPTCP socket level timeout, and the first subflow socket timeout.
> The latter is not influenced/touched by the exposed setsockopt().
> 
> Overall the above makes the SO_SNDTIMEO a no-op on connect.
> 
> Since mptcp_connect is invoked via inet_stream_connect and the
> latter properly handle the MPTCP level timeout, we can address the
> issue making the nested subflow level connect always unblocking.
> 
> This also allow simplifying a bit the code, dropping an ugly hack
> to handle the fastopen and custom proto_ops connect.
> 
> The issues predates the blamed commit below, but the current resolution
> requires the infrastructure introduced there.
> 
> Fixes: 54f1944ed6d2 ("mptcp: factor out mptcp_connect()")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/399
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Thank you for the fix and the review!

I just applied it in our repo with Mat's RvB tag. I also added Ondrej's
Reported-by tag.

New patches for t/upstream-net and t/upstream:
- 59d33b72ee59: mptcp: fix connect timeout handling
- 8d8fd087a52c: tg:msg: add Ondrej's Reported-by tag
- Results: 8ecd3b5ba105..fa4a5780caa4 (export-net)
- Results: d9d25c574c4e..b8e056f78785 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230515T155446
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230515T155446

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH mptcp-net] mptcp: fix connect timeout handling
  2023-05-15 15:46     ` Matthieu Baerts
@ 2023-05-15 17:19       ` Paolo Abeni
  0 siblings, 0 replies; 9+ messages in thread
From: Paolo Abeni @ 2023-05-15 17:19 UTC (permalink / raw)
  To: Matthieu Baerts, Mat Martineau; +Cc: mptcp, Ondrej Mosnacek

On Mon, 2023-05-15 at 17:46 +0200, Matthieu Baerts wrote:
> Hi Paolo, Mat,
> 
> On 15/05/2023 10:16, Paolo Abeni wrote:
> > On Fri, 2023-05-12 at 12:24 -0700, Mat Martineau wrote:
> > > On Fri, 5 May 2023, Paolo Abeni wrote:
> > > 
> > > > Ondrej reported a functional issue WRT timeout handling on connect
> > > > with a nice reproducer.
> > > > 
> > > > The problem is that the current mptcp connect waits for both the
> > > > MPTCP socket level timeout, and the first subflow socket timeout.
> > > > The latter is not influenced/touched by the exposed setsockopt().
> > > > 
> > > > Overall the above makes the SO_SNDTIMEO a no-op on connect.
> > > > 
> > > > Since mptcp_connect is invoked via inet_stream_connect and the
> > > > latter properly handle the MPTCP level timeout, we can address the
> > > > issue making the nested subflow level connect always unblocking.
> > > > 
> > > > This also allow simplifying a bit the code, dropping an ugly hack
> > > > to handle the fastopen and custom proto_ops connect.
> > > > 
> > > > The issues predates the blamed commit below, but the current resolution
> > > > requires the infrastructure introduced there.
> > > > 
> > > > Fixes: 54f1944ed6d2 ("mptcp: factor out mptcp_connect()")
> > > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/399
> > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > > ---
> > > > Note: there is needed follow-up correctly propagating the SYN_SENT
> > > > -> CLOSE state transition from the first subflow to the msk socket in
> > > > case of connect error, but that is somewhat independed/pre-existent.
> > > > 
> > > > Sending out early for testing.
> > > 
> > > Hi Paolo -
> > > 
> > > Patch LGTM, and appreciate the cleanup of msk->connect_flags:
> > > 
> > > Reviewed-by: Mat Martineau <martineau@kernel.org>
> > > 
> > > I think it would be good to also add a selftest in net-next to check the 
> > > connect timeout (set to a very short time), but that doesn't need to block 
> > > this -net change.
> > 
> > Agreed. Perhaps a pktdrill script would could be simpler option?!?
> 
> Good idea, that seems more appropriated, especially with the timeout part.
> 
> Just to avoid deadlocks: is there someone working on that? :)

Yep, just opended a PR:

https://github.com/multipath-tcp/packetdrill/pull/115

/P


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-05-15 17:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-05 19:34 [PATCH mptcp-net] mptcp: fix connect timeout handling Paolo Abeni
2023-05-05 20:39 ` mptcp: fix connect timeout handling: Tests Results MPTCP CI
2023-05-08  9:32 ` [PATCH mptcp-net] mptcp: fix connect timeout handling Paolo Abeni
2023-05-12 19:24 ` Mat Martineau
2023-05-15  8:16   ` Paolo Abeni
2023-05-15 15:46     ` Matthieu Baerts
2023-05-15 17:19       ` Paolo Abeni
2023-05-12 21:10 ` mptcp: fix connect timeout handling: Tests Results MPTCP CI
2023-05-15 15:58 ` [PATCH mptcp-net] mptcp: fix connect timeout handling Matthieu Baerts

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.