From 7429e8912acb74a61c7928852a02ceaf3c144e81 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 28 Feb 2024 23:21:31 -0500 Subject: [PATCH] wgengine/netstack: fix bug with duplicate SYN packets in client limit This fixes a bug that was introduced in #11258 where the handling of the per-client limit didn't properly account for the fact that the gVisor TCP forwarder will return 'true' to indicate that it's handled a duplicate SYN packet, but not launch the handler goroutine. In such a case, we neither decremented our per-client limit in the wrapper function, nor did we do so in the handler function, leading to our per-client limit table slowly filling up without bound. Fix this by doing the same duplicate-tracking logic that the TCP forwarder does so we can detect such cases and appropriately decrement our in-flight counter. Updates tailscale/corp#12184 Signed-off-by: Andrew Dunham Change-Id: Ib6011a71d382a10d68c0802593f34b8153d06892 --- wgengine/netstack/netstack.go | 63 ++++++++++++++++++++++++------ wgengine/netstack/netstack_test.go | 12 +++++- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/wgengine/netstack/netstack.go b/wgengine/netstack/netstack.go index fe24f23c3..7e7beca3e 100644 --- a/wgengine/netstack/netstack.go +++ b/wgengine/netstack/netstack.go @@ -224,6 +224,19 @@ type Impl struct { // global limit, preventing a misbehaving client from starving the // global limit. connsInFlightByClient map[netip.Addr]int + // packetsInFlight tracks whether we're already handling a packet by + // the given endpoint ID; clients can send repeated SYN packets while + // trying to establish a connection (and while we're dialing the + // upstream address). If we don't deduplicate based on the endpoint, + // each SYN retransmit results in us incrementing + // connsInFlightByClient, and not decrementing them because the + // underlying TCP forwarder returns 'true' to indicate that the packet + // is handled but never actually launches our acceptTCP function. + // + // This mimics the 'inFlight' map in the TCP forwarder; it's + // unfortunate that we have to track this all twice, but thankfully the + // map only holds pending (in-flight) packets, and it's reasonably cheap. + packetsInFlight map[stack.TransportEndpointID]struct{} } const nicID = 1 @@ -315,6 +328,7 @@ func Create(logf logger.Logf, tundev *tstun.Wrapper, e wgengine.Engine, mc *magi dialer: dialer, connsOpenBySubnetIP: make(map[netip.Addr]int), connsInFlightByClient: make(map[netip.Addr]int), + packetsInFlight: make(map[stack.TransportEndpointID]struct{}), dns: dns, tailFSForLocal: tailFSForLocal, } @@ -410,13 +424,28 @@ func (ns *Impl) wrapTCPProtocolHandler(h protocolHandlerFunc) protocolHandlerFun // NOTE: the counter is decremented in // decrementInFlightTCPForward, called from the acceptTCP // function, below. + ns.mu.Lock() + if _, ok := ns.packetsInFlight[tei]; ok { + // We're already handling this packet; just bail early + // (this is also what would happen in the TCP + // forwarder). + ns.mu.Unlock() + return true + } + + // Check the per-client limit. inFlight := ns.connsInFlightByClient[remoteIP] tooManyInFlight := inFlight >= maxInFlightConnectionAttemptsPerClient() if !tooManyInFlight { ns.connsInFlightByClient[remoteIP]++ } + + // We're handling this packet now; see the comment on the + // packetsInFlight field for more details. + ns.packetsInFlight[tei] = struct{}{} ns.mu.Unlock() + if debugNetstack() { ns.logf("[v2] netstack: in-flight connections for client %v: %d", remoteIP, inFlight) } @@ -429,18 +458,23 @@ func (ns *Impl) wrapTCPProtocolHandler(h protocolHandlerFunc) protocolHandlerFun // On return, if this packet isn't handled by the inner handler // we're wrapping (`h`), we need to decrement the per-client - // in-flight count. This can happen if the underlying - // forwarder's limit has been reached, at which point it will - // return false to indicate that it's not handling the packet, - // and it will not run acceptTCP. If we don't decrement here, - // then we would eventually increment the per-client counter up - // to the limit and never decrement because we'd never hit the - // codepath in acceptTCP, below. + // in-flight count and remove the ID from our tracking map. + // This can happen if the underlying forwarder's limit has been + // reached, at which point it will return false to indicate + // that it's not handling the packet, and it will not run + // acceptTCP. If we don't decrement here, then we would + // eventually increment the per-client counter up to the limit + // and never decrement because we'd never hit the codepath in + // acceptTCP, below, or just drop all packets from the same + // endpoint due to the packetsInFlight check. defer func() { if !handled { ns.mu.Lock() + delete(ns.packetsInFlight, tei) ns.connsInFlightByClient[remoteIP]-- + new := ns.connsInFlightByClient[remoteIP] ns.mu.Unlock() + ns.logf("netstack: decrementing connsInFlightByClient[%v] because the packet was not handled; new value is %d", remoteIP, new) } }() @@ -454,10 +488,13 @@ func (ns *Impl) wrapTCPProtocolHandler(h protocolHandlerFunc) protocolHandlerFun } } -func (ns *Impl) decrementInFlightTCPForward(remoteAddr netip.Addr) { +func (ns *Impl) decrementInFlightTCPForward(tei stack.TransportEndpointID, remoteAddr netip.Addr) { ns.mu.Lock() defer ns.mu.Unlock() + // Remove this packet so future SYNs from this address will be handled. + delete(ns.packetsInFlight, tei) + was := ns.connsInFlightByClient[remoteAddr] newVal := was - 1 if newVal == 0 { @@ -1047,12 +1084,14 @@ func (ns *Impl) acceptTCP(r *tcp.ForwarderRequest) { } // After we've returned from this function or have otherwise reached a - // non-pending state, decrement the per-client in-flight count so - // future TCP connections aren't dropped. + // non-pending state, decrement the per-client in-flight count and + // remove this endpoint from our packet tracking map so future TCP + // connections aren't dropped. inFlightCompleted := false + tei := r.ID() defer func() { if !inFlightCompleted { - ns.decrementInFlightTCPForward(clientRemoteIP) + ns.decrementInFlightTCPForward(tei, clientRemoteIP) } }() @@ -1114,7 +1153,7 @@ func (ns *Impl) acceptTCP(r *tcp.ForwarderRequest) { // "in-flight" state; decrement our per-client limit right now, // and tell the defer in acceptTCP that it doesn't need to do // so upon return. - ns.decrementInFlightTCPForward(clientRemoteIP) + ns.decrementInFlightTCPForward(tei, clientRemoteIP) inFlightCompleted = true // The ForwarderRequest.CreateEndpoint above asynchronously diff --git a/wgengine/netstack/netstack_test.go b/wgengine/netstack/netstack_test.go index 7b7f1e72f..73e1d1297 100644 --- a/wgengine/netstack/netstack_test.go +++ b/wgengine/netstack/netstack_test.go @@ -580,6 +580,13 @@ func TestTCPForwardLimits(t *testing.T) { t.Logf("got connection in progress") } + // Inject another packet, which will be deduplicated and thus not + // increment our counter. + parsed.Decode(pkt) + if resp := impl.injectInbound(&parsed, impl.tundev); resp != filter.DropSilently { + t.Errorf("got filter outcome %v, want filter.DropSilently", resp) + } + // Verify that we now have a single in-flight address in our map. impl.mu.Lock() inFlight := maps.Clone(impl.connsInFlightByClient) @@ -633,8 +640,11 @@ func TestTCPForwardLimits_PerClient(t *testing.T) { destAddr := netip.MustParseAddr("192.0.2.1") // Helpers + var port uint16 = 1234 mustInjectPacket := func() { - pkt := tcp4syn(t, client, destAddr, 1234, 4567) + pkt := tcp4syn(t, client, destAddr, port, 4567) + port++ // to avoid deduplication based on endpoint + var parsed packet.Parsed parsed.Decode(pkt)