tempesta-tech / tempesta

All-in-one solution for high performance web content delivery and advanced protection against DDoS and web attacks
https://tempesta-tech.com/
GNU General Public License v2.0
618 stars 103 forks source link

Extra calls of `tfw_classify_conn_estab` #2096

Closed EvgeniiMekhanik closed 6 months ago

EvgeniiMekhanik commented 6 months ago

There is a problem with extra calls of tfw_classify_conn_estab in case when this function return T_BLOCK. To understand this problem look at the tcp handshake: client sends SYN (and switched to TCP_SYN_SENT state) to server, after receiving SYN server (which is in TCP_LISTEN state) sends SYN_ACK to client. After receiving SYN_ACK client send last ACK to server and switched to ESTABLISHED state. In this state client can send data to server. When server receives last ACK in tcp_v4_rcv sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source, th->dest, sdif, &refcounted); is returned in TCP_NEW_SYN_RECV state (it is previously created request sock), then if tcp_check_req->syn_recv_sock->tempesta_new_clntsk fails we send TCP_RST to client. But if client send some just after sending last ACK, we again try to establish connection tcp_check_req->syn_recv_sock->tempesta_new_clntsk. Also we can cat again try to establish connection because client can try to retransmit last ACK of TCP handshake before he receive our RST. Eventually client will be blocked, but count of calls of tfw_classify_conn_estab can be (1 - 3).

krizhanovsky commented 6 months ago

@EvgeniiMekhanik I'm not sure if I understand what's going on correctly:

  1. suppose we receive ACK on a listening socket on the server side (the last ACK in the handshake), so we're in tcp_v4_rcv() and __inet_lookup_skb() returns a request socket.
  2. we go to the sk->sk_state == TCP_NEW_SYN_RECV and call tcp_check_req(), which sends RST and returns nsk = NULL
  3. next we fall to if (!nsk) { in tcp_check_req() and put the request socket reqsk_put(req)

What's the reference counter for req, when it goes to reqsk_put(req)? I'd expect that here we free the request socket, so when we receive another ACK (or anything) from a client after we failed in tfw_classify_conn_estab(), we won't find anything in __inet_lookup_skb() this time and just drop the segment.

EvgeniiMekhanik commented 6 months ago

After calling tcp_conn_request (where we create new request sock) new created request sock has refcounter == 2. The after __inet_lookup_skb it become equal to 3! When tcp_check_req previously we just call reqsk_put as you wrote and refcounter became equal to 2 again! Now we call inet_csk_reqsk_queue_drop also which decrement refcounter two times, so when last reqsk_put is called we really drop and delete request sock!