[PATCH v2 net-next 0/4] tcp: fix tcp_poll() races

[PATCH v2 net-next 0/4] tcp: fix tcp_poll() races

[Eric Dumazet]

Flakes in packetdrill tests stressing epoll_wait()
were root caused to bad ordering in tcp_write_err()

Precisely, we have to call sk_error_report() after
tcp_done().

When fixing this issue, we discovered tcp_abort(),
tcp_v4_err() and tcp_v6_err() had similar issues.

Since tcp_reset() has the correct ordering,
first patch takes part of it and creates
tcp_done_with_error() helper.

v2: added @err parameter to tcp_done_with_error()
    as suggested by David Laight.

Eric Dumazet (4):
  tcp: add tcp_done_with_error() helper
  tcp: fix race in tcp_write_err()
  tcp: fix races in tcp_abort()
  tcp: fix races in tcp_v[46]_err()

 include/net/tcp.h    |  1 +
 net/ipv4/tcp.c       |  8 ++------
 net/ipv4/tcp_input.c | 32 +++++++++++++++++++++-----------
 net/ipv4/tcp_ipv4.c  | 11 +++--------
 net/ipv4/tcp_timer.c |  6 +-----
 net/ipv6/tcp_ipv6.c  | 10 +++-------
 6 files changed, 31 insertions(+), 37 deletions(-)

-- 
2.45.1.288.g0e0cd299f1-goog

[PATCH v2 net-next 1/4] tcp: add tcp_done_with_error() helper

[Eric Dumazet]

tcp_reset() ends with a sequence that is carefuly ordered.

We need to fix [e]poll bugs in the following patches,
it makes sense to use a common helper.

Suggested-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/tcp.h    |  1 +
 net/ipv4/tcp.c       |  2 +-
 net/ipv4/tcp_input.c | 32 +++++++++++++++++++++-----------
 3 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 060e95b331a286ad7c355be11dc03250d2944920..32815a40dea16637d2cc49d46863b532a44fbab3 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -677,6 +677,7 @@ void tcp_skb_collapse_tstamp(struct sk_buff *skb,
 /* tcp_input.c */
 void tcp_rearm_rto(struct sock *sk);
 void tcp_synack_rtt_meas(struct sock *sk, struct request_sock *req);
+void tcp_done_with_error(struct sock *sk, int err);
 void tcp_reset(struct sock *sk, struct sk_buff *skb);
 void tcp_fin(struct sock *sk);
 void tcp_check_space(struct sock *sk);
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 681b54e1f3a64387787738ab6495531b8abe1771..2a8f8d8676ff1d30ea9f8cd47ccf9236940eb299 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -598,7 +598,7 @@ __poll_t tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
 		 */
 		mask |= EPOLLOUT | EPOLLWRNORM;
 	}
-	/* This barrier is coupled with smp_wmb() in tcp_reset() */
+	/* This barrier is coupled with smp_wmb() in tcp_done_with_error() */
 	smp_rmb();
 	if (READ_ONCE(sk->sk_err) ||
 	    !skb_queue_empty_lockless(&sk->sk_error_queue))
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 9c04a9c8be9dfaa0ec2437b3748284e57588b216..5aadf64e554d8009b2739613c279bbf82a05bbdd 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4436,9 +4436,26 @@ static enum skb_drop_reason tcp_sequence(const struct tcp_sock *tp,
 	return SKB_NOT_DROPPED_YET;
 }
 
+
+void tcp_done_with_error(struct sock *sk, int err)
+{
+	/* This barrier is coupled with smp_rmb() in tcp_poll() */
+	WRITE_ONCE(sk->sk_err, err);
+	smp_wmb();
+
+	tcp_write_queue_purge(sk);
+	tcp_done(sk);
+
+	if (!sock_flag(sk, SOCK_DEAD))
+		sk_error_report(sk);
+}
+EXPORT_SYMBOL(tcp_done_with_error);
+
 /* When we get a reset we do this. */
 void tcp_reset(struct sock *sk, struct sk_buff *skb)
 {
+	int err;
+
 	trace_tcp_receive_reset(sk);
 
 	/* mptcp can't tell us to ignore reset pkts,
@@ -4450,24 +4467,17 @@ void tcp_reset(struct sock *sk, struct sk_buff *skb)
 	/* We want the right error as BSD sees it (and indeed as we do). */
 	switch (sk->sk_state) {
 	case TCP_SYN_SENT:
-		WRITE_ONCE(sk->sk_err, ECONNREFUSED);
+		err = ECONNREFUSED;
 		break;
 	case TCP_CLOSE_WAIT:
-		WRITE_ONCE(sk->sk_err, EPIPE);
+		err = EPIPE;
 		break;
 	case TCP_CLOSE:
 		return;
 	default:
-		WRITE_ONCE(sk->sk_err, ECONNRESET);
+		err = ECONNRESET;
 	}
-	/* This barrier is coupled with smp_rmb() in tcp_poll() */
-	smp_wmb();
-
-	tcp_write_queue_purge(sk);
-	tcp_done(sk);
-
-	if (!sock_flag(sk, SOCK_DEAD))
-		sk_error_report(sk);
+	tcp_done_with_error(sk, err);
 }
 
 /*
-- 
2.45.1.288.g0e0cd299f1-goog

[PATCH v2 net-next 2/4] tcp: fix race in tcp_write_err()

[Eric Dumazet]

I noticed flakes in a packetdrill test, expecting an epoll_wait()
to return EPOLLERR | EPOLLHUP on a failed connect() attempt,
after multiple SYN retransmits. It sometimes return EPOLLERR only.

The issue is that tcp_write_err():
 1) writes an error in sk->sk_err,
 2) calls sk_error_report(),
 3) then calls tcp_done().

tcp_done() is writing SHUTDOWN_MASK into sk->sk_shutdown,
among other things.

Problem is that the awaken user thread (from 2) sk_error_report())
might call tcp_poll() before tcp_done() has written sk->sk_shutdown.

tcp_poll() only sees a non zero sk->sk_err and returns EPOLLERR.

This patch fixes the issue by making sure to call sk_error_report()
after tcp_done().

tcp_write_err() also lacks an smp_wmb().

We can reuse tcp_done_with_error() to factor out the details,
as Neal suggested.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_timer.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 83fe7f62f7f10ab111512a3ef15a97a04c79cb4a..3e8604ae7d06c5b010a2034e3295675a7d358f13 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -74,11 +74,7 @@ u32 tcp_clamp_probe0_to_user_timeout(const struct sock *sk, u32 when)
 
 static void tcp_write_err(struct sock *sk)
 {
-	WRITE_ONCE(sk->sk_err, READ_ONCE(sk->sk_err_soft) ? : ETIMEDOUT);
-	sk_error_report(sk);
-
-	tcp_write_queue_purge(sk);
-	tcp_done(sk);
+	tcp_done_with_error(sk, READ_ONCE(sk->sk_err_soft) ? : ETIMEDOUT);
 	__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONTIMEOUT);
 }
 
-- 
2.45.1.288.g0e0cd299f1-goog

[PATCH v2 net-next 3/4] tcp: fix races in tcp_abort()

[Eric Dumazet]

tcp_abort() has the same issue than the one fixed in the prior patch
in tcp_write_err().

In order to get consistent results from tcp_poll(), we must call
sk_error_report() after tcp_done().

We can use tcp_done_with_error() to centralize this logic.

Fixes: c1e64e298b8c ("net: diag: Support destroying TCP sockets.")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 2a8f8d8676ff1d30ea9f8cd47ccf9236940eb299..5fa68e7f6ddbfd325523365cb41de07d5b938e47 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4576,14 +4576,10 @@ int tcp_abort(struct sock *sk, int err)
 	bh_lock_sock(sk);
 
 	if (!sock_flag(sk, SOCK_DEAD)) {
-		WRITE_ONCE(sk->sk_err, err);
-		/* This barrier is coupled with smp_rmb() in tcp_poll() */
-		smp_wmb();
-		sk_error_report(sk);
 		if (tcp_need_reset(sk->sk_state))
 			tcp_send_active_reset(sk, GFP_ATOMIC,
 					      SK_RST_REASON_NOT_SPECIFIED);
-		tcp_done(sk);
+		tcp_done_with_error(sk, err);
 	}
 
 	bh_unlock_sock(sk);
-- 
2.45.1.288.g0e0cd299f1-goog

[PATCH v2 net-next 4/4] tcp: fix races in tcp_v[46]_err()

[Eric Dumazet]

These functions have races when they:

1) Write sk->sk_err
2) call sk_error_report(sk)
3) call tcp_done(sk)

As described in prior patches in this series:

An smp_wmb() is missing.
We should call tcp_done() before sk_error_report(sk)
to have consistent tcp_poll() results on SMP hosts.

Use tcp_done_with_error() where we centralized the
correct sequence.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_ipv4.c | 11 +++--------
 net/ipv6/tcp_ipv6.c | 10 +++-------
 2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 30ef0c8f5e92d301c31ea1a05f662c1fc4cf37af..158fad0cbe43d2029461f6cef3ee698c2acce528 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -611,15 +611,10 @@ int tcp_v4_err(struct sk_buff *skb, u32 info)
 
 		ip_icmp_error(sk, skb, err, th->dest, info, (u8 *)th);
 
-		if (!sock_owned_by_user(sk)) {
-			WRITE_ONCE(sk->sk_err, err);
-
-			sk_error_report(sk);
-
-			tcp_done(sk);
-		} else {
+		if (!sock_owned_by_user(sk))
+			tcp_done_with_error(sk, err);
+		else
 			WRITE_ONCE(sk->sk_err_soft, err);
-		}
 		goto out;
 	}
 
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 4c3605485b68e7c333a0144df3d685b3db9ff45d..10c804b3638a99a9ae4cf3d81bd4481ac06703f9 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -490,14 +490,10 @@ static int tcp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 
 		ipv6_icmp_error(sk, skb, err, th->dest, ntohl(info), (u8 *)th);
 
-		if (!sock_owned_by_user(sk)) {
-			WRITE_ONCE(sk->sk_err, err);
-			sk_error_report(sk);		/* Wake people up to see the error (see connect in sock.c) */
-
-			tcp_done(sk);
-		} else {
+		if (!sock_owned_by_user(sk))
+			tcp_done_with_error(sk, err);
+		else
 			WRITE_ONCE(sk->sk_err_soft, err);
-		}
 		goto out;
 	case TCP_LISTEN:
 		break;
-- 
2.45.1.288.g0e0cd299f1-goog

Re: [PATCH v2 net-next 1/4] tcp: add tcp_done_with_error() helper

[Neal Cardwell]

On Tue, May 28, 2024 at 8:53 AM Eric Dumazet <edumazet@google.com> wrote:
>
> tcp_reset() ends with a sequence that is carefuly ordered.
>
> We need to fix [e]poll bugs in the following patches,
> it makes sense to use a common helper.
>
> Suggested-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks, Eric!

neal

Re: [PATCH v2 net-next 2/4] tcp: fix race in tcp_write_err()

[Neal Cardwell]

On Tue, May 28, 2024 at 8:53 AM Eric Dumazet <edumazet@google.com> wrote:
>
> I noticed flakes in a packetdrill test, expecting an epoll_wait()
> to return EPOLLERR | EPOLLHUP on a failed connect() attempt,
> after multiple SYN retransmits. It sometimes return EPOLLERR only.
>
> The issue is that tcp_write_err():
>  1) writes an error in sk->sk_err,
>  2) calls sk_error_report(),
>  3) then calls tcp_done().
>
> tcp_done() is writing SHUTDOWN_MASK into sk->sk_shutdown,
> among other things.
>
> Problem is that the awaken user thread (from 2) sk_error_report())
> might call tcp_poll() before tcp_done() has written sk->sk_shutdown.
>
> tcp_poll() only sees a non zero sk->sk_err and returns EPOLLERR.
>
> This patch fixes the issue by making sure to call sk_error_report()
> after tcp_done().
>
> tcp_write_err() also lacks an smp_wmb().
>
> We can reuse tcp_done_with_error() to factor out the details,
> as Neal suggested.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks, Eric!

neal

Re: [PATCH v2 net-next 3/4] tcp: fix races in tcp_abort()

[Neal Cardwell]

On Tue, May 28, 2024 at 8:53 AM Eric Dumazet <edumazet@google.com> wrote:
>
> tcp_abort() has the same issue than the one fixed in the prior patch
> in tcp_write_err().
>
> In order to get consistent results from tcp_poll(), we must call
> sk_error_report() after tcp_done().
>
> We can use tcp_done_with_error() to centralize this logic.
>
> Fixes: c1e64e298b8c ("net: diag: Support destroying TCP sockets.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks, Eric!

neal

Re: [PATCH v2 net-next 4/4] tcp: fix races in tcp_v[46]_err()

[Neal Cardwell]

On Tue, May 28, 2024 at 8:53 AM Eric Dumazet <edumazet@google.com> wrote:
>
> These functions have races when they:
>
> 1) Write sk->sk_err
> 2) call sk_error_report(sk)
> 3) call tcp_done(sk)
>
> As described in prior patches in this series:
>
> An smp_wmb() is missing.
> We should call tcp_done() before sk_error_report(sk)
> to have consistent tcp_poll() results on SMP hosts.
>
> Use tcp_done_with_error() where we centralized the
> correct sequence.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks, Eric!

neal

Re: [PATCH v2 net-next 0/4] tcp: fix tcp_poll() races

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 28 May 2024 12:52:49 +0000 you wrote:
> Flakes in packetdrill tests stressing epoll_wait()
> were root caused to bad ordering in tcp_write_err()
> 
> Precisely, we have to call sk_error_report() after
> tcp_done().
> 
> When fixing this issue, we discovered tcp_abort(),
> tcp_v4_err() and tcp_v6_err() had similar issues.
> 
> [...]

Here is the summary with links:
  - [v2,net-next,1/4] tcp: add tcp_done_with_error() helper
    https://git.kernel.org/netdev/net-next/c/5e514f1cba09
  - [v2,net-next,2/4] tcp: fix race in tcp_write_err()
    https://git.kernel.org/netdev/net-next/c/853c3bd7b791
  - [v2,net-next,3/4] tcp: fix races in tcp_abort()
    https://git.kernel.org/netdev/net-next/c/5ce4645c23cf
  - [v2,net-next,4/4] tcp: fix races in tcp_v[46]_err()
    https://git.kernel.org/netdev/net-next/c/fde6f897f2a1

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html