From c122d8d6c82da43a6de4879a5f08365c6ba3c85f Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Wed, 15 Mar 2023 10:52:04 -0700 Subject: [PATCH] gro: fix bug where handshake packets would be stuck waiting in GRO Also enables GRO in syscall tests. GRO should be totally transparent and should not affect behavior. This ensures it's tested and fixes the bug it uncovered. GRO would not immediately flush the final ACK in the SYN-SYN/ACK-ACK handshake. This could lead to a situation where - Client A calls connect(), which returns once the final ACK of the handshake is sent and A reaches state ESTABLISHED - The ACK gets GRO'd, delaying it from reaching the server - Client B calls non-blocking connect() - Client B's ACK gets GRO'd as well - Client B is marked as ESTABLISHED - The server, with accept queue size 1, is only going to accept one connection, but two clients are ESTABLISHED. It now immediately flushes packets with no payload, as they are important to TCP connection state and management. PiperOrigin-RevId: 516870932 --- pkg/tcpip/stack/gro.go | 5 +++-- runsc/boot/network.go | 20 +++++++++++++------- runsc/sandbox/network.go | 16 ++++++++++------ test/runner/main.go | 1 + 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/pkg/tcpip/stack/gro.go b/pkg/tcpip/stack/gro.go index a3b708fd5..29a1b7061 100644 --- a/pkg/tcpip/stack/gro.go +++ b/pkg/tcpip/stack/gro.go @@ -226,6 +226,8 @@ func (gb *groBucket) found(gd *groDispatcher, groPkt *groPacket, flushGROPkt boo // Flush groPkt or merge the packets. pktSize := pkt.Data().Size() flags := tcpHdr.Flags() + dataOff := tcpHdr.DataOffset() + tcpPayloadSize := pkt.Data().Size() - len(ipHdr) - int(dataOff) if flushGROPkt { // Flush the existing GRO packet. Don't hold bucket.mu while // processing the packet. @@ -239,12 +241,10 @@ func (gb *groBucket) found(gd *groDispatcher, groPkt *groPacket, flushGROPkt boo } else if groPkt != nil { // Merge pkt in to GRO packet. buf := pkt.Data().ToBuffer() - dataOff := tcpHdr.DataOffset() buf.TrimFront(int64(len(ipHdr)) + int64(dataOff)) groPkt.pkt.Data().MergeBuffer(&buf) buf.Release() // Update the IP total length. - tcpPayloadSize := pkt.Data().Size() - len(ipHdr) - int(dataOff) updateIPHdr(groPkt.ipHdr, tcpPayloadSize) // Add flags from the packet to the GRO packet. groPkt.tcpHdr.SetFlags(uint8(groPkt.tcpHdr.Flags() | (flags & (header.TCPFlagFin | header.TCPFlagPsh)))) @@ -261,6 +261,7 @@ func (gb *groBucket) found(gd *groDispatcher, groPkt *groPacket, flushGROPkt boo // malformed, a local GSO packet, or has already been handled by host // GRO. flush := header.TCPFlags(flags)&(header.TCPFlagUrg|header.TCPFlagPsh|header.TCPFlagRst|header.TCPFlagSyn|header.TCPFlagFin) != 0 + flush = flush || tcpPayloadSize == 0 if groPkt != nil { flush = flush || pktSize != groPkt.initialLength } diff --git a/runsc/boot/network.go b/runsc/boot/network.go index b7b34c630..4f9c2d112 100644 --- a/runsc/boot/network.go +++ b/runsc/boot/network.go @@ -121,17 +121,19 @@ type XDPLink struct { LinkAddress net.HardwareAddr QDisc config.QueueingDiscipline Neighbors []Neighbor + GvisorGROTimeout time.Duration // NumChannels controls how many underlying FDs are to be used to // create this endpoint. NumChannels int } -// LoopbackLink configures a loopback li nk. +// LoopbackLink configures a loopback link. type LoopbackLink struct { - Name string - Addresses []IPWithPrefix - Routes []Route + Name string + Addresses []IPWithPrefix + Routes []Route + GvisorGROTimeout time.Duration } // CreateLinksAndRoutesArgs are arguments to CreateLinkAndRoutes. @@ -216,7 +218,10 @@ func (n *Network) CreateLinksAndRoutes(args *CreateLinksAndRoutesArgs, _ *struct linkEP := packetsocket.New(ethernet.New(loopback.New())) log.Infof("Enabling loopback interface %q with id %d on addresses %+v", link.Name, nicID, link.Addresses) - opts := stack.NICOptions{Name: link.Name} + opts := stack.NICOptions{ + Name: link.Name, + GROTimeout: link.GvisorGROTimeout, + } if err := n.createNICWithAddrs(nicID, linkEP, opts, link.Addresses); err != nil { return err } @@ -383,8 +388,9 @@ func (n *Network) CreateLinksAndRoutes(args *CreateLinksAndRoutesArgs, _ *struct log.Infof("Enabling interface %q with id %d on addresses %+v (%v) w/ %d channels", link.Name, nicID, link.Addresses, mac, link.NumChannels) opts := stack.NICOptions{ - Name: link.Name, - QDisc: qDisc, + Name: link.Name, + QDisc: qDisc, + GROTimeout: link.GvisorGROTimeout, } if err := n.createNICWithAddrs(nicID, sniffEP, opts, link.Addresses); err != nil { return err diff --git a/runsc/sandbox/network.go b/runsc/sandbox/network.go index f5b1a00b3..5a9773f19 100644 --- a/runsc/sandbox/network.go +++ b/runsc/sandbox/network.go @@ -60,7 +60,7 @@ func setupNetwork(conn *urpc.Client, pid int, conf *config.Config) error { switch conf.Network { case config.NetworkNone: log.Infof("Network is disabled, create loopback interface only") - if err := createDefaultLoopbackInterface(conn); err != nil { + if err := createDefaultLoopbackInterface(conf, conn); err != nil { return fmt.Errorf("creating default loopback interface: %v", err) } case config.NetworkSandbox: @@ -78,9 +78,11 @@ func setupNetwork(conn *urpc.Client, pid int, conf *config.Config) error { return nil } -func createDefaultLoopbackInterface(conn *urpc.Client) error { +func createDefaultLoopbackInterface(conf *config.Config, conn *urpc.Client) error { + link := boot.DefaultLoopbackLink + link.GvisorGROTimeout = conf.GvisorGROTimeout if err := conn.Call(boot.NetworkCreateLinksAndRoutes, &boot.CreateLinksAndRoutesArgs{ - LoopbackLinks: []boot.LoopbackLink{boot.DefaultLoopbackLink}, + LoopbackLinks: []boot.LoopbackLink{link}, }, nil); err != nil { return fmt.Errorf("creating loopback link and routes: %v", err) } @@ -157,7 +159,7 @@ func createInterfacesAndRoutesFromNS(conn *urpc.Client, nsPath string, conf *con // We build our own loopback device. if iface.Flags&net.FlagLoopback != 0 { - link, err := loopbackLink(iface, allAddrs) + link, err := loopbackLink(conf, iface, allAddrs) if err != nil { return fmt.Errorf("getting loopback link for iface %q: %w", iface.Name, err) } @@ -261,6 +263,7 @@ func createInterfacesAndRoutesFromNS(conn *urpc.Client, nsPath string, conf *con Neighbors: neighbors, LinkAddress: linkAddress, Addresses: addresses, + GvisorGROTimeout: conf.GvisorGROTimeout, }) } else { link := boot.FDBasedLink{ @@ -492,9 +495,10 @@ func createSocketXDP(iface net.Interface) ([]*os.File, error) { // loopbackLink returns the link with addresses and routes for a loopback // interface. -func loopbackLink(iface net.Interface, addrs []net.Addr) (boot.LoopbackLink, error) { +func loopbackLink(conf *config.Config, iface net.Interface, addrs []net.Addr) (boot.LoopbackLink, error) { link := boot.LoopbackLink{ - Name: iface.Name, + Name: iface.Name, + GvisorGROTimeout: conf.GvisorGROTimeout, } for _, addr := range addrs { ipNet, ok := addr.(*net.IPNet) diff --git a/test/runner/main.go b/test/runner/main.go index c017499dd..5ad4ceaf7 100644 --- a/test/runner/main.go +++ b/test/runner/main.go @@ -229,6 +229,7 @@ func runRunsc(tc *gtest.TestCase, spec *specs.Spec) error { "-watchdog-action=panic", "-platform", *platform, "-file-access", *fileAccess, + "-gvisor-gro=200000ns", } if *network == "host" && !testutil.TestEnvSupportsRawSockets {