[PATCH mptcp-net] mptcp: do not rely on implicit state check in mptcp_listen()

Paolo Abeni posted 1 patch 10 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/9a42270be1b4d109351b22ad0331bab8051803fb.1687951613.git.pabeni@redhat.com
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, Mat Martineau <martineau@kernel.org>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
There is a newer version of this series
net/mptcp/protocol.c | 5 +++++
1 file changed, 5 insertions(+)
[PATCH mptcp-net] mptcp: do not rely on implicit state check in mptcp_listen()
Posted by Paolo Abeni 10 months, 1 week ago
Since the blamed commit, closing the first subflow resets the first
subflow socket state to SS_UNCONNECTED.

The current mptcp listen implementation relies only on such
state to prevent touching not-fully-disconnected sockets.

Incoming mptcp fastclose (or paired endpoint removal) unconditionally
closes the first subflow.

All the above allows an incoming fastclose followed by a listen() call
to successfully race with a blocking recvmsg(), potentially causing the
latter to hit a divide by zero bug in cleanup_rbuf/__tcp_select_window().

Address the issue explicitly checking the msk socket state in
mptcp_listen(). An alternative solution would be moving the first
subflow socket state update into mptcp_disconnect(), but in the long
term the first subflow socket should be removed: better avoid relaying
on it for internal consistency check.

Fixes: b29fcfb54cd7 ("mptcp: full disconnect implementation")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
This is basically a new version of "mptcp: fix bogus socket state update",
but since it uses a quite different logic, no revision increase ;)

Should close issues/414
---
 net/mptcp/protocol.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index a50eaa01ba8a..d3ff2a2c60de 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3751,6 +3751,11 @@ static int mptcp_listen(struct socket *sock, int backlog)
 	pr_debug("msk=%p", msk);
 
 	lock_sock(sk);
+
+	err = -EINVAL;
+	if (sock->state != SS_UNCONNECTED || sock->type != SOCK_STREAM)
+		goto unlock;
+
 	ssock = __mptcp_nmpc_socket(msk);
 	if (IS_ERR(ssock)) {
 		err = PTR_ERR(ssock);
-- 
2.40.1
Re: [PATCH mptcp-net] mptcp: do not rely on implicit state check in mptcp_listen()
Posted by Matthieu Baerts 10 months ago
Hi Paolo, Christoph,

On 28/06/2023 13:27, Paolo Abeni wrote:
> Since the blamed commit, closing the first subflow resets the first
> subflow socket state to SS_UNCONNECTED.
> 
> The current mptcp listen implementation relies only on such
> state to prevent touching not-fully-disconnected sockets.
> 
> Incoming mptcp fastclose (or paired endpoint removal) unconditionally
> closes the first subflow.
> 
> All the above allows an incoming fastclose followed by a listen() call
> to successfully race with a blocking recvmsg(), potentially causing the
> latter to hit a divide by zero bug in cleanup_rbuf/__tcp_select_window().
> 
> Address the issue explicitly checking the msk socket state in
> mptcp_listen(). An alternative solution would be moving the first
> subflow socket state update into mptcp_disconnect(), but in the long
> term the first subflow socket should be removed: better avoid relaying
> on it for internal consistency check.
> 
> Fixes: b29fcfb54cd7 ("mptcp: full disconnect implementation")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> This is basically a new version of "mptcp: fix bogus socket state update",
> but since it uses a quite different logic, no revision increase ;)
> 
> Should close issues/414

Thank you for the patch! It looks good to me!

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

I guess we can also add:

Reported-by: Christoph Paasch <cpaasch@apple.com>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/414

Just applied in our tree (fixes for -net):

New patches for t/upstream-net and t/upstream:
- 40575a81e44f: mptcp: do not rely on implicit state check in mptcp_listen()
- Results: 1dacd2512f3b..40787b521a4a (export-net)
- Results: ba5bf7787049..d99b5b5a67e4 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230629T150635
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230629T150635

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