From 8ebc4d767a432c7a4a97e25e048a1380b2a2e442 Mon Sep 17 00:00:00 2001 From: Ghanan Gowripalan Date: Thu, 5 Jan 2023 11:13:51 -0800 Subject: [PATCH] Accept no buffer space errors when forwarding The outgoing device queue may be full when forwarding packets and those errors should not result in a panic. PiperOrigin-RevId: 499946528 --- pkg/tcpip/network/internal/ip/errors.go | 8 + pkg/tcpip/network/internal/ip/stats.go | 5 + pkg/tcpip/network/ipv4/ipv4.go | 6 +- pkg/tcpip/network/ipv6/ipv6.go | 6 +- pkg/tcpip/tcpip.go | 4 + pkg/tcpip/tests/integration/forward_test.go | 159 +++++++++++++------- 6 files changed, 132 insertions(+), 56 deletions(-) diff --git a/pkg/tcpip/network/internal/ip/errors.go b/pkg/tcpip/network/internal/ip/errors.go index 5ff59fd59..0624996f3 100644 --- a/pkg/tcpip/network/internal/ip/errors.go +++ b/pkg/tcpip/network/internal/ip/errors.go @@ -34,6 +34,14 @@ func (*ErrTTLExceeded) isForwardingError() {} func (*ErrTTLExceeded) String() string { return "ttl exceeded" } +// ErrOutgoingDeviceNoBufferSpace indicates that the outgoing device does not +// have enough space to hold a buffer. +type ErrOutgoingDeviceNoBufferSpace struct{} + +func (*ErrOutgoingDeviceNoBufferSpace) isForwardingError() {} + +func (*ErrOutgoingDeviceNoBufferSpace) String() string { return "no device buffer space" } + // ErrParameterProblem indicates the received packet had a problem with an IP // parameter. type ErrParameterProblem struct{} diff --git a/pkg/tcpip/network/internal/ip/stats.go b/pkg/tcpip/network/internal/ip/stats.go index 83b0b63fb..9861bce0d 100644 --- a/pkg/tcpip/network/internal/ip/stats.go +++ b/pkg/tcpip/network/internal/ip/stats.go @@ -64,6 +64,10 @@ type MultiCounterIPForwardingStats struct { // were dropped due to insufficent buffer space in the pending packet queue. NoMulticastPendingQueueBufferSpace tcpip.MultiCounterStat + // OutgoingDeviceNoBufferSpace is the number of packets that were dropped due + // to insufficient space in the outgoing device. + OutgoingDeviceNoBufferSpace tcpip.MultiCounterStat + // Errors is the number of IP packets received which could not be // successfully forwarded. Errors tcpip.MultiCounterStat @@ -82,6 +86,7 @@ func (m *MultiCounterIPForwardingStats) Init(a, b *tcpip.IPForwardingStats) { m.UnexpectedMulticastInputInterface.Init(a.UnexpectedMulticastInputInterface, b.UnexpectedMulticastInputInterface) m.UnknownOutputEndpoint.Init(a.UnknownOutputEndpoint, b.UnknownOutputEndpoint) m.NoMulticastPendingQueueBufferSpace.Init(a.NoMulticastPendingQueueBufferSpace, b.NoMulticastPendingQueueBufferSpace) + m.OutgoingDeviceNoBufferSpace.Init(a.OutgoingDeviceNoBufferSpace, b.OutgoingDeviceNoBufferSpace) } // LINT.ThenChange(:MultiCounterIPForwardingStats, ../../../tcpip.go:IPForwardingStats) diff --git a/pkg/tcpip/network/ipv4/ipv4.go b/pkg/tcpip/network/ipv4/ipv4.go index c669b888d..b70e3bc9d 100644 --- a/pkg/tcpip/network/ipv4/ipv4.go +++ b/pkg/tcpip/network/ipv4/ipv4.go @@ -691,6 +691,8 @@ func (e *endpoint) forwardPacketWithRoute(route *stack.Route, pkt stack.PacketBu // necessary and the bit is also set. _ = e.protocol.returnError(&icmpReasonFragmentationNeeded{}, pkt, false /* deliveredLocally */) return &ip.ErrMessageTooLong{} + case *tcpip.ErrNoBufferSpace: + return &ip.ErrOutgoingDeviceNoBufferSpace{} default: return &ip.ErrOther{Err: err} } @@ -1105,7 +1107,7 @@ func (e *endpoint) handleValidatedPacket(h header.IPv4, pkt stack.PacketBufferPt // counters. func (e *endpoint) handleForwardingError(err ip.ForwardingError) { stats := e.stats.ip - switch err.(type) { + switch err := err.(type) { case nil: return case *ip.ErrLinkLocalSourceAddress: @@ -1126,6 +1128,8 @@ func (e *endpoint) handleForwardingError(err ip.ForwardingError) { stats.Forwarding.UnexpectedMulticastInputInterface.Increment() case *ip.ErrUnknownOutputEndpoint: stats.Forwarding.UnknownOutputEndpoint.Increment() + case *ip.ErrOutgoingDeviceNoBufferSpace: + stats.Forwarding.OutgoingDeviceNoBufferSpace.Increment() default: panic(fmt.Sprintf("unrecognized forwarding error: %s", err)) } diff --git a/pkg/tcpip/network/ipv6/ipv6.go b/pkg/tcpip/network/ipv6/ipv6.go index 9bdd30b64..31f48b37f 100644 --- a/pkg/tcpip/network/ipv6/ipv6.go +++ b/pkg/tcpip/network/ipv6/ipv6.go @@ -1029,6 +1029,8 @@ func (e *endpoint) forwardPacketWithRoute(route *stack.Route, pkt stack.PacketBu // outgoing link. _ = e.protocol.returnError(&icmpReasonPacketTooBig{}, pkt, false /* deliveredLocally */) return &ip.ErrMessageTooLong{} + case *tcpip.ErrNoBufferSpace: + return &ip.ErrOutgoingDeviceNoBufferSpace{} default: return &ip.ErrOther{Err: err} } @@ -1227,7 +1229,7 @@ func (e *endpoint) forwardMulticastPacketForOutgoingInterface(pkt stack.PacketBu // counters. func (e *endpoint) handleForwardingError(err ip.ForwardingError) { stats := e.stats.ip - switch err.(type) { + switch err := err.(type) { case nil: return case *ip.ErrLinkLocalSourceAddress: @@ -1248,6 +1250,8 @@ func (e *endpoint) handleForwardingError(err ip.ForwardingError) { stats.Forwarding.UnexpectedMulticastInputInterface.Increment() case *ip.ErrUnknownOutputEndpoint: stats.Forwarding.UnknownOutputEndpoint.Increment() + case *ip.ErrOutgoingDeviceNoBufferSpace: + stats.Forwarding.OutgoingDeviceNoBufferSpace.Increment() default: panic(fmt.Sprintf("unrecognized forwarding error: %s", err)) } diff --git a/pkg/tcpip/tcpip.go b/pkg/tcpip/tcpip.go index a43970901..e4c8d666b 100644 --- a/pkg/tcpip/tcpip.go +++ b/pkg/tcpip/tcpip.go @@ -1739,6 +1739,10 @@ type IPForwardingStats struct { // were dropped due to insufficent buffer space in the pending packet queue. NoMulticastPendingQueueBufferSpace *StatCounter + // OutgoingDeviceNoBufferSpace is the number of packets that were dropped due + // to insufficient space in the outgoing device. + OutgoingDeviceNoBufferSpace *StatCounter + // Errors is the number of IP packets received which could not be // successfully forwarded. Errors *StatCounter diff --git a/pkg/tcpip/tests/integration/forward_test.go b/pkg/tcpip/tests/integration/forward_test.go index 82702bf4a..ad0818e97 100644 --- a/pkg/tcpip/tests/integration/forward_test.go +++ b/pkg/tcpip/tests/integration/forward_test.go @@ -349,6 +349,19 @@ func TestForwarding(t *testing.T) { } } +type fillableLinkEndpoint struct { + *channel.Endpoint + full bool +} + +func (e *fillableLinkEndpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error) { + if e.full { + return 0, &tcpip.ErrNoBufferSpace{} + } + + return e.Endpoint.WritePackets(pkts) +} + func TestUnicastForwarding(t *testing.T) { const ( nicID1 = 1 @@ -362,6 +375,7 @@ func TestUnicastForwarding(t *testing.T) { tests := []struct { name string + netProto tcpip.NetworkProtocolNumber srcAddr, dstAddr tcpip.Address rx func(*channel.Endpoint, tcpip.Address, tcpip.Address) expectForward bool @@ -369,6 +383,7 @@ func TestUnicastForwarding(t *testing.T) { }{ { name: "IPv4 link-local source", + netProto: ipv4.ProtocolNumber, srcAddr: ipv4LinkLocalUnicastAddr, dstAddr: utils.RemoteIPv4Addr, rx: rxICMPv4EchoRequest, @@ -376,6 +391,7 @@ func TestUnicastForwarding(t *testing.T) { }, { name: "IPv4 link-local destination", + netProto: ipv4.ProtocolNumber, srcAddr: utils.RemoteIPv4Addr, dstAddr: ipv4LinkLocalUnicastAddr, rx: rxICMPv4EchoRequest, @@ -383,6 +399,7 @@ func TestUnicastForwarding(t *testing.T) { }, { name: "IPv4 non-link-local unicast", + netProto: ipv4.ProtocolNumber, srcAddr: utils.RemoteIPv4Addr, dstAddr: utils.Ipv4Addr2.AddressWithPrefix.Address, rx: rxICMPv4EchoRequest, @@ -393,6 +410,7 @@ func TestUnicastForwarding(t *testing.T) { }, { name: "IPv6 link-local source", + netProto: ipv6.ProtocolNumber, srcAddr: ipv6LinkLocalUnicastAddr, dstAddr: utils.RemoteIPv6Addr, rx: rxICMPv6EchoRequest, @@ -400,6 +418,7 @@ func TestUnicastForwarding(t *testing.T) { }, { name: "IPv6 link-local destination", + netProto: ipv6.ProtocolNumber, srcAddr: utils.RemoteIPv6Addr, dstAddr: ipv6LinkLocalUnicastAddr, rx: rxICMPv6EchoRequest, @@ -407,6 +426,7 @@ func TestUnicastForwarding(t *testing.T) { }, { name: "IPv6 non-link-local unicast", + netProto: ipv6.ProtocolNumber, srcAddr: utils.RemoteIPv6Addr, dstAddr: utils.Ipv6Addr2.AddressWithPrefix.Address, rx: rxICMPv6EchoRequest, @@ -419,68 +439,99 @@ func TestUnicastForwarding(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - s := stack.New(stack.Options{ - NetworkProtocols: []stack.NetworkProtocolFactory{ipv4.NewProtocol, ipv6.NewProtocol}, - TransportProtocols: []stack.TransportProtocolFactory{udp.NewProtocol}, - }) + for _, full := range []bool{true, false} { + t.Run(fmt.Sprintf("Full=%t", full), func(t *testing.T) { + s := stack.New(stack.Options{ + NetworkProtocols: []stack.NetworkProtocolFactory{ipv4.NewProtocol, ipv6.NewProtocol}, + TransportProtocols: []stack.TransportProtocolFactory{udp.NewProtocol}, + }) - e1 := channel.New(1, header.IPv6MinimumMTU, "") - defer e1.Close() - if err := s.CreateNIC(nicID1, e1); err != nil { - t.Fatalf("s.CreateNIC(%d, _): %s", nicID1, err) - } + e1 := channel.New(1, header.IPv6MinimumMTU, "") + defer e1.Close() + if err := s.CreateNIC(nicID1, e1); err != nil { + t.Fatalf("s.CreateNIC(%d, _): %s", nicID1, err) + } - e2 := channel.New(1, header.IPv6MinimumMTU, "") - defer e2.Close() - if err := s.CreateNIC(nicID2, e2); err != nil { - t.Fatalf("s.CreateNIC(%d, _): %s", nicID2, err) - } + e2 := fillableLinkEndpoint{Endpoint: channel.New(1, header.IPv6MinimumMTU, ""), full: full} + defer e2.Close() + if err := s.CreateNIC(nicID2, &e2); err != nil { + t.Fatalf("s.CreateNIC(%d, _): %s", nicID2, err) + } - protocolAddrV4 := tcpip.ProtocolAddress{ - Protocol: ipv4.ProtocolNumber, - AddressWithPrefix: utils.Ipv4Addr, - } - if err := s.AddProtocolAddress(nicID2, protocolAddrV4, stack.AddressProperties{}); err != nil { - t.Fatalf("AddProtocolAddress(%d, %+v, {}): %s", nicID2, protocolAddrV4, err) - } - protocolAddrV6 := tcpip.ProtocolAddress{ - Protocol: ipv6.ProtocolNumber, - AddressWithPrefix: utils.Ipv6Addr, - } - if err := s.AddProtocolAddress(nicID2, protocolAddrV6, stack.AddressProperties{}); err != nil { - t.Fatalf("AddProtocolAddress(%d, %+v, {}): %s", nicID2, protocolAddrV6, err) - } + protocolAddrV4 := tcpip.ProtocolAddress{ + Protocol: ipv4.ProtocolNumber, + AddressWithPrefix: utils.Ipv4Addr, + } + if err := s.AddProtocolAddress(nicID2, protocolAddrV4, stack.AddressProperties{}); err != nil { + t.Fatalf("AddProtocolAddress(%d, %+v, {}): %s", nicID2, protocolAddrV4, err) + } + protocolAddrV6 := tcpip.ProtocolAddress{ + Protocol: ipv6.ProtocolNumber, + AddressWithPrefix: utils.Ipv6Addr, + } + if err := s.AddProtocolAddress(nicID2, protocolAddrV6, stack.AddressProperties{}); err != nil { + t.Fatalf("AddProtocolAddress(%d, %+v, {}): %s", nicID2, protocolAddrV6, err) + } - if err := s.SetForwardingDefaultAndAllNICs(ipv4.ProtocolNumber, true); err != nil { - t.Fatalf("s.SetForwardingDefaultAndAllNICs(%d, true): %s", ipv4.ProtocolNumber, err) - } - if err := s.SetForwardingDefaultAndAllNICs(ipv6.ProtocolNumber, true); err != nil { - t.Fatalf("s.SetForwardingDefaultAndAllNICs(%d, true): %s", ipv6.ProtocolNumber, err) - } + if err := s.SetForwardingDefaultAndAllNICs(ipv4.ProtocolNumber, true); err != nil { + t.Fatalf("s.SetForwardingDefaultAndAllNICs(%d, true): %s", ipv4.ProtocolNumber, err) + } + if err := s.SetForwardingDefaultAndAllNICs(ipv6.ProtocolNumber, true); err != nil { + t.Fatalf("s.SetForwardingDefaultAndAllNICs(%d, true): %s", ipv6.ProtocolNumber, err) + } - s.SetRouteTable([]tcpip.Route{ - { - Destination: header.IPv4EmptySubnet, - NIC: nicID2, - }, - { - Destination: header.IPv6EmptySubnet, - NIC: nicID2, - }, - }) + s.SetRouteTable([]tcpip.Route{ + { + Destination: header.IPv4EmptySubnet, + NIC: nicID2, + }, + { + Destination: header.IPv6EmptySubnet, + NIC: nicID2, + }, + }) - test.rx(e1, test.srcAddr, test.dstAddr) + test.rx(e1, test.srcAddr, test.dstAddr) - p := e2.Read() - if (!p.IsNil()) != test.expectForward { - t.Fatalf("got e2.Read() = %#v, want = (_ == nil) = %t", p, test.expectForward) - } + expectForward := test.expectForward && !full + p := e2.Read() + if (!p.IsNil()) != expectForward { + t.Fatalf("got e2.Read() = %#v, want = (_ == nil) = %t", p, expectForward) + } - if test.expectForward { - payload := stack.PayloadSince(p.NetworkHeader()) - defer payload.Release() - test.checker(t, payload) - p.DecRef() + if expectForward { + payload := stack.PayloadSince(p.NetworkHeader()) + defer payload.Release() + test.checker(t, payload) + p.DecRef() + } + + checkOutgoingDeviceNoBufferSpaceCounter := func(nicID tcpip.NICID, expectErr bool) { + t.Helper() + + expectCounter := uint64(0) + if expectErr { + expectCounter = 1 + } + + netEP, err := s.GetNetworkEndpoint(nicID, test.netProto) + if err != nil { + t.Fatalf("s.GetNetworkEndpoint(%d, %d): %s", nicID, test.netProto, err) + } + + stats := netEP.Stats() + ipStats, ok := stats.(stack.IPNetworkEndpointStats) + if !ok { + t.Fatalf("%#v is not a %T", stats, ipStats) + } + + if got := ipStats.IPStats().Forwarding.OutgoingDeviceNoBufferSpace.Value(); got != expectCounter { + t.Errorf("got ipStats.IPStats().Forwarding.OutgoingDeviceNoBufferSpace.Value() = %d, want = %d", got, expectCounter) + } + } + checkOutgoingDeviceNoBufferSpaceCounter(nicID1, test.expectForward && full) + checkOutgoingDeviceNoBufferSpaceCounter(nicID2, false) + }) } }) }