netdev - Re: [PATCH] tcp: possible race between tcp_done() and tcp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3c8a7438-9ce4-593a-d492-ec8117d71af4@jp.fujitsu.com>
Date:   Thu, 30 Mar 2017 11:55:43 +0900
From:   Seiichi Ikarashi <s.ikarashi@...fujitsu.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
CC:     <netdev@...r.kernel.org>
Subject: Re: [PATCH] tcp: possible race between tcp_done() and tcp_poll()

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