| Message ID | 75d1b617-b143-3061-fcc7-4cca26ffc9fe@jp.fujitsu.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 !
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
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
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(-)