netdev - Re: [PATCH v2] tcp: fix connection reset due to tw hashdance race.

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: <AF8804B1-D096-4B80-9A1F-37FA03B04123@baidu.com>
Date: Tue, 20 Jun 2023 03:30:29 +0000
From: "Duan,Muquan" <duanmuquan@...du.com>
To: Eric Dumazet <edumazet@...gle.com>
CC: "davem@...emloft.net" <davem@...emloft.net>, "dsahern@...nel.org"
	<dsahern@...nel.org>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] tcp: fix connection reset due to tw hashdance race.

Hi, Eric,

Thanks for your comments!

Why not speak of the FIN: 
For current implementation, hashdance can be done on state FIN_WAIT2,  it may race with the ehash lookup process of  passive closer’s FIN. My new patch 3 does the tw hashdance until receiving passive closer's FIN(real TIME_WAIT),  so this race does not exist and  the 'connection refused' issue will not occur, so I did not speak of the FIN again with the new patch.  

Why speak of the SYN:
Theoretically new SYN may race with hashdance with or without my patch, and results in a retransmission of it. But It is almost impossible to get new SYN before hashdance, there are one RTT between the passive closer's FIN and new SYN. The probability is too small to and may never occur in practice. I never me0t this issue in my reproducing environment.
 For the connection reuse issue I met, I think  no tricks needed with patch 3.

About introducing the lock:
If we really need to introduce the lock, I think besides protecting the list in ehash bucket, we also need to protect the sock during consuming the patcket. Because after we find the sock and consuming  the packet, we can meet sock level race at different CPUs, for example,  the passive closer's FIN arrives too fast and finds the original sock before the hashdance begins, the FIN may be dropped in further process if the sock is destroyed on another CPU after hashdance.  

I took a look at FreeBSD, it uses hash table lock and per sock level lock.It also needs some tricks to retry for some cases, for example, sock dropped by another thread when waiting for per sock lock during the lookup: 
   /*  
     * While waiting for inp lock during the lookup, another thread
     * can have dropped the inpcb, in which case we need to loop back 
     * and try to find a new inpcb to deliver to. 
     */   
    if (inp->inp_flags & INP_DROPPED) {
        INP_WUNLOCK(inp);
        inp = NULL;
        goto findpcb;
}    

I worry about that locking on ehash bucket and per sock will introduce performance and scaling issue , especially when    there are a large number of socks or many short connections. I can take some time out to deal with this issue in the flowing a few weeks, I will try to introduce lock and write some test programs to evaluate the performance hit.

Regards!
Duanmuquan