Skip to content
  • David S. Miller's avatar
    tcp: Revert 'process defer accept as established' changes. · ec0a1966
    David S. Miller authored
    This reverts two changesets, ec3c0982
    ("[TCP]: TCP_DEFER_ACCEPT updates - process as established") and
    the follow-on bug fix 9ae27e0a
    
    
    ("tcp: Fix slab corruption with ipv6 and tcp6fuzz").
    
    This change causes several problems, first reported by Ingo Molnar
    as a distcc-over-loopback regression where connections were getting
    stuck.
    
    Ilpo Järvinen first spotted the locking problems.  The new function
    added by this code, tcp_defer_accept_check(), only has the
    child socket locked, yet it is modifying state of the parent
    listening socket.
    
    Fixing that is non-trivial at best, because we can't simply just grab
    the parent listening socket lock at this point, because it would
    create an ABBA deadlock.  The normal ordering is parent listening
    socket --> child socket, but this code path would require the
    reverse lock ordering.
    
    Next is a problem noticed by Vitaliy Gusev, he noted:
    
    ----------------------------------------
    >--- a/net/ipv4/tcp_timer.c
    >+++ b/net/ipv4/tcp_timer.c
    >@@ -481,6 +481,11 @@ static void tcp_keepalive_timer (unsigned long data)
    > 		goto death;
    > 	}
    >
    >+	if (tp->defer_tcp_accept.request && sk->sk_state == TCP_ESTABLISHED) {
    >+		tcp_send_active_reset(sk, GFP_ATOMIC);
    >+		goto death;
    
    Here socket sk is not attached to listening socket's request queue. tcp_done()
    will not call inet_csk_destroy_sock() (and tcp_v4_destroy_sock() which should
    release this sk) as socket is not DEAD. Therefore socket sk will be lost for
    freeing.
    ----------------------------------------
    
    Finally, Alexey Kuznetsov argues that there might not even be any
    real value or advantage to these new semantics even if we fix all
    of the bugs:
    
    ----------------------------------------
    Hiding from accept() sockets with only out-of-order data only
    is the only thing which is impossible with old approach. Is this really
    so valuable? My opinion: no, this is nothing but a new loophole
    to consume memory without control.
    ----------------------------------------
    
    So revert this thing for now.
    
    Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
    ec0a1966