From d0c0549e607699e0186065ad9186431f12260487 Mon Sep 17 00:00:00 2001 From: Sam Balana Date: Tue, 9 Feb 2021 11:45:01 -0800 Subject: [PATCH] Deprecate Failed state in favor of Unreachable state ... as per RFC 7048. The Failed state is an internal state that is not specified by any RFC; replacing it with the Unreachable state enables us to expose this state while keeping our terminology consistent with RFC 4861 and RFC 7048. Unreachable state replaces all internal references for Failed state. However unlike the Failed state, change events are dispatched when moving into Unreachable state. This gives developers insight into whether a neighbor entry failed address resolution or whether it was explicitly removed. The Failed state will be removed entirely once all references to it are removed. This is done to avoid a Fuchsia roll failure. Updates #4667 PiperOrigin-RevId: 356554104 --- pkg/tcpip/stack/neighbor_cache.go | 10 +- pkg/tcpip/stack/neighbor_cache_test.go | 3 +- pkg/tcpip/stack/neighbor_entry.go | 57 +++++-- pkg/tcpip/stack/neighbor_entry_test.go | 196 ++++++++++++++++++------ pkg/tcpip/stack/neighborstate_string.go | 7 +- 5 files changed, 197 insertions(+), 76 deletions(-) diff --git a/pkg/tcpip/stack/neighbor_cache.go b/pkg/tcpip/stack/neighbor_cache.go index a77fe575a..533287c4c 100644 --- a/pkg/tcpip/stack/neighbor_cache.go +++ b/pkg/tcpip/stack/neighbor_cache.go @@ -25,9 +25,13 @@ const neighborCacheSize = 512 // max entries per interface // NeighborStats holds metrics for the neighbor table. type NeighborStats struct { - // FailedEntryLookups counts the number of lookups performed on an entry in - // Failed state. + // FailedEntryLookups is deprecated; UnreachableEntryLookups should be used + // instead. FailedEntryLookups *tcpip.StatCounter + + // UnreachableEntryLookups counts the number of lookups performed on an + // entry in Unreachable state. + UnreachableEntryLookups *tcpip.StatCounter } // neighborCache maps IP addresses to link addresses. It uses the Least @@ -143,7 +147,7 @@ func (n *neighborCache) entry(remoteAddr, localAddr tcpip.Address, onResolve fun onResolve(LinkResolutionResult{LinkAddress: entry.mu.neigh.LinkAddr, Success: true}) } return entry.mu.neigh, nil, nil - case Unknown, Incomplete, Failed: + case Unknown, Incomplete, Unreachable: if onResolve != nil { entry.mu.onResolve = append(entry.mu.onResolve, onResolve) } diff --git a/pkg/tcpip/stack/neighbor_cache_test.go b/pkg/tcpip/stack/neighbor_cache_test.go index d60a49fe8..909912662 100644 --- a/pkg/tcpip/stack/neighbor_cache_test.go +++ b/pkg/tcpip/stack/neighbor_cache_test.go @@ -1610,12 +1610,11 @@ func TestNeighborCacheRetryResolution(t *testing.T) { } } - // Verify the entry is in Failed state. wantEntries := []NeighborEntry{ { Addr: entry.Addr, LinkAddr: "", - State: Failed, + State: Unreachable, }, } if diff := cmp.Diff(linkRes.neigh.entries(), wantEntries, entryDiffOptsWithSort()...); diff != "" { diff --git a/pkg/tcpip/stack/neighbor_entry.go b/pkg/tcpip/stack/neighbor_entry.go index 4ed149ee8..03fef52ee 100644 --- a/pkg/tcpip/stack/neighbor_entry.go +++ b/pkg/tcpip/stack/neighbor_entry.go @@ -38,7 +38,8 @@ type NeighborEntry struct { } // NeighborState defines the state of a NeighborEntry within the Neighbor -// Unreachability Detection state machine, as per RFC 4861 section 7.3.2. +// Unreachability Detection state machine, as per RFC 4861 section 7.3.2 and +// RFC 7048. type NeighborState uint8 const ( @@ -61,13 +62,24 @@ const ( Delay // Probe means a reachability confirmation is actively being sought by // periodically retransmitting reachability probes until a reachability - // confirmation is received, or until the max amount of probes has been sent. + // confirmation is received, or until the maximum number of probes has been + // sent. Probe // Static describes entries that have been explicitly added by the user. They // do not expire and are not deleted until explicitly removed. Static - // Failed means recent attempts of reachability have returned inconclusive. + // Failed is deprecated and should no longer be used. + // + // TODO(gvisor.dev/issue/4667): Remove this once all references to Failed + // are removed from Fuchsia. Failed + // Unreachable means reachability confirmation failed; the maximum number of + // reachability probes has been sent and no replies have been received. + // + // TODO(gvisor.dev/issue/5472): Add the following sentence when we implement + // RFC 7048: "Packets continue to be sent to the neighbor while + // re-attempting to resolve the address." + Unreachable ) type timer struct { @@ -310,8 +322,8 @@ func (e *neighborEntry) setStateLocked(next NeighborState) { } if timedoutResolution || err != nil { - e.dispatchRemoveEventLocked() - e.setStateLocked(Failed) + e.setStateLocked(Unreachable) + e.dispatchChangeEventLocked() return } @@ -320,7 +332,7 @@ func (e *neighborEntry) setStateLocked(next NeighborState) { }), } - case Failed: + case Unreachable: e.notifyCompletionLocked(false /* succeeded */) case Unknown, Stale, Static: @@ -339,15 +351,19 @@ func (e *neighborEntry) setStateLocked(next NeighborState) { // Precondition: e.mu MUST be locked. func (e *neighborEntry) handlePacketQueuedLocked(localAddr tcpip.Address) { switch e.mu.neigh.State { - case Failed: - e.cache.nic.stats.Neighbor.FailedEntryLookups.Increment() - - fallthrough - case Unknown: + case Unknown, Unreachable: + prev := e.mu.neigh.State e.mu.neigh.State = Incomplete e.mu.neigh.UpdatedAtNanos = e.cache.nic.stack.clock.NowNanoseconds() - e.dispatchAddEventLocked() + switch prev { + case Unknown: + e.dispatchAddEventLocked() + case Unreachable: + e.dispatchChangeEventLocked() + e.cache.nic.stats.Neighbor.UnreachableEntryLookups.Increment() + } + config := e.nudState.Config() // Protected by e.mu. @@ -385,8 +401,8 @@ func (e *neighborEntry) handlePacketQueuedLocked(localAddr tcpip.Address) { } if timedoutResolution || err != nil { - e.dispatchRemoveEventLocked() - e.setStateLocked(Failed) + e.setStateLocked(Unreachable) + e.dispatchChangeEventLocked() return } @@ -418,7 +434,7 @@ func (e *neighborEntry) handleProbeLocked(remoteLinkAddr tcpip.LinkAddress) { // checks MUST be done by the NetworkEndpoint. switch e.mu.neigh.State { - case Unknown, Failed: + case Unknown: e.mu.neigh.LinkAddr = remoteLinkAddr e.setStateLocked(Stale) e.dispatchAddEventLocked() @@ -447,6 +463,13 @@ func (e *neighborEntry) handleProbeLocked(remoteLinkAddr tcpip.LinkAddress) { e.dispatchChangeEventLocked() } + case Unreachable: + // TODO(gvisor.dev/issue/5472): Do not change the entry if the link + // address is the same, as per RFC 7048. + e.mu.neigh.LinkAddr = remoteLinkAddr + e.setStateLocked(Stale) + e.dispatchChangeEventLocked() + case Static: // Do nothing @@ -549,7 +572,7 @@ func (e *neighborEntry) handleConfirmationLocked(linkAddr tcpip.LinkAddress, fla } e.mu.isRouter = flags.IsRouter - case Unknown, Failed, Static: + case Unknown, Unreachable, Static: // Do nothing default: @@ -571,7 +594,7 @@ func (e *neighborEntry) handleUpperLevelConfirmationLocked() { e.dispatchChangeEventLocked() } - case Unknown, Incomplete, Failed, Static: + case Unknown, Incomplete, Unreachable, Static: // Do nothing default: diff --git a/pkg/tcpip/stack/neighbor_entry_test.go b/pkg/tcpip/stack/neighbor_entry_test.go index f80d4ef8a..47a9e2448 100644 --- a/pkg/tcpip/stack/neighbor_entry_test.go +++ b/pkg/tcpip/stack/neighbor_entry_test.go @@ -70,38 +70,39 @@ func eventDiffOptsWithSort() []cmp.Option { } // The following unit tests exercise every state transition and verify its -// behavior with RFC 4681. +// behavior with RFC 4681 and RFC 7048. // -// | From | To | Cause | Update | Action | Event | -// | ========== | ========== | ========================================== | ======== | ===========| ======= | -// | Unknown | Unknown | Confirmation w/ unknown address | | | Added | -// | Unknown | Incomplete | Packet queued to unknown address | | Send probe | Added | -// | Unknown | Stale | Probe | | | Added | -// | Incomplete | Incomplete | Retransmit timer expired | | Send probe | Changed | -// | Incomplete | Reachable | Solicited confirmation | LinkAddr | Notify | Changed | -// | Incomplete | Stale | Unsolicited confirmation | LinkAddr | Notify | Changed | -// | Incomplete | Stale | Probe | LinkAddr | Notify | Changed | -// | Incomplete | Failed | Max probes sent without reply | | Notify | Removed | -// | Reachable | Reachable | Confirmation w/ different isRouter flag | IsRouter | | | -// | Reachable | Stale | Reachable timer expired | | | Changed | -// | Reachable | Stale | Probe or confirmation w/ different address | | | Changed | -// | Stale | Reachable | Solicited override confirmation | LinkAddr | | Changed | -// | Stale | Reachable | Solicited confirmation w/o address | | Notify | Changed | -// | Stale | Stale | Override confirmation | LinkAddr | | Changed | -// | Stale | Stale | Probe w/ different address | LinkAddr | | Changed | -// | Stale | Delay | Packet sent | | | Changed | -// | Delay | Reachable | Upper-layer confirmation | | | Changed | -// | Delay | Reachable | Solicited override confirmation | LinkAddr | | Changed | -// | Delay | Reachable | Solicited confirmation w/o address | | Notify | Changed | -// | Delay | Stale | Probe or confirmation w/ different address | | | Changed | -// | Delay | Probe | Delay timer expired | | Send probe | Changed | -// | Probe | Reachable | Solicited override confirmation | LinkAddr | | Changed | -// | Probe | Reachable | Solicited confirmation w/ same address | | Notify | Changed | -// | Probe | Reachable | Solicited confirmation w/o address | | Notify | Changed | -// | Probe | Stale | Probe or confirmation w/ different address | | | Changed | -// | Probe | Probe | Retransmit timer expired | | | Changed | -// | Probe | Failed | Max probes sent without reply | | Notify | Removed | -// | Failed | Incomplete | Packet queued | | Send probe | Added | +// | From | To | Cause | Update | Action | Event | +// | =========== | =========== | ========================================== | ======== | ===========| ======= | +// | Unknown | Unknown | Confirmation w/ unknown address | | | Added | +// | Unknown | Incomplete | Packet queued to unknown address | | Send probe | Added | +// | Unknown | Stale | Probe | | | Added | +// | Incomplete | Incomplete | Retransmit timer expired | | Send probe | Changed | +// | Incomplete | Reachable | Solicited confirmation | LinkAddr | Notify | Changed | +// | Incomplete | Stale | Unsolicited confirmation | LinkAddr | Notify | Changed | +// | Incomplete | Stale | Probe | LinkAddr | Notify | Changed | +// | Incomplete | Unreachable | Max probes sent without reply | | Notify | Changed | +// | Reachable | Reachable | Confirmation w/ different isRouter flag | IsRouter | | | +// | Reachable | Stale | Reachable timer expired | | | Changed | +// | Reachable | Stale | Probe or confirmation w/ different address | | | Changed | +// | Stale | Reachable | Solicited override confirmation | LinkAddr | | Changed | +// | Stale | Reachable | Solicited confirmation w/o address | | Notify | Changed | +// | Stale | Stale | Override confirmation | LinkAddr | | Changed | +// | Stale | Stale | Probe w/ different address | LinkAddr | | Changed | +// | Stale | Delay | Packet sent | | | Changed | +// | Delay | Reachable | Upper-layer confirmation | | | Changed | +// | Delay | Reachable | Solicited override confirmation | LinkAddr | | Changed | +// | Delay | Reachable | Solicited confirmation w/o address | | Notify | Changed | +// | Delay | Stale | Probe or confirmation w/ different address | | | Changed | +// | Delay | Probe | Delay timer expired | | Send probe | Changed | +// | Probe | Reachable | Solicited override confirmation | LinkAddr | | Changed | +// | Probe | Reachable | Solicited confirmation w/ same address | | Notify | Changed | +// | Probe | Reachable | Solicited confirmation w/o address | | Notify | Changed | +// | Probe | Stale | Probe or confirmation w/ different address | | | Changed | +// | Probe | Probe | Retransmit timer expired | | | Changed | +// | Probe | Unreachable | Max probes sent without reply | | Notify | Changed | +// | Unreachable | Incomplete | Packet queued | | Send probe | Changed | +// | Unreachable | Stale | Probe w/ different address | LinkAddr | | Changed | type testEntryEventType uint8 @@ -448,7 +449,7 @@ func TestEntryIncompleteToIncompleteDoesNotChangeUpdatedAt(t *testing.T) { clock.Advance(c.RetransmitTimer) // UpdatedAt should change after failing address resolution. Timing out after - // sending the last probe transitions the entry to Failed. + // sending the last probe transitions the entry to Unreachable. { wantProbes := []entryTestProbeInfo{ { @@ -478,12 +479,12 @@ func TestEntryIncompleteToIncompleteDoesNotChangeUpdatedAt(t *testing.T) { }, }, { - EventType: entryTestRemoved, + EventType: entryTestChanged, NICID: entryTestNICID, Entry: NeighborEntry{ Addr: entryTestAddr1, LinkAddr: tcpip.LinkAddress(""), - State: Incomplete, + State: Unreachable, }, }, } @@ -755,7 +756,7 @@ func TestEntryIncompleteToStaleWhenProbe(t *testing.T) { nudDisp.mu.Unlock() } -func TestEntryIncompleteToFailed(t *testing.T) { +func TestEntryIncompleteToUnreachable(t *testing.T) { c := DefaultNUDConfigurations() c.MaxMulticastProbes = 3 e, nudDisp, linkRes, clock := entryTestSetup(c) @@ -807,12 +808,12 @@ func TestEntryIncompleteToFailed(t *testing.T) { }, }, { - EventType: entryTestRemoved, + EventType: entryTestChanged, NICID: entryTestNICID, Entry: NeighborEntry{ Addr: entryTestAddr1, LinkAddr: tcpip.LinkAddress(""), - State: Incomplete, + State: Unreachable, }, }, } @@ -823,8 +824,8 @@ func TestEntryIncompleteToFailed(t *testing.T) { nudDisp.mu.Unlock() e.mu.Lock() - if e.mu.neigh.State != Failed { - t.Errorf("got e.mu.neigh.State = %q, want = %q", e.mu.neigh.State, Failed) + if e.mu.neigh.State != Unreachable { + t.Errorf("got e.mu.neigh.State = %q, want = %q", e.mu.neigh.State, Unreachable) } e.mu.Unlock() } @@ -3294,7 +3295,7 @@ func TestEntryProbeToReachableWhenSolicitedConfirmationWithoutAddress(t *testing nudDisp.mu.Unlock() } -func TestEntryProbeToFailed(t *testing.T) { +func TestEntryProbeToUnreachable(t *testing.T) { c := DefaultNUDConfigurations() c.MaxMulticastProbes = 3 c.MaxUnicastProbes = 3 @@ -3355,11 +3356,11 @@ func TestEntryProbeToFailed(t *testing.T) { e.mu.Unlock() } - // Wait for the last probe to expire, causing a transition to Failed. + // Wait for the last probe to expire, causing a transition to Unreachable. clock.Advance(c.RetransmitTimer) e.mu.Lock() - if e.mu.neigh.State != Failed { - t.Errorf("got e.mu.neigh.State = %q, want = %q", e.mu.neigh.State, Failed) + if e.mu.neigh.State != Unreachable { + t.Errorf("got e.mu.neigh.State = %q, want = %q", e.mu.neigh.State, Unreachable) } e.mu.Unlock() @@ -3401,12 +3402,12 @@ func TestEntryProbeToFailed(t *testing.T) { }, }, { - EventType: entryTestRemoved, + EventType: entryTestChanged, NICID: entryTestNICID, Entry: NeighborEntry{ Addr: entryTestAddr1, LinkAddr: entryTestLinkAddr1, - State: Probe, + State: Unreachable, }, }, } @@ -3417,7 +3418,7 @@ func TestEntryProbeToFailed(t *testing.T) { nudDisp.mu.Unlock() } -func TestEntryFailedToIncomplete(t *testing.T) { +func TestEntryUnreachableToIncomplete(t *testing.T) { c := DefaultNUDConfigurations() c.MaxMulticastProbes = 3 e, nudDisp, linkRes, clock := entryTestSetup(c) @@ -3461,8 +3462,8 @@ func TestEntryFailedToIncomplete(t *testing.T) { } e.mu.Lock() - if e.mu.neigh.State != Failed { - t.Errorf("got e.mu.neigh.State = %q, want = %q", e.mu.neigh.State, Failed) + if e.mu.neigh.State != Unreachable { + t.Errorf("got e.mu.neigh.State = %q, want = %q", e.mu.neigh.State, Unreachable) } e.mu.Unlock() @@ -3484,16 +3485,16 @@ func TestEntryFailedToIncomplete(t *testing.T) { }, }, { - EventType: entryTestRemoved, + EventType: entryTestChanged, NICID: entryTestNICID, Entry: NeighborEntry{ Addr: entryTestAddr1, LinkAddr: tcpip.LinkAddress(""), - State: Incomplete, + State: Unreachable, }, }, { - EventType: entryTestAdded, + EventType: entryTestChanged, NICID: entryTestNICID, Entry: NeighborEntry{ Addr: entryTestAddr1, @@ -3508,3 +3509,96 @@ func TestEntryFailedToIncomplete(t *testing.T) { } nudDisp.mu.Unlock() } + +func TestEntryUnreachableToStale(t *testing.T) { + wantProbes := []entryTestProbeInfo{ + // The Incomplete-to-Incomplete state transition is tested here by + // verifying that 3 reachability probes were sent. + { + RemoteAddress: entryTestAddr1, + RemoteLinkAddress: tcpip.LinkAddress(""), + LocalAddress: entryTestAddr2, + }, + { + RemoteAddress: entryTestAddr1, + RemoteLinkAddress: tcpip.LinkAddress(""), + LocalAddress: entryTestAddr2, + }, + { + RemoteAddress: entryTestAddr1, + RemoteLinkAddress: tcpip.LinkAddress(""), + LocalAddress: entryTestAddr2, + }, + } + + c := DefaultNUDConfigurations() + c.MaxMulticastProbes = uint32(len(wantProbes)) + e, nudDisp, linkRes, clock := entryTestSetup(c) + + // TODO(gvisor.dev/issue/4872): Use helper functions to start entry tests in + // their expected state. + e.mu.Lock() + e.handlePacketQueuedLocked(entryTestAddr2) + if e.mu.neigh.State != Incomplete { + t.Errorf("got e.mu.neigh.State = %q, want = %q", e.mu.neigh.State, Incomplete) + } + e.mu.Unlock() + + waitFor := c.RetransmitTimer * time.Duration(c.MaxMulticastProbes) + clock.Advance(waitFor) + + linkRes.mu.Lock() + diff := cmp.Diff(wantProbes, linkRes.probes) + linkRes.mu.Unlock() + if diff != "" { + t.Fatalf("link address resolver probes mismatch (-want, +got):\n%s", diff) + } + + e.mu.Lock() + if e.mu.neigh.State != Unreachable { + t.Errorf("got e.mu.neigh.State = %q, want = %q", e.mu.neigh.State, Unreachable) + } + e.mu.Unlock() + + e.mu.Lock() + e.handleProbeLocked(entryTestLinkAddr2) + if e.mu.neigh.State != Stale { + t.Errorf("got e.mu.neigh.State = %q, want = %q", e.mu.neigh.State, Stale) + } + e.mu.Unlock() + + wantEvents := []testEntryEventInfo{ + { + EventType: entryTestAdded, + NICID: entryTestNICID, + Entry: NeighborEntry{ + Addr: entryTestAddr1, + LinkAddr: tcpip.LinkAddress(""), + State: Incomplete, + }, + }, + { + EventType: entryTestChanged, + NICID: entryTestNICID, + Entry: NeighborEntry{ + Addr: entryTestAddr1, + LinkAddr: tcpip.LinkAddress(""), + State: Unreachable, + }, + }, + { + EventType: entryTestChanged, + NICID: entryTestNICID, + Entry: NeighborEntry{ + Addr: entryTestAddr1, + LinkAddr: entryTestLinkAddr2, + State: Stale, + }, + }, + } + nudDisp.mu.Lock() + if diff := cmp.Diff(wantEvents, nudDisp.events, eventDiffOpts()...); diff != "" { + t.Errorf("nud dispatcher events mismatch (-want, +got):\n%s", diff) + } + nudDisp.mu.Unlock() +} diff --git a/pkg/tcpip/stack/neighborstate_string.go b/pkg/tcpip/stack/neighborstate_string.go index aa7311ec6..765df4d7a 100644 --- a/pkg/tcpip/stack/neighborstate_string.go +++ b/pkg/tcpip/stack/neighborstate_string.go @@ -1,4 +1,4 @@ -// Copyright 2020 The gVisor Authors. +// Copyright 2021 The gVisor Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -30,11 +30,12 @@ func _() { _ = x[Probe-5] _ = x[Static-6] _ = x[Failed-7] + _ = x[Unreachable-8] } -const _NeighborState_name = "UnknownIncompleteReachableStaleDelayProbeStaticFailed" +const _NeighborState_name = "UnknownIncompleteReachableStaleDelayProbeStaticFailedUnreachable" -var _NeighborState_index = [...]uint8{0, 7, 17, 26, 31, 36, 41, 47, 53} +var _NeighborState_index = [...]uint8{0, 7, 17, 26, 31, 36, 41, 47, 53, 64} func (i NeighborState) String() string { if i >= NeighborState(len(_NeighborState_index)-1) {