[PATCH] tcp: possible race between tcp_done() and tcp_poll()

Similar to commit a4d258036ed9b2a1811.

Between receiving a packet and tcp_poll(), sk->sk_err is protected by memory barriers but
sk->sk_shutdown and sk->sk_state are not. So possibly, POLLIN|POLLRDNORM|POLLRDHUP might
not be set even when receiving a RST packet.

Signed-off-by: Seiichi Ikarashi <redacted>

 net/ipv4/tcp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index cf45555..c8bc86e 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -456,6 +456,8 @@ unsigned int tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
 
 	sock_poll_wait(file, sk_sleep(sk), wait);
 
+	/* This barrier is coupled with smp_wmb() in tcp_reset() */
+	smp_rmb();
 	state = sk_state_load(sk);
 	if (state == TCP_LISTEN)
 		return inet_csk_listen_poll(sk);

@@ -540,8 +542,6 @@ unsigned int tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
 		 */
 		mask |= POLLOUT | POLLWRNORM;
 	}
-	/* This barrier is coupled with smp_wmb() in tcp_reset() */
-	smp_rmb();
 	if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
 		mask |= POLLERR;
 

@@ -3291,6 +3291,9 @@ void tcp_done(struct sock *sk)
 
 	sk->sk_shutdown = SHUTDOWN_MASK;
 
+	/* This barrier is coupled with smp_rmb() in tcp_poll() */
+	smp_wmb();
+
 	if (!sock_flag(sk, SOCK_DEAD))
 		sk->sk_state_change(sk);
 	else

Hello!

On 3/29/2017 8:22 AM, Seiichi Ikarashi wrote:

Similar to commit a4d258036ed9b2a1811.

    Commit citing is standardized: it should specify 12-digit (at least) SHA1 
and the commit summary line enclosed in ("").

Between receiving a packet and tcp_poll(), sk->sk_err is protected by memory barriers but
sk->sk_shutdown and sk->sk_state are not. So possibly, POLLIN|POLLRDNORM|POLLRDHUP might
not be set even when receiving a RST packet.

Signed-off-by: Seiichi Ikarashi <redacted>

     Should be --- before the diffstat.

 net/ipv4/tcp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

[...]

MBR, Sergei

Thanks Sergei!

On 2017-03-29 20:57, Sergei Shtylyov wrote:

Hello!

On 3/29/2017 8:22 AM, Seiichi Ikarashi wrote:

quoted

Similar to commit a4d258036ed9b2a1811.

   Commit citing is standardized: it should specify 12-digit (at least) SHA1 and the commit summary line enclosed in ("").

quoted

Between receiving a packet and tcp_poll(), sk->sk_err is protected by memory barriers but
sk->sk_shutdown and sk->sk_state are not. So possibly, POLLIN|POLLRDNORM|POLLRDHUP might
not be set even when receiving a RST packet.

Signed-off-by: Seiichi Ikarashi <redacted>

    Should be --- before the diffstat.

I'll resend.

Seiichi Ikarashi

Similar to a4d258036ed9 ("tcp: Fix race in tcp_poll").

Between receiving a packet and tcp_poll(), sk->sk_err is protected by memory barriers but
sk->sk_shutdown and sk->sk_state are not. So possibly, POLLIN|POLLRDNORM|POLLRDHUP might
not be set even when receiving a RST packet.

Signed-off-by: Seiichi Ikarashi <redacted>

---
 net/ipv4/tcp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index cf45555..c8bc86e 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -456,6 +456,8 @@ unsigned int tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
 
 	sock_poll_wait(file, sk_sleep(sk), wait);
 
+	/* This barrier is coupled with smp_wmb() in tcp_reset() */
+	smp_rmb();
 	state = sk_state_load(sk);
 	if (state == TCP_LISTEN)
 		return inet_csk_listen_poll(sk);

@@ -540,8 +542,6 @@ unsigned int tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
 		 */
 		mask |= POLLOUT | POLLWRNORM;
 	}
-	/* This barrier is coupled with smp_wmb() in tcp_reset() */
-	smp_rmb();
 	if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
 		mask |= POLLERR;
 

@@ -3291,6 +3291,9 @@ void tcp_done(struct sock *sk)
 
 	sk->sk_shutdown = SHUTDOWN_MASK;
 
+	/* This barrier is coupled with smp_rmb() in tcp_poll() */
+	smp_wmb();
+
 	if (!sock_flag(sk, SOCK_DEAD))
 		sk->sk_state_change(sk);
 	else

On Thu, 2017-03-30 at 09:35 +0900, Seiichi Ikarashi wrote:

Similar to a4d258036ed9 ("tcp: Fix race in tcp_poll").

Between receiving a packet and tcp_poll(), sk->sk_err is protected by memory barriers but
sk->sk_shutdown and sk->sk_state are not.

...

Are you telling us that sk_state_load() has no barrier ?

This would imply that smp_load_acquire() should be replaced ?

Might I ask on which arch you got a problem ?

Thanks !

Hi Eric,

On 2017-03-30 11:31, Eric Dumazet wrote:

On Thu, 2017-03-30 at 09:35 +0900, Seiichi Ikarashi wrote:

quoted

Similar to a4d258036ed9 ("tcp: Fix race in tcp_poll").

Between receiving a packet and tcp_poll(), sk->sk_err is protected by memory barriers but
sk->sk_shutdown and sk->sk_state are not.

...

quoted

 So possibly, POLLIN|POLLRDNORM|POLLRDHUP might
not be set even when receiving a RST packet.

Signed-off-by: Seiichi Ikarashi <redacted>

---
 net/ipv4/tcp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index cf45555..c8bc86e 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -456,6 +456,8 @@ unsigned int tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
 
 	sock_poll_wait(file, sk_sleep(sk), wait);
 
+	/* This barrier is coupled with smp_wmb() in tcp_reset() */
+	smp_rmb();
 	state = sk_state_load(sk);

Are you telling us that sk_state_load() has no barrier ?

This would imply that smp_load_acquire() should be replaced ?

Ooops, of course you're right.
sk->sk_state _is_ protected by sk_state_{load,store}().

So my concern is only for sk->sk_shutdown.

quoted

 	if (state == TCP_LISTEN)
 		return inet_csk_listen_poll(sk);

@@ -540,8 +542,6 @@ unsigned int tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
 		 */
 		mask |= POLLOUT | POLLWRNORM;
 	}
-	/* This barrier is coupled with smp_wmb() in tcp_reset() */
-	smp_rmb();
 	if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
 		mask |= POLLERR;
 

@@ -3291,6 +3291,9 @@ void tcp_done(struct sock *sk)
 
 	sk->sk_shutdown = SHUTDOWN_MASK;
 
+	/* This barrier is coupled with smp_rmb() in tcp_poll() */
+	smp_wmb();
+
 	if (!sock_flag(sk, SOCK_DEAD))
 		sk->sk_state_change(sk);
 	else

Might I ask on which arch you got a problem ?

I got a report that receiving a RST packet but poll() got only POLLERR, no POLLIN|POLLRDHUP .
It was an old x86_64 kernel which does not include sk_state_{load,store} functions.
I suspected some race might have occur above.

Thanks,
Seiichi

On Thu, 2017-03-30 at 11:55 +0900, Seiichi Ikarashi wrote:

I got a report that receiving a RST packet but poll() got only POLLERR, no POLLIN|POLLRDHUP .
It was an old x86_64 kernel which does not include sk_state_{load,store} functions.
I suspected some race might have occur above.

It looks that a one-liner patch would be enough in tcp_done()

No need adding extra barriers

Untested patch :

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1e319a525d51b0b603a5ccc5143381c752b9f2c7..4107d3fc00a53f768cdb885971a1067c810f5c9f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -3285,12 +3285,12 @@ void tcp_done(struct sock *sk)
 	if (sk->sk_state == TCP_SYN_SENT || sk->sk_state == TCP_SYN_RECV)
 		TCP_INC_STATS(sock_net(sk), TCP_MIB_ATTEMPTFAILS);
 
+	sk->sk_shutdown = SHUTDOWN_MASK;
 	tcp_set_state(sk, TCP_CLOSE);
 	tcp_clear_xmit_timers(sk);
 	if (req)
 		reqsk_fastopen_remove(sk, req, false);
 
-	sk->sk_shutdown = SHUTDOWN_MASK;
 
 	if (!sock_flag(sk, SOCK_DEAD))
 		sk->sk_state_change(sk);

On 2017-03-30 12:18, Eric Dumazet wrote:

On Thu, 2017-03-30 at 11:55 +0900, Seiichi Ikarashi wrote:

quoted

I got a report that receiving a RST packet but poll() got only POLLERR, no POLLIN|POLLRDHUP .
It was an old x86_64 kernel which does not include sk_state_{load,store} functions.
I suspected some race might have occur above.

It looks that a one-liner patch would be enough in tcp_done()

No need adding extra barriers

I agree.
sk->sk_shutdown seems not affect the behavior of both tcp_clear_xmit_timers()
and reqsk_fastopen_remove().

Untested patch :

Sorry, I cannot test it because I do not have a replication environment.