Move ICMP frag-needed handling into sleeveForwarder

router/connection.go

@@ -446,8 +446,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

@@ -29,7 +29,7 @@ func (dec *EthernetDecoder) DecodeLayers(data []byte) {
 	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,
@@ -57,11 +57,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", dec.IP.DstIP, dec.IP.SrcIP, mtu)
-	return sendFrame(buf.Bytes())
+	return buf.Bytes(), nil
 }
 
 var (

router/local_peer.go

@@ -27,29 +27,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) {
@@ -58,15 +58,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.Warningf("dropping too big DF broadcast frame (%v -> %v): PMTU= %v", dec.IP.DstIP, dec.IP.SrcIP, ftbe.EPMTU)
-			} else {
-				log.Errorln(err)
-			}
-		}
+		conn.(*LocalConnection).Forward(srcPeer, conn.Remote(), frame,
+			dec, true)
 	}
 }
 

router/overlay.go

@@ -61,7 +61,8 @@ type OverlayForwarder 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)
 
 	Stop()
 
@@ -87,8 +88,8 @@ func (NullOverlay) MakeForwarder(ForwarderParams) (OverlayForwarder, error) {
 func (NullOverlay) SetListener(OverlayForwarderListener) {
 }
 
-func (NullOverlay) Forward(*Peer, *Peer, []byte, *EthernetDecoder) error {
-	return nil
+func (NullOverlay) Forward(*Peer, *Peer, []byte, *EthernetDecoder,
+	bool) {
 }
 
 func (NullOverlay) Stop() {

router/router.go

@@ -138,11 +138,7 @@ func (router *Router) handleCapturedPacket(frameData []byte, dec *EthernetDecode
 		return
 	}
 
-	err := router.Ourself.Forward(dstPeer, frameCopy, dec)
-	if ftbe, ok := err.(FrameTooBigError); ok {
-		err = dec.sendICMPFragNeeded(ftbe.EPMTU, router.Bridge.InjectPacket)
-	}
-	checkWarn(err)
+	router.Ourself.Forward(dstPeer, frameCopy, dec)
 }
 
 func (router *Router) listenTCP(localPort int) {
@@ -179,14 +175,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

@@ -390,7 +390,7 @@ func (fwd *sleeveForwarder) SetListener(listener OverlayForwarderListener) {
 }
 
 func (fwd *sleeveForwarder) Forward(src *Peer, dst *Peer, frame []byte,
-	dec *EthernetDecoder) error {
+	dec *EthernetDecoder, broadcast bool) {
 	fwd.lock.RLock()
 	haveContact := (fwd.remoteAddr != nil)
 	mtu := fwd.mtu
@@ -400,7 +400,7 @@ func (fwd *sleeveForwarder) Forward(src *Peer, dst *Peer, frame []byte,
 	if !haveContact {
 		log.Print(fwd.logPrefix(),
 			"Cannot forward frame yet - awaiting contact")
-		return nil
+		return
 	}
 
 	srcName := src.NameByte
@@ -417,34 +417,57 @@ func (fwd *sleeveForwarder) Forward(src *Peer, dst *Peer, frame []byte,
 	// multiple times. So it's better to drop things at the beginning
 	// of our pipeline.
 	if dec.DF() {
-		if frameTooBig(frame, mtu) {
-			return FrameTooBigError{EPMTU: mtu}
+		if !frameTooBig(frame, mtu) {
+			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.Print(fwd.logPrefix(), "dropping too big DF broadcast frame (", dec.IP.DstIP, " -> ", dec.IP.SrcIP, "): MTU=", mtu)
+			return
+		}
+
+		// Send an ICMP back to where the frame came from
+		fragNeededPacket, err := dec.makeICMPFragNeeded(mtu)
+		if err != nil {
+			log.Print(fwd.logPrefix(), err)
+			return
+		}
+
+		dec.DecodeLayers(fragNeededPacket)
+
+		// The frag-needed packet does not have DF set, so the
+		// potential recursion here is bounded.
+		fwd.sleeve.consumer(dst, src, fragNeededPacket, 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, mtu) {
 		fwd.aggregate(fwd.aggregatorDFChan, srcName, dstName, frame)
-		return nil
+		return
 	}
 
 	// 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, mtu,
+	checkWarn(fragment(dec.Eth, dec.IP, mtu,
 		func(segFrame []byte) {
 			fwd.aggregate(fwd.aggregatorDFChan, srcName, dstName,
 				segFrame)
-		})
+		}))
 }
 
 func (fwd *sleeveForwarder) aggregate(ch chan<- aggregatorFrame, src []byte,
@@ -455,14 +478,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, mtu int,
 	forward func([]byte)) error {
 	// We are not doing any sort of NAT, so we don't need to worry