From 07b1d7413e8b648b85fa9276a516732dd93276b4 Mon Sep 17 00:00:00 2001
From: Ghanan Gowripalan <ghanan@google.com>
Date: Thu, 8 Oct 2020 17:32:41 -0700
Subject: Only block resolution when NUD is incomplete

When a completed entry exists for a neighbor, there is no need to block
while reachability is (re)confirmed. The stack should continue to use
the neighbor's link address while NUD is performed.

Test: stack_test.TestNeighborCacheReplace
PiperOrigin-RevId: 336199043
---
 pkg/tcpip/stack/neighbor_cache.go      | 15 +++++---
 pkg/tcpip/stack/neighbor_cache_test.go | 66 +++++++++++++++++++---------------
 2 files changed, 47 insertions(+), 34 deletions(-)

(limited to 'pkg/tcpip')

diff --git a/pkg/tcpip/stack/neighbor_cache.go b/pkg/tcpip/stack/neighbor_cache.go
index 27e1feec0..4df288798 100644
--- a/pkg/tcpip/stack/neighbor_cache.go
+++ b/pkg/tcpip/stack/neighbor_cache.go
@@ -131,10 +131,17 @@ func (n *neighborCache) entry(remoteAddr, localAddr tcpip.Address, linkRes LinkA
 	defer entry.mu.Unlock()
 
 	switch s := entry.neigh.State; s {
-	case Reachable, Static:
+	case Stale:
+		entry.handlePacketQueuedLocked()
+		fallthrough
+	case Reachable, Static, Delay, Probe:
+		// As per RFC 4861 section 7.3.3:
+		//  "Neighbor Unreachability Detection operates in parallel with the sending
+		//   of packets to a neighbor. While reasserting a neighbor's reachability,
+		//   a node continues sending packets to that neighbor using the cached
+		//   link-layer address."
 		return entry.neigh, nil, nil
-
-	case Unknown, Incomplete, Stale, Delay, Probe:
+	case Unknown, Incomplete:
 		entry.addWakerLocked(w)
 
 		if entry.done == nil {
@@ -147,10 +154,8 @@ func (n *neighborCache) entry(remoteAddr, localAddr tcpip.Address, linkRes LinkA
 
 		entry.handlePacketQueuedLocked()
 		return entry.neigh, entry.done, tcpip.ErrWouldBlock
-
 	case Failed:
 		return entry.neigh, nil, tcpip.ErrNoLinkAddress
-
 	default:
 		panic(fmt.Sprintf("Invalid cache entry state: %s", s))
 	}
diff --git a/pkg/tcpip/stack/neighbor_cache_test.go b/pkg/tcpip/stack/neighbor_cache_test.go
index a0b7da5cd..fcd54ed83 100644
--- a/pkg/tcpip/stack/neighbor_cache_test.go
+++ b/pkg/tcpip/stack/neighbor_cache_test.go
@@ -1500,24 +1500,26 @@ func TestNeighborCacheReplace(t *testing.T) {
 	}
 
 	// Verify the entry exists
-	e, doneCh, err := neigh.entry(entry.Addr, entry.LocalAddr, linkRes, nil)
-	if err != nil {
-		t.Errorf("unexpected error from neigh.entry(%s, %s, _, nil): %s", entry.Addr, entry.LocalAddr, err)
-	}
-	if doneCh != nil {
-		t.Errorf("unexpected done channel from neigh.entry(%s, %s, _, nil): %v", entry.Addr, entry.LocalAddr, doneCh)
-	}
-	if t.Failed() {
-		t.FailNow()
-	}
-	want := NeighborEntry{
-		Addr:      entry.Addr,
-		LocalAddr: entry.LocalAddr,
-		LinkAddr:  entry.LinkAddr,
-		State:     Reachable,
-	}
-	if diff := cmp.Diff(e, want, entryDiffOpts()...); diff != "" {
-		t.Errorf("neigh.entry(%s, %s, _, nil) mismatch (-got, +want):\n%s", entry.Addr, entry.LinkAddr, diff)
+	{
+		e, doneCh, err := neigh.entry(entry.Addr, entry.LocalAddr, linkRes, nil)
+		if err != nil {
+			t.Errorf("unexpected error from neigh.entry(%s, %s, _, nil): %s", entry.Addr, entry.LocalAddr, err)
+		}
+		if doneCh != nil {
+			t.Errorf("unexpected done channel from neigh.entry(%s, %s, _, nil): %v", entry.Addr, entry.LocalAddr, doneCh)
+		}
+		if t.Failed() {
+			t.FailNow()
+		}
+		want := NeighborEntry{
+			Addr:      entry.Addr,
+			LocalAddr: entry.LocalAddr,
+			LinkAddr:  entry.LinkAddr,
+			State:     Reachable,
+		}
+		if diff := cmp.Diff(e, want, entryDiffOpts()...); diff != "" {
+			t.Errorf("neigh.entry(%s, %s, _, nil) mismatch (-got, +want):\n%s", entry.Addr, entry.LinkAddr, diff)
+		}
 	}
 
 	// Notify of a link address change
@@ -1536,28 +1538,34 @@ func TestNeighborCacheReplace(t *testing.T) {
 		IsRouter:  false,
 	})
 
-	// Requesting the entry again should start address resolution
+	// Requesting the entry again should start neighbor reachability confirmation.
+	//
+	// Verify the entry's new link address and the new state.
 	{
-		_, doneCh, err := neigh.entry(entry.Addr, entry.LocalAddr, linkRes, nil)
-		if err != tcpip.ErrWouldBlock {
-			t.Fatalf("got neigh.entry(%s, %s, _, nil) = %v, want = %s", entry.Addr, entry.LocalAddr, err, tcpip.ErrWouldBlock)
+		e, _, err := neigh.entry(entry.Addr, entry.LocalAddr, linkRes, nil)
+		if err != nil {
+			t.Fatalf("neigh.entry(%s, %s, _, nil): %s", entry.Addr, entry.LocalAddr, err)
 		}
-		clock.Advance(config.DelayFirstProbeTime + typicalLatency)
-		select {
-		case <-doneCh:
-		default:
-			t.Fatalf("expected notification from done channel returned by neigh.entry(%s, %s, _, nil)", entry.Addr, entry.LocalAddr)
+		want := NeighborEntry{
+			Addr:      entry.Addr,
+			LocalAddr: entry.LocalAddr,
+			LinkAddr:  updatedLinkAddr,
+			State:     Delay,
 		}
+		if diff := cmp.Diff(e, want, entryDiffOpts()...); diff != "" {
+			t.Errorf("neigh.entry(%s, %s, _, nil) mismatch (-got, +want):\n%s", entry.Addr, entry.LocalAddr, diff)
+		}
+		clock.Advance(config.DelayFirstProbeTime + typicalLatency)
 	}
 
-	// Verify the entry's new link address
+	// Verify that the neighbor is now reachable.
 	{
 		e, _, err := neigh.entry(entry.Addr, entry.LocalAddr, linkRes, nil)
 		clock.Advance(typicalLatency)
 		if err != nil {
 			t.Errorf("unexpected error from neigh.entry(%s, %s, _, nil): %s", entry.Addr, entry.LocalAddr, err)
 		}
-		want = NeighborEntry{
+		want := NeighborEntry{
 			Addr:      entry.Addr,
 			LocalAddr: entry.LocalAddr,
 			LinkAddr:  updatedLinkAddr,
-- 
cgit v1.2.3