icmpv6: end address resolution when an NS sets the entry STALE

[adamgeorge309]

A Neighbor Solicitation (NS) with a Source Link-Layer Address option that found an INCOMPLETE Neighbour Cache entry set it STALE, but left the address-resolution timer running and the queued packets waiting. The timer went on sending multicast NSs for a neighbour whose link-layer address was known, and then declared address resolution failed and deleted the entry. RFC 4861 Appendix C gives this transition as "Record link-layer address. Send queued packets." The NS now cancels the timer and sends the queued packets, as a Neighbor Advertisement (NA) that completes resolution already does.

Risk: runs in which an INCOMPLETE entry receives an NS from its neighbour, typically two nodes that resolve each other at the same moment, send fewer multicast NSs, keep their entries, and send the queued datagrams earlier. The fingerprints of three IPv6 rows of examples/manetrouting/gpsr (Greedy Perimeter Stateless Routing, GPSR) change and are recorded here; their statistical baselines need re-recording after the merge.

Closes #1336

One commit: icmpv6: fix: end address resolution when an NS sets the entry STALE.

The problem

initiateAddressResolution() schedules Neighbour::arTimer (src/inet/networklayer/icmpv6/Ipv6NeighbourDiscovery.cc:692-695 on master), and Neighbour Discovery cancels it on a state change only in processNaForIncompleteNceState() (lines 2262-2264), together with sending the queued packets. processNsWithSpecifiedSrcAddr() (lines 1957-1966) records the link-layer address of an existing entry and sets it STALE when the cached address differs, which is the case for an INCOMPLETE entry unless a Router Advertisement already recorded the same address, but does neither. The NA that answers the entry's own NS then finds it STALE; processNaForOtherNceStates() sends the queued packets but leaves the timer. processArTimeout() (lines 698-721) does not check the state: it sends another multicast NS while fewer than MAX_MULTICAST_SOLICIT have been sent, and then calls dropQueuedPacketsAwaitingAr(), which returns any queued packets as Internet Control Message Protocol for IPv6 (ICMPv6) Destination Unreachable and deletes the entry.

RFC 4861 Appendix C:

INCOMPLETE NS, RS, RA, Redirect Record link-layer STALE
address. Send queued
packets.

Section 7.2.2 retransmits "While awaiting a response", and Appendix C has a retransmit timeout only in INCOMPLETE and PROBE; it has none in STALE, DELAY or REACHABLE.

The fix

When the existing entry is INCOMPLETE, processNsWithSpecifiedSrcAddr() now records the address and sets it STALE even if a Router Advertisement already recorded the same address, and it cancels and deletes arTimer and calls sendQueuedPacketsToIpv6Module(), the two steps processNaForIncompleteNceState() takes. For entries in the other states the Section 7.2.3 rule is unchanged: STALE only when the address differs. The packets go back to Ipv6, where resolveNeighbour() finds the STALE entry, returns its link-layer address and starts the DELAY state, as Section 7.3.3 requires for a packet sent to a STALE neighbour.

Not changed: a Router Advertisement with a Source Link-Layer Address option records the address of an existing entry but leaves its state (lines 1474-1478), so an INCOMPLETE entry stays INCOMPLETE until an NA or an NS arrives; Redirect and Router Solicitation handling do not update the Neighbour Cache. No C++ interface, Network Description (NED) parameter, message field or feature descriptor changes.

Verification

Reproduction: two StandardHost6 on one 10 Mb/s link with Ipv6FlatNetworkConfigurator, each running a UdpBasicApp to the other (64 B every 1 s, stopping at 12 s, run to 20 s). The committed test, tests/module/IPv6_ND_solicitation_ends_address_resolution.test, uses the simultaneous start without a stop time and runs to 10 s; it fails on master and passes with this change.

Both hosts together	Master, start at 6 s and 6 s	This change, 6 s and 6 s	Master and this change, 6 s and 6.5 s (control)
Sending another Address Resolution NS message	8	0	0
Address Resolution has failed.	4	0	0
Initiating Address Resolution	4	2	1
datagrams received (a, b)	6, 6	6, 6	6, 5 (b's application opens its socket at 6.5 s, after a's first datagram)

With this change host a sends the datagram queued at 6 s when b's NS arrives (6.0000785 s) instead of when b's NA does (6.0001665 s on master); the entry goes to DELAY, and the NA at 6.0002865 s sets it REACHABLE and cancels the DELAY timer. On master, a deletes its entry for b at 9 s and at 12 s; after 9 s it resolves b again.

examples/manetrouting/gpsr, run for 20 s as in the fingerprint test, with **.neighbourDiscovery.cmdenv-log-level = detail:

Configuration	Address resolutions declared failed	Multicast NS retransmissions	Queued packets returned for an ICMPv6 error	Ping replies received (host[0])
IPv6, master	17	59	21	7 of 17
IPv6, this change	9	39	16	9 of 17
DynamicIPv6, master	15	40	29	0 of 11
DynamicIPv6, this change	10	27	17	1 of 11

Release build (make MODE=release in the root and tests/module/lib), each suite run on master 41eee1d619 and with this change:

Command	Master	This change
tests/fingerprint: ./fingerprinttest -s -F tyf	1774 run, 5 failures, 62 errors, 1 expected error	the same failing and erroring rows, with the same computed fingerprints for the 5 failures; the gpsr rows IPv6, MultiIPv6 and DynamicIPv6 pass with the fingerprints this commit records, and master computes the previous ones (for example e008-a90a/tplx for IPv6); ./fingerprinttest -s -m gpsr, with every ingredient including tyf, passes its 10 rows
tests/module: inet_run_module_tests -m release --no-build -l ERROR	653 run, 363 pass, 290 fail	365 pass, 288 fail: the same failing tests by name, except that the new test and Ieee80211BlockAckVariant_1 now pass; Ieee80211BlockAckVariant_1 failed in the master run and in a run of an earlier version of this change, and passed in all 7 later runs, one of them with master's Ipv6NeighbourDiscovery.cc

The 5 fingerprint failures are rows of mipv6-refactoring.csv whose recorded ~tNlb fingerprints are stale on master; the 62 errors are configurations whose optional features are not built. Of the 290 module failures on master, 274 are tcp_* tests, one is the new test, and 15 are ConvolutionalCoder12, ConvolutionalCoder34, DimensionalMediumAnalogModel_PrintAttenuation_1, EtherHost_lifecycle, ExternalProcess_3, Ieee80211BitDomain, Ieee80211BlockAckVariant_1, Ieee80211SymbolDomain, Ieee8021d-Rstp, Ieee8021d-Stp, IPv6_packet_too_big, LogNormalShadowing_Correlated_4, MIPv6_tcp_handover, PacketGate_1, UDPSocket_1; all of them except Ieee80211BlockAckVariant_1 fail with this change as well.

Debug build (make MODE=debug in the root and tests/module/lib, exit 0, no compiler warning in Ipv6NeighbourDiscovery.cc): inet_run_module_tests -m debug --no-build -l ERROR passes the new test and IPv6_NUD_stale_to_probe.

Statistical baselines (the separate statistics repository at 8fdf7f5): the gpsr IPv6 and DynamicIPv6 runs record 2376 scalars each, all equal on master to the scalar at the same position in the baseline, which records more; with this change 1139 (IPv6) and 825 (DynamicIPv6) of them differ, for example host[0].app[0] pingRxSeq:count from 7 to 9 and from 0 to 1. MultiIPv6 has the same tplx and ~tNl fingerprints as IPv6, before and after. The three baselines belong in a commit to the statistics repository after this merges, naming this one. Not verified by a test: the sending of the queued datagrams from the NS path (in the reproduction the NA would send them too), and an INCOMPLETE entry whose address a Router Advertisement already recorded.

Gates: check-pull-request.sh --base origin/master (commits, classification, includes, NED parameters, description), check-source-seals.sh --base origin/master and check-architecture.sh src/inet/networklayer/icmpv6 pass.

Relationship to other pull requests

#1334 fixes a different defect on the same NA path (ReachableTime not restarted by a solicited NA); in the reproduction its defect accounts for the Neighbour Unreachability Detection starts on master. Ipv6NeighbourCache::remove() does not cancel arTimer (not reported yet); on master the leftover timer of this defect can outlive an entry that Neighbour Unreachability Detection removes, and this change removes that case for the NS path. #1260 changes the RetransTimer that initiateAddressResolution() and processArTimeout() schedule with, and adds no cancellation. #1152 and #1262 also change the fingerprints of these three gpsr rows; whichever merges later re-records them. #1322, #1324, #1326, #1328 and #1334 also add an entry under "Notable bug fixes" in WHATSNEW; whichever merges later renumbers.