Do not cache remote link address in Route

...unless explicitly requested via ResolveWith.

Remove cancelled channels from pending packets as we can use the link
resolution channel in a FIFO to limit the number of maximum pending
resolutions we should queue packets for.

This change also defers starting the goroutine that handles link
resolution completion to when link resolution succeeds, fails or
gets cancelled due to the max number of pending resolutions being
reached.

Fixes #751.

PiperOrigin-RevId: 353130577

1 files changed, 33 insertions, 34 deletions

diff --git a/pkg/tcpip/stack/route.go b/pkg/tcpip/stack/route.go
index 1ff7b3a37..093b676aa 100644
--- a/pkg/tcpip/stack/route.go
+++ b/pkg/tcpip/stack/route.go
@@ -86,12 +86,21 @@ type RouteInfo struct {
 	RemoteLinkAddress tcpip.LinkAddress
 }
 
-// Fields returns a RouteInfo with all of r's exported fields. This allows
-// callers to store the route's fields without retaining a reference to it.
+// Fields returns a RouteInfo with all of the known values for the route's
+// fields.
+//
+// If any fields are unknown (e.g. remote link address when it is waiting for
+// link address resolution), they will be unset.
 func (r *Route) Fields() RouteInfo {
+	r.mu.RLock()
+	defer r.mu.RUnlock()
+	return r.fieldsLocked()
+}
+
+func (r *Route) fieldsLocked() RouteInfo {
 	return RouteInfo{
 		routeInfo:         r.routeInfo,
-		RemoteLinkAddress: r.RemoteLinkAddress(),
+		RemoteLinkAddress: r.mu.remoteLinkAddress,
 	}
 }
 
@@ -306,29 +315,26 @@ func (r *Route) ResolveWith(addr tcpip.LinkAddress) {
 	r.mu.remoteLinkAddress = addr
 }
 
-// Resolve attempts to resolve the link address if necessary.
+// ResolvedFields is like Fields but also attempts to resolve the remote link
+// address if it is not yet known.
 //
-// Returns tcpip.ErrWouldBlock if address resolution requires blocking (e.g.
-// waiting for ARP reply). If address resolution is required, a notification
-// channel is also returned for the caller to block on. The channel is closed
+// If address resolution is required, returns tcpip.ErrWouldBlock and a
+// notification channel for the caller to block on. The channel will be readable
 // once address resolution is complete (successful or not). If a callback is
 // provided, it will be called when address resolution is complete, regardless
-// of success or failure.
-func (r *Route) Resolve(afterResolve func()) (<-chan struct{}, *tcpip.Error) {
-	r.mu.Lock()
-
-	if !r.isResolutionRequiredRLocked() {
-		// Nothing to do if there is no cache (which does the resolution on cache miss) or
-		// link address is already known.
-		r.mu.Unlock()
-		return nil, nil
+// of success or failure before the notification channel is readable.
+//
+// Note, the route will not cache the remote link address when address
+// resolution completes.
+func (r *Route) ResolvedFields(afterResolve func()) (RouteInfo, <-chan struct{}, *tcpip.Error) {
+	r.mu.RLock()
+	fields := r.fieldsLocked()
+	resolutionRequired := r.isResolutionRequiredRLocked()
+	r.mu.RUnlock()
+	if !resolutionRequired {
+		return fields, nil, nil
 	}
 
-	// Increment the route's reference count because finishResolution retains a
-	// reference to the route and releases it when called.
-	r.acquireLocked()
-	r.mu.Unlock()
-
 	nextAddr := r.NextHop
 	if nextAddr == "" {
 		nextAddr = r.RemoteAddress
@@ -341,18 +347,15 @@ func (r *Route) Resolve(afterResolve func()) (<-chan struct{}, *tcpip.Error) {
 		linkAddressResolutionRequestLocalAddr = r.LocalAddress
 	}
 
-	finishResolution := func(linkAddress tcpip.LinkAddress, ok bool) {
-		if ok {
-			r.ResolveWith(linkAddress)
-		}
+	linkAddr, ch, err := r.outgoingNIC.getNeighborLinkAddress(nextAddr, linkAddressResolutionRequestLocalAddr, r.linkRes, func(tcpip.LinkAddress, bool) {
 		if afterResolve != nil {
 			afterResolve()
 		}
-		r.Release()
+	})
+	if err == nil {
+		fields.RemoteLinkAddress = linkAddr
 	}
-
-	_, ch, err := r.outgoingNIC.getNeighborLinkAddress(nextAddr, linkAddressResolutionRequestLocalAddr, r.linkRes, finishResolution)
-	return ch, err
+	return fields, ch, err
 }
 
 // local returns true if the route is a local route.
@@ -371,11 +374,7 @@ func (r *Route) IsResolutionRequired() bool {
 }
 
 func (r *Route) isResolutionRequiredRLocked() bool {
-	if !r.isValidForOutgoingRLocked() || r.mu.remoteLinkAddress != "" || r.local() {
-		return false
-	}
-
-	return r.linkRes != nil
+	return len(r.mu.remoteLinkAddress) == 0 && r.linkRes != nil && r.isValidForOutgoingRLocked() && !r.local()
 }
 
 func (r *Route) isValidForOutgoing() bool {