* [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.