Move ICMP frag-needed handling into sleeveForwarder

router/connection.go

@@ -379,8 +379,8 @@ func (conn *LocalConnection) extendReadDeadline() {
 	conn.TCPConn.SetReadDeadline(time.Now().Add(TCPHeartbeat * 2))
 }
 
-func (conn *LocalConnection) Forward(srcPeer, dstPeer *Peer, frame []byte, dec *EthernetDecoder) error {
-	return conn.forwarder.Forward(srcPeer, dstPeer, frame, dec)
+func (conn *LocalConnection) Forward(srcPeer, dstPeer *Peer, frame []byte, dec *EthernetDecoder, broadcast bool) {
+	conn.forwarder.Forward(srcPeer, dstPeer, frame, dec, broadcast)
 }
 
 func tickerChan(ticker *time.Ticker) <-chan time.Time {

router/ethernet_decoder.go

@@ -25,7 +25,7 @@ func (dec *EthernetDecoder) DecodeLayers(data []byte) error {
 	return dec.parser.DecodeLayers(data, &dec.decoded)
 }
 
-func (dec *EthernetDecoder) sendICMPFragNeeded(mtu int, sendFrame func([]byte) error) error {
+func (dec *EthernetDecoder) makeICMPFragNeeded(mtu int) ([]byte, error) {
 	buf := gopacket.NewSerializeBuffer()
 	opts := gopacket.SerializeOptions{
 		FixLengths:       true,
@@ -53,11 +53,11 @@ func (dec *EthernetDecoder) sendICMPFragNeeded(mtu int, sendFrame func([]byte) e
 			Seq:      uint16(mtu)},
 		&payload)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	log.Printf("Sending ICMP 3,4 (%v -> %v): PMTU= %v\n", dec.IP.DstIP, dec.IP.SrcIP, mtu)
-	return sendFrame(buf.Bytes())
+	return buf.Bytes(), nil
 }
 
 var (

router/interhost.go

@@ -41,7 +41,8 @@ type InterHostForwarder interface {
 	// Forward a packet across the connection.  The caller must
 	// supply an EthernetDecoder specific to this thread, which
 	// has already been used to decode the frame.
-	Forward(src *Peer, dest *Peer, frame []byte, dec *EthernetDecoder) error
+	Forward(src *Peer, dest *Peer, frame []byte, dec *EthernetDecoder,
+		broadcast bool)
 
 	Close()
 
@@ -68,8 +69,8 @@ func (NullInterHost) MakeForwarder(*Peer, net.IP, *net.UDPAddr, uint64,
 func (NullInterHost) SetListener(InterHostForwarderListener) {
 }
 
-func (NullInterHost) Forward(*Peer, *Peer, []byte, *EthernetDecoder) error {
-	return nil
+func (NullInterHost) Forward(*Peer, *Peer, []byte, *EthernetDecoder,
+	bool) {
 }
 
 func (NullInterHost) Close() {

router/local_peer.go

@@ -28,29 +28,29 @@ func NewLocalPeer(name PeerName, nickName string, router *Router) *LocalPeer {
 	return peer
 }
 
-func (peer *LocalPeer) Forward(dstPeer *Peer, frame []byte, dec *EthernetDecoder) error {
-	return peer.Relay(peer.Peer, dstPeer, frame, dec)
+func (peer *LocalPeer) Forward(dstPeer *Peer, frame []byte, dec *EthernetDecoder) {
+	peer.Relay(peer.Peer, dstPeer, frame, dec)
 }
 
 func (peer *LocalPeer) Broadcast(frame []byte, dec *EthernetDecoder) {
 	peer.RelayBroadcast(peer.Peer, frame, dec)
 }
 
-func (peer *LocalPeer) Relay(srcPeer, dstPeer *Peer, frame []byte, dec *EthernetDecoder) error {
+func (peer *LocalPeer) Relay(srcPeer, dstPeer *Peer, frame []byte, dec *EthernetDecoder) {
 	relayPeerName, found := peer.router.Routes.Unicast(dstPeer.Name)
 	if !found {
 		// Not necessarily an error as there could be a race with the
 		// dst disappearing whilst the frame is in flight
 		log.Println("Received packet for unknown destination:", dstPeer)
-		return nil
+		return
 	}
 	conn, found := peer.ConnectionTo(relayPeerName)
 	if !found {
 		// Again, could just be a race, not necessarily an error
 		log.Println("Unable to find connection to relay peer", relayPeerName)
-		return nil
+		return
 	}
-	return conn.(*LocalConnection).Forward(srcPeer, dstPeer, frame, dec)
+	conn.(*LocalConnection).Forward(srcPeer, dstPeer, frame, dec, false)
 }
 
 func (peer *LocalPeer) RelayBroadcast(srcPeer *Peer, frame []byte, dec *EthernetDecoder) {
@@ -59,15 +59,8 @@ func (peer *LocalPeer) RelayBroadcast(srcPeer *Peer, frame []byte, dec *Ethernet
 		return
 	}
 	for _, conn := range peer.ConnectionsTo(nextHops) {
-		err := conn.(*LocalConnection).Forward(srcPeer, conn.Remote(),
-			frame, dec)
-		if err != nil {
-			if ftbe, ok := err.(FrameTooBigError); ok {
-				log.Printf("dropping too big DF broadcast frame (%v -> %v): PMTU= %v\n", dec.IP.DstIP, dec.IP.SrcIP, ftbe.EPMTU)
-			} else {
-				log.Println(err)
-			}
-		}
+		conn.(*LocalConnection).Forward(srcPeer, conn.Remote(), frame,
+			dec, true)
 	}
 }
 

router/router.go

@@ -135,11 +135,7 @@ func (router *Router) CapturedPacket(frameData []byte, dec *EthernetDecoder) {
 		return
 	}
 
-	err := router.Ourself.Forward(dstPeer, frameCopy, dec)
-	if ftbe, ok := err.(FrameTooBigError); ok {
-		err = dec.sendICMPFragNeeded(ftbe.EPMTU, router.IntraHost.InjectPacket)
-	}
-	checkWarn(err)
+	router.Ourself.Forward(dstPeer, frameCopy, dec)
 }
 
 func (router *Router) listenTCP(localPort int) {
@@ -175,14 +171,7 @@ func (router *Router) handleForwardedPacket(srcPeer *Peer, dstPeer *Peer,
 	if dstPeer != router.Ourself.Peer {
 		// it's not for us, we're just relaying it
 		router.LogFrame("Relaying", frame, dec)
-		err := router.Ourself.Relay(srcPeer, dstPeer, frame, dec)
-		if ftbe, ok := err.(FrameTooBigError); ok {
-			err = dec.sendICMPFragNeeded(ftbe.EPMTU, func(icmpFrame []byte) error {
-				return router.Ourself.Forward(srcPeer, icmpFrame, nil)
-			})
-		}
-
-		checkWarn(err)
+		router.Ourself.Relay(srcPeer, dstPeer, frame, dec)
 		return
 	}
 

router/sleeve.go

@@ -346,12 +346,7 @@ func (fwd *sleeveForwarder) SetListener(listener InterHostForwarderListener) {
 }
 
 func (fwd *sleeveForwarder) Forward(src *Peer, dst *Peer, frame []byte,
-	dec *EthernetDecoder) error {
-	var df bool
-	if dec != nil {
-		df = dec.DF()
-	}
-
+	dec *EthernetDecoder, broadcast bool) {
 	fwd.lock.RLock()
 	haveContact := (fwd.remoteAddr != nil)
 	effectivePMTU := fwd.effectivePMTU
@@ -360,7 +355,7 @@ func (fwd *sleeveForwarder) Forward(src *Peer, dst *Peer, frame []byte,
 
 	if !haveContact {
 		fwd.log("Cannot forward frame yet - awaiting contact")
-		return nil
+		return
 	}
 
 	srcName := src.NameByte
@@ -376,25 +371,48 @@ func (fwd *sleeveForwarder) Forward(src *Peer, dst *Peer, frame []byte,
 	// drop will likely get re-transmitted we end up paying that cost
 	// multiple times. So it's better to drop things at the beginning
 	// of our pipeline.
-	if df {
-		if frameTooBig(frame, effectivePMTU) {
-			return FrameTooBigError{EPMTU: effectivePMTU}
+	if dec.DF() {
+		if !frameTooBig(frame, effectivePMTU) {
+			fwd.aggregate(fwd.aggregatorDFChan, srcName, dstName,
+				frame)
+			return
 		}
 
-		fwd.aggregate(fwd.aggregatorDFChan, srcName, dstName, frame)
-		return nil
+		// Why do we need an explicit broadcast hint here,
+		// rather than just checking the frame for a broadcast
+		// destination MAC address?  Because even
+		// non-broadcast frames can be broadcast, if the
+		// destination MAC was not in our MAC cache.
+		if broadcast {
+			log.Printf("dropping too big DF broadcast frame (%v -> %v): PMTU= %v\n", dec.IP.DstIP, dec.IP.SrcIP, effectivePMTU)
+			return
+		}
+
+		// Send an ICMP back to where the frame came from
+		fn, err := dec.makeICMPFragNeeded(effectivePMTU)
+		if err != nil {
+			log.Println(err)
+			return
+		}
+
+		dec.DecodeLayers(fn)
+
+		// The frag-needed packet does not have DF set, so the
+		// potential recursion here is bounded.
+		fwd.sleeve.consumer(dst, src, fn, dec)
+		return
 	}
 
-	if stackFrag || dec == nil || len(dec.decoded) < 2 {
+	if stackFrag || len(dec.decoded) < 2 {
 		fwd.aggregate(fwd.aggregatorChan, srcName, dstName, frame)
-		return nil
+		return
 	}
 
 	// Don't have trustworthy stack, so we're going to have to
 	// send it DF in any case.
 	if !frameTooBig(frame, effectivePMTU) {
 		fwd.aggregate(fwd.aggregatorDFChan, srcName, dstName, frame)
-		return nil
+		return
 	}
 
 	fwd.sleeve.logFrame("Fragmenting", frame, &dec.Eth)
@@ -402,11 +420,11 @@ func (fwd *sleeveForwarder) Forward(src *Peer, dst *Peer, frame []byte,
 	// We can't trust the stack to fragment, we have IP, and we
 	// have a frame that's too big for the MTU, so we have to
 	// fragment it ourself.
-	return fragment(dec.Eth, dec.IP, effectivePMTU,
+	checkWarn(fragment(dec.Eth, dec.IP, effectivePMTU,
 		func(segFrame []byte) {
 			fwd.aggregate(fwd.aggregatorDFChan, srcName, dstName,
 				segFrame)
-		})
+		}))
 }
 
 func (fwd *sleeveForwarder) aggregate(ch chan<- aggregatorFrame, src []byte,
@@ -417,14 +435,6 @@ func (fwd *sleeveForwarder) aggregate(ch chan<- aggregatorFrame, src []byte,
 	}
 }
 
-type FrameTooBigError struct {
-	EPMTU int // effective pmtu, i.e. what we tell packet senders
-}
-
-func (ftbe FrameTooBigError) Error() string {
-	return fmt.Sprint("Frame too big error. Effective PMTU is ", ftbe.EPMTU)
-}
-
 func fragment(eth layers.Ethernet, ip layers.IPv4, pmtu int,
 	forward func([]byte)) error {
 	// We are not doing any sort of NAT, so we don't need to worry