diff mbox series

[net-next,v2] mptcp: be careful on MPTCP-level ack.

Message ID df78ed9c38edfbed50b9d275993e716e4cd14ee7.1606213300.git.pabeni@redhat.com
State Accepted, archived
Commit e7a4633911271d1abbb9a665beff2a9bcad4c9a2
Delegated to: Matthieu Baerts
Headers show
Series [net-next,v2] mptcp: be careful on MPTCP-level ack. | expand

Commit Message

Paolo Abeni Nov. 24, 2020, 10:23 a.m. UTC
We can enter the main mptcp_recvmsg() loop even when
no sublflows is connected. As note by Eric that would
result in a divide by zero oops on ack generation.

Address the issue checking the subflow status before
sending the ack.

Additionally protect mptcp_recvmsg() against invocation
with weird socket states.

Reported-and-suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - better naming
 - fixed a few typos (introducing bad crashes)
---
 net/mptcp/protocol.c | 67 ++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 18 deletions(-)

Comments

Matthieu Baerts Nov. 24, 2020, 2:25 p.m. UTC | #1
Hi Paolo,

On 24/11/2020 11:23, Paolo Abeni wrote:
> We can enter the main mptcp_recvmsg() loop even when
> no sublflows is connected. As note by Eric that would
> result in a divide by zero oops on ack generation.
> 
> Address the issue checking the subflow status before
> sending the ack.
> 
> Additionally protect mptcp_recvmsg() against invocation
> with weird socket states.

Thank you for the patch!

Just added at the top of the tree:

- e7a463391127: mptcp: be careful on MPTCP-level ack
- 85bdaa84e054: conflict in 
t/mptcp-protect-the-rx-path-with-the-msk-socket-spinlock
- af40879251c9: mptcp: adapt new code to topic

- Results: 7e538fe431fc..28691a4d4d0f

May you double check the conflicts resolution please? :)

Tests + export are going to be started soon.

Cheers,
Matt
Paolo Abeni Nov. 24, 2020, 3:03 p.m. UTC | #2
On Tue, 2020-11-24 at 15:25 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 24/11/2020 11:23, Paolo Abeni wrote:
> > We can enter the main mptcp_recvmsg() loop even when
> > no sublflows is connected. As note by Eric that would
> > result in a divide by zero oops on ack generation.
> > 
> > Address the issue checking the subflow status before
> > sending the ack.
> > 
> > Additionally protect mptcp_recvmsg() against invocation
> > with weird socket states.
> 
> Thank you for the patch!
> 
> Just added at the top of the tree:
> 
> - e7a463391127: mptcp: be careful on MPTCP-level ack
> - 85bdaa84e054: conflict in 
> t/mptcp-protect-the-rx-path-with-the-msk-socket-spinlock
> - af40879251c9: mptcp: adapt new code to topic
> 
> - Results: 7e538fe431fc..28691a4d4d0f
> 
> May you double check the conflicts resolution please? :)

LGTM,

Thanks!

/P
diff mbox series

Patch

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 4b7794835fea..b50164efb741 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -419,31 +419,57 @@  static bool mptcp_subflow_active(struct mptcp_subflow_context *subflow)
 	return ((1 << ssk->sk_state) & (TCPF_ESTABLISHED | TCPF_CLOSE_WAIT));
 }
 
-static void mptcp_send_ack(struct mptcp_sock *msk, bool force)
+static inline bool tcp_can_send_ack(const struct sock *ssk)
+{
+	return !((1 << inet_sk_state_load(ssk)) &
+	       (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE));
+}
+
+static void mptcp_send_ack(struct mptcp_sock *msk)
 {
 	struct mptcp_subflow_context *subflow;
-	struct sock *pick = NULL;
 
 	mptcp_for_each_subflow(msk, subflow) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 
-		if (force) {
-			lock_sock(ssk);
+		lock_sock(ssk);
+		if (tcp_can_send_ack(ssk))
 			tcp_send_ack(ssk);
-			release_sock(ssk);
-			continue;
-		}
-
-		/* if the hintes ssk is still active, use it */
-		pick = ssk;
-		if (ssk == msk->ack_hint)
-			break;
+		release_sock(ssk);
 	}
-	if (!force && pick) {
-		lock_sock(pick);
-		tcp_cleanup_rbuf(pick, 1);
-		release_sock(pick);
+}
+
+static bool mptcp_subflow_cleanup_rbuf(struct sock *ssk)
+{
+	int ret;
+
+	lock_sock(ssk);
+	ret = tcp_can_send_ack(ssk);
+	if (ret)
+		tcp_cleanup_rbuf(ssk, 1);
+	release_sock(ssk);
+	return ret;
+}
+
+static void mptcp_cleanup_rbuf(struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow;
+
+	/* if the hinted ssk is still active, try to use it */
+	if (likely(msk->ack_hint)) {
+		mptcp_for_each_subflow(msk, subflow) {
+			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+			if (msk->ack_hint == ssk &&
+			    mptcp_subflow_cleanup_rbuf(ssk))
+				return;
+		}
 	}
+
+	/* otherwise pick the first active subflow */
+	mptcp_for_each_subflow(msk, subflow)
+		if (mptcp_subflow_cleanup_rbuf(mptcp_subflow_tcp_sock(subflow)))
+			return;
 }
 
 static bool mptcp_check_data_fin(struct sock *sk)
@@ -494,7 +520,7 @@  static bool mptcp_check_data_fin(struct sock *sk)
 
 		ret = true;
 		mptcp_set_timeout(sk, NULL);
-		mptcp_send_ack(msk, true);
+		mptcp_send_ack(msk);
 		mptcp_close_wake_up(sk);
 	}
 	return ret;
@@ -1579,6 +1605,11 @@  static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 		return -EOPNOTSUPP;
 
 	lock_sock(sk);
+	if (unlikely(sk->sk_state == TCP_LISTEN)) {
+		copied = -ENOTCONN;
+		goto out_err;
+	}
+
 	timeo = sock_rcvtimeo(sk, nonblock);
 
 	len = min_t(size_t, len, INT_MAX);
@@ -1604,7 +1635,7 @@  static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 		/* be sure to advertise window change */
 		old_space = READ_ONCE(msk->old_wspace);
 		if ((tcp_space(sk) - old_space) >= old_space)
-			mptcp_send_ack(msk, false);
+			mptcp_cleanup_rbuf(msk);
 
 		/* only the master socket status is relevant here. The exit
 		 * conditions mirror closely tcp_recvmsg()