Messages in this thread | | | From | Eric Dumazet <> | Date | Thu, 19 Nov 2020 20:45:29 +0100 | Subject | Re: [PATCH v7] tcp: fix race condition when creating child sockets from syncookies |
| |
On Thu, Nov 19, 2020 at 8:24 PM Ricardo Dias <rdias@singlestore.com> wrote: > > When the TCP stack is in SYN flood mode, the server child socket is > created from the SYN cookie received in a TCP packet with the ACK flag > set. > > The child socket is created when the server receives the first TCP > packet with a valid SYN cookie from the client. Usually, this packet > corresponds to the final step of the TCP 3-way handshake, the ACK > packet. But is also possible to receive a valid SYN cookie from the > first TCP data packet sent by the client, and thus create a child socket > from that SYN cookie. > > Since a client socket is ready to send data as soon as it receives the > SYN+ACK packet from the server, the client can send the ACK packet (sent > by the TCP stack code), and the first data packet (sent by the userspace > program) almost at the same time, and thus the server will equally > receive the two TCP packets with valid SYN cookies almost at the same > instant. > > When such event happens, the TCP stack code has a race condition that > occurs between the momement a lookup is done to the established > connections hashtable to check for the existence of a connection for the > same client, and the moment that the child socket is added to the > established connections hashtable. As a consequence, this race condition > can lead to a situation where we add two child sockets to the > established connections hashtable and deliver two sockets to the > userspace program to the same client. > > This patch fixes the race condition by checking if an existing child > socket exists for the same client when we are adding the second child > socket to the established connections socket. If an existing child > socket exists, we return that socket and use it to process the TCP > packet received, and discard the second child socket to the same client. > > Signed-off-by: Ricardo Dias <rdias@singlestore.com> > --- > v7 (2020-11-19): > * Changed the approach to re-use the first (existing) socket created > from thge syncookie. Instead of returning the existing socket in > tcp_(v4|v6)_syn_recv_sock and continue the protocol state machine > execution, tcp_(v4|v6)_syn_recv_sock signals that already exists a > socket, and tells tcp_(v4|v6)_rcv to lookup the socket again in the > established connections table. > This new approach fixes the errors reported by Eric for the previous > version of the patch. > * Also fixes the memory leaks by making sure that the newly created > socket in syn_recv_sock is destroyed in case an already existing > socket exists.
I think this is going too far. Your patch is too complex/risky, and will be hard to backport to old kernels, because TCP stack has changed a lot.
Alternative approach would be to detect the race and simply drop the packet that lost the battle.
| |