All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tcp: possible race between tcp_done() and tcp_poll()
@ 2017-03-29  5:22 Seiichi Ikarashi
  2017-03-29 11:57 ` Sergei Shtylyov
  2017-03-30  0:35 ` Seiichi Ikarashi
  0 siblings, 2 replies; 8+ messages in thread
From: Seiichi Ikarashi @ 2017-03-29  5:22 UTC (permalink / raw)
  To: netdev

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 <s.ikarashi@jp.fujitsu.com>

 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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] tcp: possible race between tcp_done() and tcp_poll()
  2017-03-29  5:22 [PATCH] tcp: possible race between tcp_done() and tcp_poll() Seiichi Ikarashi
@ 2017-03-29 11:57 ` Sergei Shtylyov
  2017-03-29 23:19   ` Seiichi Ikarashi
  2017-03-30  0:35 ` Seiichi Ikarashi
  1 sibling, 1 reply; 8+ messages in thread
From: Sergei Shtylyov @ 2017-03-29 11:57 UTC (permalink / raw)
  To: Seiichi Ikarashi, netdev

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 <s.ikarashi@jp.fujitsu.com>
>

     Should be --- before the diffstat.

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

[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tcp: possible race between tcp_done() and tcp_poll()
  2017-03-29 11:57 ` Sergei Shtylyov
@ 2017-03-29 23:19   ` Seiichi Ikarashi
  0 siblings, 0 replies; 8+ messages in thread
From: Seiichi Ikarashi @ 2017-03-29 23:19 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: netdev

Thanks Sergei!

On 2017-03-29 20:57, Sergei Shtylyov wrote:
> 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 <s.ikarashi@jp.fujitsu.com>
>>
> 
>     Should be --- before the diffstat.

I'll resend.

Seiichi Ikarashi

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] tcp: possible race between tcp_done() and tcp_poll()
  2017-03-29  5:22 [PATCH] tcp: possible race between tcp_done() and tcp_poll() Seiichi Ikarashi
  2017-03-29 11:57 ` Sergei Shtylyov
@ 2017-03-30  0:35 ` Seiichi Ikarashi
  2017-03-30  2:31   ` Eric Dumazet
  1 sibling, 1 reply; 8+ messages in thread
From: Seiichi Ikarashi @ 2017-03-30  0:35 UTC (permalink / raw)
  To: netdev

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 <s.ikarashi@jp.fujitsu.com>

---
 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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] tcp: possible race between tcp_done() and tcp_poll()
  2017-03-30  0:35 ` Seiichi Ikarashi
@ 2017-03-30  2:31   ` Eric Dumazet
  2017-03-30  2:55     ` Seiichi Ikarashi
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2017-03-30  2:31 UTC (permalink / raw)
  To: Seiichi Ikarashi; +Cc: netdev

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.

...

>  So possibly, POLLIN|POLLRDNORM|POLLRDHUP might
> not be set even when receiving a RST packet.
> 
> Signed-off-by: Seiichi Ikarashi <s.ikarashi@jp.fujitsu.com>
> 
> ---
>  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 ?

>  	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 ?

Thanks !

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tcp: possible race between tcp_done() and tcp_poll()
  2017-03-30  2:31   ` Eric Dumazet
@ 2017-03-30  2:55     ` Seiichi Ikarashi
  2017-03-30  3:18       ` Eric Dumazet
  0 siblings, 1 reply; 8+ messages in thread
From: Seiichi Ikarashi @ 2017-03-30  2:55 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev

Hi Eric,

On 2017-03-30 11:31, Eric Dumazet wrote:
> 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.
> 
> ...
> 
>>  So possibly, POLLIN|POLLRDNORM|POLLRDHUP might
>> not be set even when receiving a RST packet.
>>
>> Signed-off-by: Seiichi Ikarashi <s.ikarashi@jp.fujitsu.com>
>>
>> ---
>>  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.

> 
>>  	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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] tcp: possible race between tcp_done() and tcp_poll()
  2017-03-30  2:55     ` Seiichi Ikarashi
@ 2017-03-30  3:18       ` Eric Dumazet
  2017-03-30  3:48         ` Seiichi Ikarashi
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2017-03-30  3:18 UTC (permalink / raw)
  To: Seiichi Ikarashi; +Cc: netdev

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);

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] tcp: possible race between tcp_done() and tcp_poll()
  2017-03-30  3:18       ` Eric Dumazet
@ 2017-03-30  3:48         ` Seiichi Ikarashi
  0 siblings, 0 replies; 8+ messages in thread
From: Seiichi Ikarashi @ 2017-03-30  3:48 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev

On 2017-03-30 12:18, Eric Dumazet wrote:
> 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

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.

> 
> 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);
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-03-30  3:49 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-29  5:22 [PATCH] tcp: possible race between tcp_done() and tcp_poll() Seiichi Ikarashi
2017-03-29 11:57 ` Sergei Shtylyov
2017-03-29 23:19   ` Seiichi Ikarashi
2017-03-30  0:35 ` Seiichi Ikarashi
2017-03-30  2:31   ` Eric Dumazet
2017-03-30  2:55     ` Seiichi Ikarashi
2017-03-30  3:18       ` Eric Dumazet
2017-03-30  3:48         ` Seiichi Ikarashi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.