[PATCH v2 mptcp-net 1/2] mptcp: handle correctly disconnect() failures

[PATCH v2 mptcp-net 1/2] mptcp: handle correctly disconnect() failures

[Paolo Abeni]

Currently the mptcp code has assumes that disconnect() can fail only
at mptcp_sendmsg_fastopen() time - to avoid a deadlock scenario - and
don't even bother returning an error code.

Soon mptcp_disconnect() will handle more error conditions: let's track
them explicitly.

As a bonus, explicitly annotate TCP-level disconnect as not failing:
the mptcp code never blocks for event on the subflows.

Fixes: 7d803344fdc3 ("mptcp: fix deadlock in fastopen error path")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d8e152a8619a..ce1a70ad37cf 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1759,7 +1759,13 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
 		if (ret && ret != -EINPROGRESS && ret != -ERESTARTSYS && ret != -EINTR)
 			*copied_syn = 0;
 	} else if (ret && ret != -EINPROGRESS) {
-		mptcp_disconnect(sk, 0);
+		/* The disconnect() op called by tcp_sendmsg_fastopen()/
+		 * __inet_stream_connect() is failed, due to looking check,
+		 * see mptcp_disconnect().
+		 * Attempt it again outside the problematic scope.
+		 */
+		if (!mptcp_disconnect(sk, 0))
+			sk->sk_socket->state = SS_UNCONNECTED;
 	}
 	inet_sk(sk)->defer_connect = 0;
 
@@ -2424,7 +2430,10 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 
 	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
 	if (!dispose_it) {
-		tcp_disconnect(ssk, 0);
+		/* The MPTCP code never wait on the subflow sockets, TCP-level
+		 * disconnect should never fail
+		 */
+		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
 		msk->subflow->state = SS_UNCONNECTED;
 		mptcp_subflow_ctx_reset(subflow);
 		release_sock(ssk);
@@ -2870,7 +2879,7 @@ void mptcp_subflow_shutdown(struct sock *sk, struct sock *ssk, int how)
 			break;
 		fallthrough;
 	case TCP_SYN_SENT:
-		tcp_disconnect(ssk, O_NONBLOCK);
+		WARN_ON_ONCE(tcp_disconnect(ssk, O_NONBLOCK));
 		break;
 	default:
 		if (__mptcp_check_fallback(mptcp_sk(sk))) {
@@ -3134,11 +3143,10 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 
 	/* We are on the fastopen error path. We can't call straight into the
 	 * subflows cleanup code due to lock nesting (we are already under
-	 * msk->firstsocket lock). Do nothing and leave the cleanup to the
-	 * caller.
+	 * msk->firstsocket lock).
 	 */
 	if (msk->fastopening)
-		return 0;
+		return -EBUSY;
 
 	mptcp_listen_inuse_dec(sk);
 	inet_sk_state_store(sk, TCP_CLOSE);
-- 
2.40.1

[PATCH v2 mptcp-net 2/2] mptcp: fix possible divide by zero in recvmsg()

[Paolo Abeni]

Christoph reported a devide by zero bug in mptcp_recvmsg():

divide error: 0000 [#1] PREEMPT SMP
CPU: 1 PID: 19978 Comm: syz-executor.6 Not tainted 6.4.0-rc2-gffcc7899081b #20
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014
RIP: 0010:__tcp_select_window+0x30e/0x420 net/ipv4/tcp_output.c:3018
Code: 11 ff 0f b7 cd c1 e9 0c b8 ff ff ff ff d3 e0 89 c1 f7 d1 01 cb 21 c3 eb 17 e8 2e 83 11 ff 31 db eb 0e e8 25 83 11 ff 89 d8 99 <f7> 7c 24 04 29 d3 65 48 8b 04 25 28 00 00 00 48 3b 44 24 10 75 60
RSP: 0018:ffffc90000a07a18 EFLAGS: 00010246
RAX: 000000000000ffd7 RBX: 000000000000ffd7 RCX: 0000000000040000
RDX: 0000000000000000 RSI: 000000000003ffff RDI: 0000000000040000
RBP: 000000000000ffd7 R08: ffffffff820cf297 R09: 0000000000000001
R10: 0000000000000000 R11: ffffffff8103d1a0 R12: 0000000000003f00
R13: 0000000000300000 R14: ffff888101cf3540 R15: 0000000000180000
FS:  00007f9af4c09640(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b33824000 CR3: 000000012f241001 CR4: 0000000000170ee0
Call Trace:
 <TASK>
 __tcp_cleanup_rbuf+0x138/0x1d0 net/ipv4/tcp.c:1611
 mptcp_recvmsg+0xcb8/0xdd0 net/mptcp/protocol.c:2034
 inet_recvmsg+0x127/0x1f0 net/ipv4/af_inet.c:861
 ____sys_recvmsg+0x269/0x2b0 net/socket.c:1019
 ___sys_recvmsg+0xe6/0x260 net/socket.c:2764
 do_recvmmsg+0x1a5/0x470 net/socket.c:2858
 __do_sys_recvmmsg net/socket.c:2937 [inline]
 __se_sys_recvmmsg net/socket.c:2953 [inline]
 __x64_sys_recvmmsg+0xa6/0x130 net/socket.c:2953
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x47/0xa0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x72/0xdc
RIP: 0033:0x7f9af58fc6a9
Code: 5c c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 4f 37 0d 00 f7 d8 64 89 01 48
RSP: 002b:00007f9af4c08cd8 EFLAGS: 00000246 ORIG_RAX: 000000000000012b
RAX: ffffffffffffffda RBX: 00000000006bc050 RCX: 00007f9af58fc6a9
RDX: 0000000000000001 RSI: 0000000020000140 RDI: 0000000000000004
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000f00 R11: 0000000000000246 R12: 00000000006bc05c
R13: fffffffffffffea8 R14: 00000000006bc050 R15: 000000000001fe40
 </TASK>

mptcp_recvmsg is allowed to release the msk socket lock when
blocking, and before re-acquiring it another thread could have
switched the sock to TCP_LISTEN status - with a prior
connect(AF_UNSPEC) - also clearing icsk_ack.rcv_mss.

Address the issue preventing the disconnect if some other process is
concurrently performing a blocking syscall on the same socket, alike
commit XXXXXXXXX ("tcp: deny tcp_disconnect() when threads are waiting").

Fixes: a6b118febbab ("mptcp: add receive buffer auto-tuning")
Reported-by: Christoph Paasch <cpaasch@apple.com>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/404
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
  - refactor on top of the TCP fix
note:
  - the hash of the TCP fix is missing, such commit should be applied
    today or tomorrow morning in the worst-case scenario.
---
 net/mptcp/protocol.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ce1a70ad37cf..585c84f0b0eb 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3141,6 +3141,12 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 
+	/* Deny disconnect if other threads are blocked in sk_wait_event()
+	 * or inet_wait_for_connect().
+	 */
+	if (sk->sk_wait_pending)
+		return -EBUSY;
+
 	/* We are on the fastopen error path. We can't call straight into the
 	 * subflows cleanup code due to lock nesting (we are already under
 	 * msk->firstsocket lock).
-- 
2.40.1

Re: mptcp: fix possible divide by zero in recvmsg(): Build Failure

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/42bb08e7d5ec3c1cc5cba43fa1215f80255e99a9.1685367663.git.pabeni@redhat.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/5113790043

Status: cancelled
Initiator: matttbe
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/2dfdc2932f2e

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

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

Re: mptcp: fix possible divide by zero in recvmsg(): 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):
  - Unstable: 1 failed test(s): packetdrill_fastopen 🔴:
  - Task: https://cirrus-ci.com/task/6203727577612288
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6203727577612288/summary/summary.txt

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

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

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

Initiator: Matthieu Baerts
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/2dfdc2932f2e


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

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

For more details:

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


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

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

Re: [PATCH v2 mptcp-net 2/2] mptcp: fix possible divide by zero in recvmsg()

[Christoph Paasch]

> On May 29, 2023, at 6:41 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> Christoph reported a devide by zero bug in mptcp_recvmsg():
> 
> divide error: 0000 [#1] PREEMPT SMP
> CPU: 1 PID: 19978 Comm: syz-executor.6 Not tainted 6.4.0-rc2-gffcc7899081b #20
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014
> RIP: 0010:__tcp_select_window+0x30e/0x420 net/ipv4/tcp_output.c:3018
> Code: 11 ff 0f b7 cd c1 e9 0c b8 ff ff ff ff d3 e0 89 c1 f7 d1 01 cb 21 c3 eb 17 e8 2e 83 11 ff 31 db eb 0e e8 25 83 11 ff 89 d8 99 <f7> 7c 24 04 29 d3 65 48 8b 04 25 28 00 00 00 48 3b 44 24 10 75 60
> RSP: 0018:ffffc90000a07a18 EFLAGS: 00010246
> RAX: 000000000000ffd7 RBX: 000000000000ffd7 RCX: 0000000000040000
> RDX: 0000000000000000 RSI: 000000000003ffff RDI: 0000000000040000
> RBP: 000000000000ffd7 R08: ffffffff820cf297 R09: 0000000000000001
> R10: 0000000000000000 R11: ffffffff8103d1a0 R12: 0000000000003f00
> R13: 0000000000300000 R14: ffff888101cf3540 R15: 0000000000180000
> FS:  00007f9af4c09640(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b33824000 CR3: 000000012f241001 CR4: 0000000000170ee0
> Call Trace:
> <TASK>
> __tcp_cleanup_rbuf+0x138/0x1d0 net/ipv4/tcp.c:1611
> mptcp_recvmsg+0xcb8/0xdd0 net/mptcp/protocol.c:2034
> inet_recvmsg+0x127/0x1f0 net/ipv4/af_inet.c:861
> ____sys_recvmsg+0x269/0x2b0 net/socket.c:1019
> ___sys_recvmsg+0xe6/0x260 net/socket.c:2764
> do_recvmmsg+0x1a5/0x470 net/socket.c:2858
> __do_sys_recvmmsg net/socket.c:2937 [inline]
> __se_sys_recvmmsg net/socket.c:2953 [inline]
> __x64_sys_recvmmsg+0xa6/0x130 net/socket.c:2953
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x47/0xa0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x72/0xdc
> RIP: 0033:0x7f9af58fc6a9
> Code: 5c c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 4f 37 0d 00 f7 d8 64 89 01 48
> RSP: 002b:00007f9af4c08cd8 EFLAGS: 00000246 ORIG_RAX: 000000000000012b
> RAX: ffffffffffffffda RBX: 00000000006bc050 RCX: 00007f9af58fc6a9
> RDX: 0000000000000001 RSI: 0000000020000140 RDI: 0000000000000004
> RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000f00 R11: 0000000000000246 R12: 00000000006bc05c
> R13: fffffffffffffea8 R14: 00000000006bc050 R15: 000000000001fe40
> </TASK>
> 
> mptcp_recvmsg is allowed to release the msk socket lock when
> blocking, and before re-acquiring it another thread could have
> switched the sock to TCP_LISTEN status - with a prior
> connect(AF_UNSPEC) - also clearing icsk_ack.rcv_mss.
> 
> Address the issue preventing the disconnect if some other process is
> concurrently performing a blocking syscall on the same socket, alike
> commit XXXXXXXXX ("tcp: deny tcp_disconnect() when threads are waiting").
> 
> Fixes: a6b118febbab ("mptcp: add receive buffer auto-tuning")
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/404
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>  - refactor on top of the TCP fix
> note:
>  - the hash of the TCP fix is missing, such commit should be applied
>    today or tomorrow morning in the worst-case scenario.
> ---
> net/mptcp/protocol.c | 6 ++++++
> 1 file changed, 6 insertions(+)

Same on this one - it works!

Tested-by: Christoph Paasch <cpaasch@apple.com>

> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index ce1a70ad37cf..585c84f0b0eb 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3141,6 +3141,12 @@ static int mptcp_disconnect(struct sock *sk, int flags)
> {
> 	struct mptcp_sock *msk = mptcp_sk(sk);
> 
> +	/* Deny disconnect if other threads are blocked in sk_wait_event()
> +	 * or inet_wait_for_connect().
> +	 */
> +	if (sk->sk_wait_pending)
> +		return -EBUSY;
> +
> 	/* We are on the fastopen error path. We can't call straight into the
> 	 * subflows cleanup code due to lock nesting (we are already under
> 	 * msk->firstsocket lock).
> -- 
> 2.40.1
> 
> 

Re: [PATCH v2 mptcp-net 2/2] mptcp: fix possible divide by zero in recvmsg()

[Matthieu Baerts]

Hi Paolo,

On 29/05/2023 15:41, Paolo Abeni wrote:
> Christoph reported a devide by zero bug in mptcp_recvmsg():
> 
> divide error: 0000 [#1] PREEMPT SMP
> CPU: 1 PID: 19978 Comm: syz-executor.6 Not tainted 6.4.0-rc2-gffcc7899081b #20
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014
> RIP: 0010:__tcp_select_window+0x30e/0x420 net/ipv4/tcp_output.c:3018
> Code: 11 ff 0f b7 cd c1 e9 0c b8 ff ff ff ff d3 e0 89 c1 f7 d1 01 cb 21 c3 eb 17 e8 2e 83 11 ff 31 db eb 0e e8 25 83 11 ff 89 d8 99 <f7> 7c 24 04 29 d3 65 48 8b 04 25 28 00 00 00 48 3b 44 24 10 75 60
> RSP: 0018:ffffc90000a07a18 EFLAGS: 00010246
> RAX: 000000000000ffd7 RBX: 000000000000ffd7 RCX: 0000000000040000
> RDX: 0000000000000000 RSI: 000000000003ffff RDI: 0000000000040000
> RBP: 000000000000ffd7 R08: ffffffff820cf297 R09: 0000000000000001
> R10: 0000000000000000 R11: ffffffff8103d1a0 R12: 0000000000003f00
> R13: 0000000000300000 R14: ffff888101cf3540 R15: 0000000000180000
> FS:  00007f9af4c09640(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b33824000 CR3: 000000012f241001 CR4: 0000000000170ee0
> Call Trace:
>  <TASK>
>  __tcp_cleanup_rbuf+0x138/0x1d0 net/ipv4/tcp.c:1611
>  mptcp_recvmsg+0xcb8/0xdd0 net/mptcp/protocol.c:2034
>  inet_recvmsg+0x127/0x1f0 net/ipv4/af_inet.c:861
>  ____sys_recvmsg+0x269/0x2b0 net/socket.c:1019
>  ___sys_recvmsg+0xe6/0x260 net/socket.c:2764
>  do_recvmmsg+0x1a5/0x470 net/socket.c:2858
>  __do_sys_recvmmsg net/socket.c:2937 [inline]
>  __se_sys_recvmmsg net/socket.c:2953 [inline]
>  __x64_sys_recvmmsg+0xa6/0x130 net/socket.c:2953
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x47/0xa0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> RIP: 0033:0x7f9af58fc6a9
> Code: 5c c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 4f 37 0d 00 f7 d8 64 89 01 48
> RSP: 002b:00007f9af4c08cd8 EFLAGS: 00000246 ORIG_RAX: 000000000000012b
> RAX: ffffffffffffffda RBX: 00000000006bc050 RCX: 00007f9af58fc6a9
> RDX: 0000000000000001 RSI: 0000000020000140 RDI: 0000000000000004
> RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000f00 R11: 0000000000000246 R12: 00000000006bc05c
> R13: fffffffffffffea8 R14: 00000000006bc050 R15: 000000000001fe40
>  </TASK>
> 
> mptcp_recvmsg is allowed to release the msk socket lock when
> blocking, and before re-acquiring it another thread could have
> switched the sock to TCP_LISTEN status - with a prior
> connect(AF_UNSPEC) - also clearing icsk_ack.rcv_mss.
> 
> Address the issue preventing the disconnect if some other process is
> concurrently performing a blocking syscall on the same socket, alike
> commit XXXXXXXXX ("tcp: deny tcp_disconnect() when threads are waiting").

FYI, I used: 4faeee0cf8a5

> Fixes: a6b118febbab ("mptcp: add receive buffer auto-tuning")
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/404
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>   - refactor on top of the TCP fix
> note:
>   - the hash of the TCP fix is missing, such commit should be applied
>     today or tomorrow morning in the worst-case scenario.

Thank you for the patches and the tests!

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Now in our tree (fixes for -net) without typos reported by codespell:

New patches for t/upstream-net and t/upstream:
- 1e8b6536343a: mptcp: handle correctly disconnect() failures
- ffcd0af9b916: mptcp: fix possible divide by zero in recvmsg()
- Results: cb95946976d7..978898338078 (export-net)
- Results: d5bf0f97d0f0..bd369ef919fb (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230531T155928
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230531T155928

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