Invoke address resolution upon subsequent traffic to Failed neighbor

Removes the period of time in which subseqeuent traffic to a Failed neighbor
immediately fails with ErrNoLinkAddress. A Failed neighbor is one in which
address resolution fails; or in other words, the neighbor's IP address cannot
be translated to a MAC address.

This means removing the Failed state for linkAddrCache and allowing transitiong
out of Failed into Incomplete for neighborCache. Previously, both caches would
transition entries to Failed after address resolution fails. In this state, any
subsequent traffic requested within an unreachable time would immediately fail
with ErrNoLinkAddress. This does not follow RFC 4861 section 7.3.3:

  If address resolution fails, the entry SHOULD be deleted, so that subsequent
  traffic to that neighbor invokes the next-hop determination procedure again.
  Invoking next-hop determination at this point ensures that alternate default
  routers are tried.

The API for getting a link address for a given address, whether through the link
address cache or the neighbor table, is updated to optionally take a callback
which will be called when address resolution completes. This allows `Route` to
handle completing link resolution internally, so callers of (*Route).Resolve
(e.g. endpoints) don’t have to keep track of when it completes and update the
Route accordingly.

This change also removes the wakers from LinkAddressCache, NeighborCache, and
Route in favor of the callbacks, and callers that previously used a waker can
now just pass a callback to (*Route).Resolve that will notify the waker on
resolution completion.

Fixes #4796

Startblock:
  has LGTM from sbalana
  and then
  add reviewer ghanan
PiperOrigin-RevId: 348597478

1 files changed, 25 insertions, 85 deletions

diff --git a/pkg/tcpip/stack/linkaddrcache_test.go b/pkg/tcpip/stack/linkaddrcache_test.go
index d2e37f38d..6883045b5 100644
--- a/pkg/tcpip/stack/linkaddrcache_test.go
+++ b/pkg/tcpip/stack/linkaddrcache_test.go
@@ -21,7 +21,6 @@ import (
 	"testing"
 	"time"
 
-	"gvisor.dev/gvisor/pkg/sleep"
 	"gvisor.dev/gvisor/pkg/sync"
 	"gvisor.dev/gvisor/pkg/tcpip"
 )
@@ -50,6 +49,7 @@ type testLinkAddressResolver struct {
 }
 
 func (r *testLinkAddressResolver) LinkAddressRequest(targetAddr, _ tcpip.Address, _ tcpip.LinkAddress, _ NetworkInterface) *tcpip.Error {
+	// TODO(gvisor.dev/issue/5141): Use a fake clock.
 	time.AfterFunc(r.delay, func() { r.fakeRequest(targetAddr) })
 	if f := r.onLinkAddressRequest; f != nil {
 		f()
@@ -78,16 +78,18 @@ func (*testLinkAddressResolver) LinkAddressProtocol() tcpip.NetworkProtocolNumbe
 }
 
 func getBlocking(c *linkAddrCache, addr tcpip.FullAddress, linkRes LinkAddressResolver) (tcpip.LinkAddress, *tcpip.Error) {
-	w := sleep.Waker{}
-	s := sleep.Sleeper{}
-	s.AddWaker(&w, 123)
-	defer s.Done()
-
+	var attemptedResolution bool
 	for {
-		if got, _, err := c.get(addr, linkRes, "", nil, &w); err != tcpip.ErrWouldBlock {
-			return got, err
+		got, ch, err := c.get(addr, linkRes, "", nil, nil)
+		if err == tcpip.ErrWouldBlock {
+			if attemptedResolution {
+				return got, tcpip.ErrNoLinkAddress
+			}
+			attemptedResolution = true
+			<-ch
+			continue
 		}
-		s.Fetch(true)
+		return got, err
 	}
 }
 
@@ -116,16 +118,19 @@ func TestCacheOverflow(t *testing.T) {
 		}
 	}
 	// The earliest entries should no longer be in the cache.
+	c.cache.Lock()
+	defer c.cache.Unlock()
 	for i := len(testAddrs) - 1; i >= len(testAddrs)-linkAddrCacheSize; i-- {
 		e := testAddrs[i]
-		if _, _, err := c.get(e.addr, nil, "", nil, nil); err != tcpip.ErrNoLinkAddress {
-			t.Errorf("check %d, c.get(%q), got error: %v, want: error ErrNoLinkAddress", i, string(e.addr.Addr), err)
+		if entry, ok := c.cache.table[e.addr]; ok {
+			t.Errorf("unexpected entry at c.cache.table[%q]: %#v", string(e.addr.Addr), entry)
 		}
 	}
 }
 
 func TestCacheConcurrent(t *testing.T) {
 	c := newLinkAddrCache(1<<63-1, 1*time.Second, 3)
+	linkRes := &testLinkAddressResolver{cache: c}
 
 	var wg sync.WaitGroup
 	for r := 0; r < 16; r++ {
@@ -133,7 +138,6 @@ func TestCacheConcurrent(t *testing.T) {
 		go func() {
 			for _, e := range testAddrs {
 				c.add(e.addr, e.linkAddr)
-				c.get(e.addr, nil, "", nil, nil) // make work for gotsan
 			}
 			wg.Done()
 		}()
@@ -144,7 +148,7 @@ func TestCacheConcurrent(t *testing.T) {
 	// can fit in the cache, so our eviction strategy requires that
 	// the last entry be present and the first be missing.
 	e := testAddrs[len(testAddrs)-1]
-	got, _, err := c.get(e.addr, nil, "", nil, nil)
+	got, _, err := c.get(e.addr, linkRes, "", nil, nil)
 	if err != nil {
 		t.Errorf("c.get(%q)=%q, got error: %v", string(e.addr.Addr), got, err)
 	}
@@ -153,18 +157,22 @@ func TestCacheConcurrent(t *testing.T) {
 	}
 
 	e = testAddrs[0]
-	if _, _, err := c.get(e.addr, nil, "", nil, nil); err != tcpip.ErrNoLinkAddress {
-		t.Errorf("c.get(%q), got error: %v, want: error ErrNoLinkAddress", string(e.addr.Addr), err)
+	c.cache.Lock()
+	defer c.cache.Unlock()
+	if entry, ok := c.cache.table[e.addr]; ok {
+		t.Errorf("unexpected entry at c.cache.table[%q]: %#v", string(e.addr.Addr), entry)
 	}
 }
 
 func TestCacheAgeLimit(t *testing.T) {
 	c := newLinkAddrCache(1*time.Millisecond, 1*time.Second, 3)
+	linkRes := &testLinkAddressResolver{cache: c}
+
 	e := testAddrs[0]
 	c.add(e.addr, e.linkAddr)
 	time.Sleep(50 * time.Millisecond)
-	if _, _, err := c.get(e.addr, nil, "", nil, nil); err != tcpip.ErrNoLinkAddress {
-		t.Errorf("c.get(%q), got error: %v, want: error ErrNoLinkAddress", string(e.addr.Addr), err)
+	if _, _, err := c.get(e.addr, linkRes, "", nil, nil); err != tcpip.ErrWouldBlock {
+		t.Errorf("got c.get(%q) = %s, want = ErrWouldBlock", string(e.addr.Addr), err)
 	}
 }
 
@@ -282,71 +290,3 @@ func TestStaticResolution(t *testing.T) {
 		t.Errorf("c.get(%q)=%q, want %q", string(addr), string(got), string(want))
 	}
 }
-
-// TestCacheWaker verifies that RemoveWaker removes a waker previously added
-// through get().
-func TestCacheWaker(t *testing.T) {
-	c := newLinkAddrCache(1<<63-1, 1*time.Second, 3)
-
-	// First, sanity check that wakers are working.
-	{
-		linkRes := &testLinkAddressResolver{cache: c}
-		s := sleep.Sleeper{}
-		defer s.Done()
-
-		const wakerID = 1
-		w := sleep.Waker{}
-		s.AddWaker(&w, wakerID)
-
-		e := testAddrs[0]
-
-		if _, _, err := c.get(e.addr, linkRes, "", nil, &w); err != tcpip.ErrWouldBlock {
-			t.Fatalf("got c.get(%q, _, _, _, _) = %s, want = %s", e.addr.Addr, err, tcpip.ErrWouldBlock)
-		}
-		id, ok := s.Fetch(true /* block */)
-		if !ok {
-			t.Fatal("got s.Fetch(true) = (_, false), want = (_, true)")
-		}
-		if id != wakerID {
-			t.Fatalf("got s.Fetch(true) = (%d, %t), want = (%d, true)", id, ok, wakerID)
-		}
-
-		if got, _, err := c.get(e.addr, linkRes, "", nil, nil); err != nil {
-			t.Fatalf("c.get(%q, _, _, _, _): %s", e.addr.Addr, err)
-		} else if got != e.linkAddr {
-			t.Fatalf("got c.get(%q) = %q, want = %q", e.addr.Addr, got, e.linkAddr)
-		}
-	}
-
-	// Check that RemoveWaker works.
-	{
-		linkRes := &testLinkAddressResolver{cache: c}
-		s := sleep.Sleeper{}
-		defer s.Done()
-
-		const wakerID = 2 // different than the ID used in the sanity check
-		w := sleep.Waker{}
-		s.AddWaker(&w, wakerID)
-
-		e := testAddrs[1]
-		linkRes.onLinkAddressRequest = func() {
-			// Remove the waker before the linkAddrCache has the opportunity to send
-			// a notification.
-			c.removeWaker(e.addr, &w)
-		}
-
-		if _, _, err := c.get(e.addr, linkRes, "", nil, &w); err != tcpip.ErrWouldBlock {
-			t.Fatalf("got c.get(%q, _, _, _, _) = %s, want = %s", e.addr.Addr, err, tcpip.ErrWouldBlock)
-		}
-
-		if got, err := getBlocking(c, e.addr, linkRes); err != nil {
-			t.Fatalf("c.get(%q, _, _, _, _): %s", e.addr.Addr, err)
-		} else if got != e.linkAddr {
-			t.Fatalf("c.get(%q) = %q, want = %q", e.addr.Addr, got, e.linkAddr)
-		}
-
-		if id, ok := s.Fetch(false /* block */); ok {
-			t.Fatalf("unexpected notification from waker with id %d", id)
-		}
-	}
-}