From af571f421cfce941bb9a9cbcbfaa5241177dcef4 Mon Sep 17 00:00:00 2001 From: Ghanan Gowripalan Date: Tue, 14 Dec 2021 14:49:31 -0800 Subject: [PATCH] Only re-route on non-noop DNAT https://github.com/google/gvisor/commit/115474bcf33c333d8eebad2e96d1b29463f455ba made a change to always perform NAT on NAT-supported hooks so stack.PacketBuffer.{D,S}NATDone will always be true after performing their relevant hooks. As an optimization, only re-routing packets if DNAT was not a no-op. With this change, stack.PacketBuffer.{D,S}NATDone is no longer used outside of the stack package so we can unexpose them. PiperOrigin-RevId: 416400576 --- pkg/tcpip/network/ipv4/ipv4.go | 10 ++++---- pkg/tcpip/network/ipv6/ipv6.go | 10 ++++---- pkg/tcpip/stack/conntrack.go | 6 ++--- pkg/tcpip/stack/iptables.go | 43 +++++++++++++++++--------------- pkg/tcpip/stack/packet_buffer.go | 12 ++++----- 5 files changed, 42 insertions(+), 39 deletions(-) diff --git a/pkg/tcpip/network/ipv4/ipv4.go b/pkg/tcpip/network/ipv4/ipv4.go index e55f5eea6..5aa18f23e 100644 --- a/pkg/tcpip/network/ipv4/ipv4.go +++ b/pkg/tcpip/network/ipv4/ipv4.go @@ -420,7 +420,8 @@ func (e *endpoint) handleFragments(_ *stack.Route, networkMTU uint32, pkt *stack // WritePacket writes a packet to the given destination address and protocol. func (e *endpoint) WritePacket(r *stack.Route, params stack.NetworkHeaderParams, pkt *stack.PacketBuffer) tcpip.Error { - if err := e.addIPHeader(r.LocalAddress(), r.RemoteAddress(), pkt, params, nil /* options */); err != nil { + dstAddr := r.RemoteAddress() + if err := e.addIPHeader(r.LocalAddress(), dstAddr, pkt, params, nil /* options */); err != nil { return err } @@ -433,15 +434,14 @@ func (e *endpoint) WritePacket(r *stack.Route, params stack.NetworkHeaderParams, return nil } - // If the packet is manipulated as per NAT Output rules, handle packet + // If the packet is manipulated as per DNAT Output rules, handle packet // based on destination address and do not send the packet to link // layer. // - // We should do this for every packet, rather than only NATted packets, but + // We should do this for every packet, rather than only DNATted packets, but // removing this check short circuits broadcasts before they are sent out to // other hosts. - if pkt.DNATDone { - netHeader := header.IPv4(pkt.NetworkHeader().View()) + if netHeader := header.IPv4(pkt.NetworkHeader().View()); dstAddr != netHeader.DestinationAddress() { if ep := e.protocol.findEndpointWithAddress(netHeader.DestinationAddress()); ep != nil { // Since we rewrote the packet but it is being routed back to us, we // can safely assume the checksum is valid. diff --git a/pkg/tcpip/network/ipv6/ipv6.go b/pkg/tcpip/network/ipv6/ipv6.go index 0c8ff6fb9..a3ea339e5 100644 --- a/pkg/tcpip/network/ipv6/ipv6.go +++ b/pkg/tcpip/network/ipv6/ipv6.go @@ -742,7 +742,8 @@ func (e *endpoint) handleFragments(r *stack.Route, networkMTU uint32, pkt *stack // WritePacket writes a packet to the given destination address and protocol. func (e *endpoint) WritePacket(r *stack.Route, params stack.NetworkHeaderParams, pkt *stack.PacketBuffer) tcpip.Error { - if err := addIPHeader(r.LocalAddress(), r.RemoteAddress(), pkt, params, nil /* extensionHeaders */); err != nil { + dstAddr := r.RemoteAddress() + if err := addIPHeader(r.LocalAddress(), dstAddr, pkt, params, nil /* extensionHeaders */); err != nil { return err } @@ -755,15 +756,14 @@ func (e *endpoint) WritePacket(r *stack.Route, params stack.NetworkHeaderParams, return nil } - // If the packet is manipulated as per NAT Output rules, handle packet + // If the packet is manipulated as per DNAT Output rules, handle packet // based on destination address and do not send the packet to link // layer. // - // We should do this for every packet, rather than only NATted packets, but + // We should do this for every packet, rather than only DNATted packets, but // removing this check short circuits broadcasts before they are sent out to // other hosts. - if pkt.DNATDone { - netHeader := header.IPv6(pkt.NetworkHeader().View()) + if netHeader := header.IPv6(pkt.NetworkHeader().View()); dstAddr != netHeader.DestinationAddress() { if ep := e.protocol.findEndpointWithAddress(netHeader.DestinationAddress()); ep != nil { // Since we rewrote the packet but it is being routed back to us, we // can safely assume the checksum is valid. diff --git a/pkg/tcpip/stack/conntrack.go b/pkg/tcpip/stack/conntrack.go index af283fcd6..7febd298f 100644 --- a/pkg/tcpip/stack/conntrack.go +++ b/pkg/tcpip/stack/conntrack.go @@ -813,7 +813,7 @@ func (cn *conn) handlePacket(pkt *PacketBuffer, hook Hook, rt *Route) bool { fullChecksum := false updatePseudoHeader := false - natDone := &pkt.SNATDone + natDone := &pkt.snatDone dnat := false switch hook { case Prerouting: @@ -822,13 +822,13 @@ func (cn *conn) handlePacket(pkt *PacketBuffer, hook Hook, rt *Route) bool { fullChecksum = true updatePseudoHeader = true - natDone = &pkt.DNATDone + natDone = &pkt.dnatDone dnat = true case Input: case Forward: panic("should not handle packet in the forwarding hook") case Output: - natDone = &pkt.DNATDone + natDone = &pkt.dnatDone dnat = true fallthrough case Postrouting: diff --git a/pkg/tcpip/stack/iptables.go b/pkg/tcpip/stack/iptables.go index 4fbc86ec0..c3a06b7c9 100644 --- a/pkg/tcpip/stack/iptables.go +++ b/pkg/tcpip/stack/iptables.go @@ -438,25 +438,23 @@ func (it *IPTables) checkNATRLocked(hook Hook, pkt *PacketBuffer, r *Route, addr return true } - var dnat bool - var natDone *bool - switch hook { - case Prerouting, Output: - dnat = true - natDone = &pkt.DNATDone - case Input, Postrouting: - dnat = false - natDone = &pkt.SNATDone - case Forward: - panic("should not attempt NAT in forwarding") - default: - panic(fmt.Sprintf("unhandled hook = %d", hook)) - } + dnat, natDone := func() (bool, bool) { + switch hook { + case Prerouting, Output: + return true, pkt.dnatDone + case Input, Postrouting: + return false, pkt.snatDone + case Forward: + panic("should not attempt NAT in forwarding") + default: + panic(fmt.Sprintf("unhandled hook = %d", hook)) + } + }() // Make sure the connection is NATed. // // If the packet was already NATed, the connection must be NATed. - if !*natDone { + if !natDone { t.conn.maybePerformNoopNAT(dnat) _ = t.conn.handlePacket(pkt, hook, r) } @@ -566,12 +564,17 @@ func (it *IPTables) CheckPostroutingPackets(pkts PacketBufferList, r *Route, add }, false /* dnat */) } +func getAddr(pkt *PacketBuffer, dnat bool) tcpip.Address { + net := pkt.Network() + if dnat { + return net.DestinationAddress() + } + return net.SourceAddress() +} + func checkPackets(pkts PacketBufferList, f func(*PacketBuffer) bool, dnat bool) (drop map[*PacketBuffer]struct{}, natPkts map[*PacketBuffer]struct{}) { for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() { - natDone := &pkt.SNATDone - if dnat { - natDone = &pkt.DNATDone - } + origAddr := getAddr(pkt, dnat) if ok := f(pkt); !ok { if drop == nil { @@ -579,7 +582,7 @@ func checkPackets(pkts PacketBufferList, f func(*PacketBuffer) bool, dnat bool) } drop[pkt] = struct{}{} } - if *natDone { + if newAddr := getAddr(pkt, dnat); newAddr != origAddr { if natPkts == nil { natPkts = make(map[*PacketBuffer]struct{}) } diff --git a/pkg/tcpip/stack/packet_buffer.go b/pkg/tcpip/stack/packet_buffer.go index 2239ba5d5..112a462ec 100644 --- a/pkg/tcpip/stack/packet_buffer.go +++ b/pkg/tcpip/stack/packet_buffer.go @@ -138,13 +138,13 @@ type PacketBuffer struct { EgressRoute RouteInfo GSOOptions GSO - // SNATDone indicates if the packet's source has been manipulated as per + // snatDone indicates if the packet's source has been manipulated as per // iptables NAT table. - SNATDone bool + snatDone bool - // DNATDone indicates if the packet's destination has been manipulated as per + // dnatDone indicates if the packet's destination has been manipulated as per // iptables NAT table. - DNATDone bool + dnatDone bool // PktType indicates the SockAddrLink.PacketType of the packet as defined in // https://www.man7.org/linux/man-pages/man7/packet.7.html. @@ -346,8 +346,8 @@ func (pk *PacketBuffer) Clone() *PacketBuffer { newPk.Owner = pk.Owner newPk.GSOOptions = pk.GSOOptions newPk.NetworkProtocolNumber = pk.NetworkProtocolNumber - newPk.DNATDone = pk.DNATDone - newPk.SNATDone = pk.SNATDone + newPk.dnatDone = pk.dnatDone + newPk.snatDone = pk.snatDone newPk.TransportProtocolNumber = pk.TransportProtocolNumber newPk.PktType = pk.PktType newPk.NICID = pk.NICID