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.ikara...@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
