From a376a0baf362506549fcc58861465fa89ed33f7f Mon Sep 17 00:00:00 2001 From: Ghanan Gowripalan Date: Sat, 26 Sep 2020 19:23:01 -0700 Subject: Remove generic ICMP errors Generic ICMP errors were required because the transport dispatcher was given the responsibility of sending ICMP errors in response to transport packet delivery failures. Instead, the transport dispatcher should let network layer know it failed to deliver a packet (and why) and let the network layer make the decision as to what error to send (if any). Fixes #4068 PiperOrigin-RevId: 333962333 --- pkg/tcpip/stack/forwarder_test.go | 4 ---- pkg/tcpip/stack/nic.go | 48 ++++++++++++++++----------------------- pkg/tcpip/stack/nic_test.go | 5 ---- pkg/tcpip/stack/registration.go | 30 ++++++++++++------------ pkg/tcpip/stack/stack_test.go | 5 ---- 5 files changed, 35 insertions(+), 57 deletions(-) (limited to 'pkg/tcpip/stack') diff --git a/pkg/tcpip/stack/forwarder_test.go b/pkg/tcpip/stack/forwarder_test.go index e30927821..38c5bac71 100644 --- a/pkg/tcpip/stack/forwarder_test.go +++ b/pkg/tcpip/stack/forwarder_test.go @@ -145,10 +145,6 @@ func (*fwdTestNetworkProtocol) Parse(pkt *PacketBuffer) (tcpip.TransportProtocol return tcpip.TransportProtocolNumber(netHeader[protocolNumberOffset]), true, true } -func (*fwdTestNetworkProtocol) ReturnError(*Route, tcpip.ICMPReason, *PacketBuffer) *tcpip.Error { - return nil -} - func (f *fwdTestNetworkProtocol) NewEndpoint(nicID tcpip.NICID, _ LinkAddressCache, _ NUDHandler, dispatcher TransportDispatcher, ep LinkEndpoint, _ *Stack) NetworkEndpoint { return &fwdTestNetworkEndpoint{ nicID: nicID, diff --git a/pkg/tcpip/stack/nic.go b/pkg/tcpip/stack/nic.go index 06d70dd1c..2875a5b60 100644 --- a/pkg/tcpip/stack/nic.go +++ b/pkg/tcpip/stack/nic.go @@ -1398,11 +1398,13 @@ func (n *NIC) forwardPacket(r *Route, protocol tcpip.NetworkProtocolNumber, pkt // DeliverTransportPacket delivers the packets to the appropriate transport // protocol endpoint. -func (n *NIC) DeliverTransportPacket(r *Route, protocol tcpip.TransportProtocolNumber, pkt *PacketBuffer) { +func (n *NIC) DeliverTransportPacket(r *Route, protocol tcpip.TransportProtocolNumber, pkt *PacketBuffer) TransportPacketDisposition { state, ok := n.stack.transportProtocols[protocol] if !ok { + // TODO(gvisor.dev/issue/4365): Let the caller know that the transport + // protocol is unrecognized. n.stack.stats.UnknownProtocolRcvdPackets.Increment() - return + return TransportPacketHandled } transProto := state.proto @@ -1423,59 +1425,47 @@ func (n *NIC) DeliverTransportPacket(r *Route, protocol tcpip.TransportProtocolN // we parse it using the minimum size. if _, ok := pkt.TransportHeader().Consume(transProto.MinimumPacketSize()); !ok { n.stack.stats.MalformedRcvdPackets.Increment() - return + // We consider a malformed transport packet handled because there is + // nothing the caller can do. + return TransportPacketHandled } - } else { - // This is either a bad packet or was re-assembled from fragments. - transProto.Parse(pkt) + } else if !transProto.Parse(pkt) { + n.stack.stats.MalformedRcvdPackets.Increment() + return TransportPacketHandled } } - if pkt.TransportHeader().View().Size() < transProto.MinimumPacketSize() { - n.stack.stats.MalformedRcvdPackets.Increment() - return - } - srcPort, dstPort, err := transProto.ParsePorts(pkt.TransportHeader().View()) if err != nil { n.stack.stats.MalformedRcvdPackets.Increment() - return + return TransportPacketHandled } id := TransportEndpointID{dstPort, r.LocalAddress, srcPort, r.RemoteAddress} if n.stack.demux.deliverPacket(r, protocol, pkt, id) { - return + return TransportPacketHandled } // Try to deliver to per-stack default handler. if state.defaultHandler != nil { if state.defaultHandler(r, id, pkt) { - return + return TransportPacketHandled } } // We could not find an appropriate destination for this packet so // give the protocol specific error handler a chance to handle it. // If it doesn't handle it then we should do so. - switch transProto.HandleUnknownDestinationPacket(r, id, pkt) { + switch res := transProto.HandleUnknownDestinationPacket(r, id, pkt); res { case UnknownDestinationPacketMalformed: n.stack.stats.MalformedRcvdPackets.Increment() + return TransportPacketHandled case UnknownDestinationPacketUnhandled: - // As per RFC: 1122 Section 3.2.2.1 A host SHOULD generate Destination - // Unreachable messages with code: - // 3 (Port Unreachable), when the designated transport protocol - // (e.g., UDP) is unable to demultiplex the datagram but has no - // protocol mechanism to inform the sender. - np, ok := n.stack.networkProtocols[r.NetProto] - if !ok { - // For this to happen stack.makeRoute() must have been called with the - // incorrect protocol number. Since we have successfully completed - // network layer processing this should be impossible. - panic(fmt.Sprintf("expected stack to have a NetworkProtocol for proto = %d", r.NetProto)) - } - - _ = np.ReturnError(r, &tcpip.ICMPReasonPortUnreachable{}, pkt) + return TransportPacketDestinationPortUnreachable case UnknownDestinationPacketHandled: + return TransportPacketHandled + default: + panic(fmt.Sprintf("unrecognized result from HandleUnknownDestinationPacket = %d", res)) } } diff --git a/pkg/tcpip/stack/nic_test.go b/pkg/tcpip/stack/nic_test.go index ef6e63b3e..dd6474297 100644 --- a/pkg/tcpip/stack/nic_test.go +++ b/pkg/tcpip/stack/nic_test.go @@ -221,11 +221,6 @@ func (*testIPv6Protocol) Parse(*PacketBuffer) (tcpip.TransportProtocolNumber, bo return 0, false, false } -// ReturnError implements NetworkProtocol.ReturnError. -func (*testIPv6Protocol) ReturnError(*Route, tcpip.ICMPReason, *PacketBuffer) *tcpip.Error { - return nil -} - var _ LinkAddressResolver = (*testIPv6Protocol)(nil) // LinkAddressProtocol implements LinkAddressResolver. diff --git a/pkg/tcpip/stack/registration.go b/pkg/tcpip/stack/registration.go index 77640cd8a..780a5ebde 100644 --- a/pkg/tcpip/stack/registration.go +++ b/pkg/tcpip/stack/registration.go @@ -197,6 +197,21 @@ type TransportProtocol interface { Parse(pkt *PacketBuffer) (ok bool) } +// TransportPacketDisposition is the result from attempting to deliver a packet +// to the transport layer. +type TransportPacketDisposition int + +const ( + // TransportPacketHandled indicates that a transport packet was handled by the + // transport layer and callers need not take any further action. + TransportPacketHandled TransportPacketDisposition = iota + + // TransportPacketDestinationPortUnreachable indicates that there weren't any + // listeners interested in the packet and the transport protocol has no means + // to notify the sender. + TransportPacketDestinationPortUnreachable +) + // TransportDispatcher contains the methods used by the network stack to deliver // packets to the appropriate transport endpoint after it has been handled by // the network layer. @@ -207,7 +222,7 @@ type TransportDispatcher interface { // pkt.NetworkHeader must be set before calling DeliverTransportPacket. // // DeliverTransportPacket takes ownership of pkt. - DeliverTransportPacket(r *Route, protocol tcpip.TransportProtocolNumber, pkt *PacketBuffer) + DeliverTransportPacket(r *Route, protocol tcpip.TransportProtocolNumber, pkt *PacketBuffer) TransportPacketDisposition // DeliverTransportControlPacket delivers control packets to the // appropriate transport protocol endpoint. @@ -342,19 +357,6 @@ type NetworkProtocol interface { // does not encapsulate anything). // - Whether pkt.Data was large enough to parse and set pkt.NetworkHeader. Parse(pkt *PacketBuffer) (proto tcpip.TransportProtocolNumber, hasTransportHdr bool, ok bool) - - // ReturnError attempts to send a suitable error message to the sender - // of a received packet. - // - pkt holds the problematic packet. - // - reason indicates what the reason for wanting a message is. - // - route is the routing information for the received packet - // ReturnError returns an error if the send failed and nil on success. - // Note that deciding to deliberately send no message is a success. - // - // TODO(gvisor.dev/issues/3871): This method should be removed or simplified - // after all (or all but one) of the ICMP error dispatch occurs through the - // protocol specific modules. May become SendPortNotFound(r, pkt). - ReturnError(r *Route, reason tcpip.ICMPReason, pkt *PacketBuffer) *tcpip.Error } // NetworkDispatcher contains the methods used by the network stack to deliver diff --git a/pkg/tcpip/stack/stack_test.go b/pkg/tcpip/stack/stack_test.go index 9ef6787c6..f92850053 100644 --- a/pkg/tcpip/stack/stack_test.go +++ b/pkg/tcpip/stack/stack_test.go @@ -216,11 +216,6 @@ func (f *fakeNetworkProtocol) Option(option tcpip.GettableNetworkProtocolOption) } } -// ReturnError implements NetworkProtocol.ReturnError -func (*fakeNetworkProtocol) ReturnError(*stack.Route, tcpip.ICMPReason, *stack.PacketBuffer) *tcpip.Error { - return nil -} - // Close implements NetworkProtocol.Close. func (*fakeNetworkProtocol) Close() {} -- cgit v1.2.3