* [PATCH net-next] mptcp: be careful on MPTCP-level ack.
@ 2020-11-24 11:20 Paolo Abeni
2020-11-24 18:08 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Abeni @ 2020-11-24 11:20 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, mptcp, Eric Dumazet
We can enter the main mptcp_recvmsg() loop even when
no subflows are connected. As note by Eric, that would
result in a divide by zero oops on ack generation.
Address the issue by 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>
---
net/mptcp/protocol.c | 67 ++++++++++++++++++++++++++++++++------------
1 file changed, 49 insertions(+), 18 deletions(-)
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()
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] mptcp: be careful on MPTCP-level ack.
2020-11-24 11:20 [PATCH net-next] mptcp: be careful on MPTCP-level ack Paolo Abeni
@ 2020-11-24 18:08 ` Jakub Kicinski
2020-11-24 18:53 ` Paolo Abeni
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kicinski @ 2020-11-24 18:08 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, mptcp, Eric Dumazet
On Tue, 24 Nov 2020 12:20:11 +0100 Paolo Abeni wrote:
> -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));
> +}
Does the compiler really not inline this trivial static function?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] mptcp: be careful on MPTCP-level ack.
2020-11-24 18:08 ` Jakub Kicinski
@ 2020-11-24 18:53 ` Paolo Abeni
0 siblings, 0 replies; 3+ messages in thread
From: Paolo Abeni @ 2020-11-24 18:53 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, mptcp, Eric Dumazet
On Tue, 2020-11-24 at 10:08 -0800, Jakub Kicinski wrote:
> On Tue, 24 Nov 2020 12:20:11 +0100 Paolo Abeni wrote:
> > -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));
> > +}
>
> Does the compiler really not inline this trivial static function?
whoops... That is just me adding an unneeded keyword. I'll send a v2.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-24 18:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-24 11:20 [PATCH net-next] mptcp: be careful on MPTCP-level ack Paolo Abeni
2020-11-24 18:08 ` Jakub Kicinski
2020-11-24 18:53 ` Paolo Abeni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).