diff mbox series

[net] mptcp: replace mptcp_disconnect with a stub

Message ID 20200429184320.30733-1-fw@strlen.de
State Accepted
Delegated to: David Miller
Headers show
Series [net] mptcp: replace mptcp_disconnect with a stub | expand

Commit Message

Florian Westphal April 29, 2020, 6:43 p.m. UTC 
Paolo points out that mptcp_disconnect is bogus:
"lock_sock(sk);
looks suspicious (lock should be already held by the caller)
And call to: tcp_disconnect(sk, flags); too, sk is not a tcp
socket".

->disconnect() gets called from e.g. inet_stream_connect when
one tries to disassociate a connected socket again (to re-connect
without closing the socket first).
MPTCP however uses mptcp_stream_connect, not inet_stream_connect,
for the mptcp-socket connect call.

inet_stream_connect only gets called indirectly, for the tcp socket,
so any ->disconnect() calls end up calling tcp_disconnect for that
tcp subflow sk.

This also explains why syzkaller has not yet reported a problem
here.  So for now replace this with a stub that doesn't do anything.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/14
Acked-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/mptcp/protocol.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

David Miller April 29, 2020, 7:40 p.m. UTC | #1 
From: Florian Westphal <fw@strlen.de>
Date: Wed, 29 Apr 2020 20:43:20 +0200

> Paolo points out that mptcp_disconnect is bogus:
> "lock_sock(sk);
> looks suspicious (lock should be already held by the caller)
> And call to: tcp_disconnect(sk, flags); too, sk is not a tcp
> socket".
> 
> ->disconnect() gets called from e.g. inet_stream_connect when
> one tries to disassociate a connected socket again (to re-connect
> without closing the socket first).
> MPTCP however uses mptcp_stream_connect, not inet_stream_connect,
> for the mptcp-socket connect call.
> 
> inet_stream_connect only gets called indirectly, for the tcp socket,
> so any ->disconnect() calls end up calling tcp_disconnect for that
> tcp subflow sk.
> 
> This also explains why syzkaller has not yet reported a problem
> here.  So for now replace this with a stub that doesn't do anything.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/14
> Acked-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Applied.
diff mbox series

Patch

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b22a63ba2348..6e0188f5d3f3 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1316,11 +1316,12 @@  static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
 
 static int mptcp_disconnect(struct sock *sk, int flags)
 {
-	lock_sock(sk);
-	__mptcp_clear_xmit(sk);
-	release_sock(sk);
-	mptcp_cancel_work(sk);
-	return tcp_disconnect(sk, flags);
+	/* Should never be called.
+	 * inet_stream_connect() calls ->disconnect, but that
+	 * refers to the subflow socket, not the mptcp one.
+	 */
+	WARN_ON_ONCE(1);
+	return 0;
 }
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)