gnrc_ipv6_nib: Force unspecified next hop addresses

[xnumad]

Reserve unspecified address in NCEs for on-link prefixes, to have
_PFX_ON_LINK => ipv6_addr_is_unspecified(dst->next_hop) be true

Contribution description

In nib_[onl|offl]_entry_t,
a next_hop of NULL (as used by prefix list)
shouldn't be interpreted as "don't care"
but as an explicit "none, not applicable",
because the next_hop value is used for association with a router at

RIOT/sys/net/gnrc/network_layer/ipv6/nib/nib.c

Line 1469 in 270aa70

(ipv6_addr_equal(&route->next_hop->ipv6, &addr))) {

The code intends to delete all off-link prefixes where next hop is the specified router [1], but (without this PR) it actually also deletes on-link prefixes on the same interface [2] because they falsely use the same next hop. (And are only distinguished as on-link through _PFX_ON_LINK flag, which however is not checked for in this place.)

(Note that on-link prefixes are added again when processing a RA Prefix Information Option, which can be in the same Router Advertisement that caused _handle_rtr_timeout and thereby their accidental deletion. This seems to be common and makes this deletion hard to notice.)

[1] which is the wrong thing to do in the first place, and was therefore removed in 9648000, thereby making this bug theoretical only
[2] such as "the /128 prefix used for managing a temporary address" in #20369 (affected)

Testing procedure

Tests: #20372

[riot-ci]

Murdock results

✔️ PASSED

8b73628 gnrc_ipv6_nib: refactor

Success	Failures	Total	Runtime
10196	0	10197	17m:31s

Artifacts

• Documentation preview

[benpicco]

Now some tests are failing, is this intended?

[fabian18]

Now some tests are failing, is this intended?

I would guess not. I can only tell so far that we fail because _nib_onl_alloc() fails with next_hop == NULL.

_nib-internal.c:509

         dst->next_hop = _nib_onl_alloc(next_hop, iface);

        if (dst->next_hop == NULL) {
            memset(dst, 0, sizeof(_nib_offl_entry_t));
            return NULL;
        }

[fabian18]

Maybe no neighbor cache entry should be allocated at all if no next hop address is known.

Neighbor Cache:
A set of entries about individual neighbors to
which traffic has been sent recently. Entries are
keyed on the neighbor's on-link unicast IP address
and contain such information as its link-layer
address, a flag indicating whether the neighbor is
a router or a host (called IsRouter in this
document), a pointer to any queued packets waiting
for address resolution to complete, etc. A
Neighbor Cache entry also contains information used
by the Neighbor Unreachability Detection algorithm,
including the reachability state, the number of
unanswered probes, and the time the next Neighbor
Unreachability Detection event is scheduled to take
place.

If there is no IPv6 address for a neighbor and neighbors are keyed by their address, why would we want to add a neighbor cache entry at all?

[xnumad]

Now some tests are failing, is this intended?

No, but turns out they test in a wrong way. :D 🤯
Opened #20377

[fabian18]

I would guess not. I can only tell so far that we fail because _nib_onl_alloc() fails with next_hop == NULL.

It feels like the test was working by coincidence.
We create one NCE for the default router from the RA.
Then we process the PIO and add a prefix.
For that we want to add a NCE with (next_hop = NULL) and iface = 5.
So for a prefix, the NCE only stores the interface on which the Prefix has been received.
The NCE is found because the NCE exists from the default router but it is only matched by the interface and just because address is NULLreturns true in _addr_equals. There could have been any other Neighbor with interface 5 in the NCE, and I think this would create a wrong reference.

I think it should not be an NCE without an address just to store an interface number for a prefix.

[fabian18]

      Prefix List  - A list of the prefixes that define a set of
                     addresses that are on-link.  Prefix List entries
                     are created from information received in Router
                     Advertisements.  Each entry has an associated
                     invalidation timer value (extracted from the
                     advertisement) used to expire prefixes when they
                     become invalid.  A special "infinity" timer value
                     specifies that a prefix remains valid forever,
                     unless a new (finite) value is received in a
                     subsequent advertisement.

                     The link-local prefix is considered to be on the
                     prefix list with an infinite invalidation timer
                     regardless of whether routers are advertising a
                     prefix for it.  Received Router Advertisements
                     SHOULD NOT modify the invalidation timer for the
                     link-local prefix.

      Default Router List
                   - A list of routers to which packets may be sent.
                     Router list entries point to entries in the
                     Neighbor Cache; the algorithm for selecting a
                     default router favors routers known to be reachable
                     over those whose reachability is suspect.  Each
                     entry also has an associated invalidation timer
                     value (extracted from Router Advertisements) used
                     to delete entries that are no longer advertised.

Conceptionally the DRL and PL are two different data structures. However, we aggregate them together as offlink entries. (sorry offlink is _PFX, _DC and _FT)
For entries in the DRL it is explicitly stated that a DR references an NCE, but no word about it for prefixes.

[benpicco]

If this causes a test to fail that was testing the wrong thing, you also need to add the fix to the test in this PR, otherwise there is no way to merge this.

[xnumad]

Thanks for the heads up. The erroneous test (#20377) is already fixed in master. Therefore I rebased.

[xnumad]

Squashed your suggested change.

[benpicco]

CI finds that tests/unittests is now failing

[fabian18]

test_nib_offl_alloc__success_overwrite_unspecified expects that an offlink entry (prefix list entry) with a previously unspecified address can be updated with the actual IPv6 address of the next hop.

But currently a new offlink entry with the next hop IPv6 address is allocated and thus TEST_ASSERT(dst1 == dst2); fails.

You could update the code like this or similar to change that an offlink entry matches by interface + prefix + next hop IPv6 if not unspecified, if that works for you.

diff --git a/sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c b/sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c
index 7a33add7d1..a011244a45 100644
--- a/sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c
+++ b/sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c
@@ -504,18 +504,21 @@ _nib_offl_entry_t *_nib_offl_alloc(const ipv6_addr_t *next_hop, unsigned iface,
         _nib_offl_entry_t *tmp = &_dsts[i];
         _nib_onl_entry_t *tmp_node = tmp->next_hop;
 
-        if ((tmp->pfx_len == pfx_len) &&                /* prefix length matches and */
-            (tmp_node != NULL) &&                       /* there is a next hop that */
-            (_nib_onl_get_if(tmp_node) == iface) &&     /* has a matching interface and */
-            _addr_equals(next_hop, tmp_node) &&         /* equal address to next_hop, also */
-            (ipv6_addr_match_prefix(&tmp->pfx, pfx) >= pfx_len)) {  /* the prefix matches */
-            /* exact match (or next hop address was previously unset) */
-            DEBUG("  %p is an exact match\n", (void *)tmp);
-            if (next_hop != NULL) {
-                memcpy(&tmp_node->ipv6, next_hop, sizeof(tmp_node->ipv6));
+        if (tmp->mode != _EMPTY) {
+            /* offlink entry not empty */
+            if (tmp->pfx_len == pfx_len && ipv6_addr_match_prefix(&tmp->pfx, pfx) >= pfx_len) {
+                /* prefix matches */
+                if (_nib_onl_get_if(tmp_node) == iface && (ipv6_addr_is_unspecified(&tmp_node->ipv6) ||
+                                                                _addr_equals(next_hop, tmp_node))) {
+                    /* next hop matches or is unspecified */
+                    DEBUG("  %p is an exact match\n", (void *)tmp);
+                    if (next_hop != NULL) {
+                        memcpy(&tmp_node->ipv6, next_hop, sizeof(tmp_node->ipv6));
+                    }
+                    tmp->next_hop->mode |= _DST;
+                    return tmp;
+                }
             }
-            tmp->next_hop->mode |= _DST;
-            return tmp;
         }
         if ((dst == NULL) && (tmp_node == NULL)) {
             dst = tmp;
@@ -523,9 +526,7 @@ _nib_offl_entry_t *_nib_offl_alloc(const ipv6_addr_t *next_hop, unsigned iface,
     }
     if (dst != NULL) {
         DEBUG("  using %p\n", (void *)dst);
-        dst->next_hop = _nib_onl_alloc(next_hop, iface);
-
-        if (dst->next_hop == NULL) {
+        if (!dst->next_hop && !(dst->next_hop = _nib_onl_alloc(next_hop, iface))) {
             memset(dst, 0, sizeof(_nib_offl_entry_t));
             return NULL;
         }

[xnumad]

There could have been any other Neighbor with interface 5 in the NCE, and I think this would create a wrong reference.

Just a note in detail on the severity of this:
I think it luckily doesn't cause any errors but it can become confusing to the user.

Like you said: _nib_offl_alloc receives and stores a reference to the first NCE where the interface matches (the address is NULL and treated as “don’t care”):

RIOT/sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c

Line 526 in 3ab7896

dst->next_hop = _nib_onl_alloc(next_hop, iface);

The IPv6 address of the neighbor (dst->next_hop->ipv6) does not matter, as the NCE is only referenced to name the interface.

The NCE will never be accidentally deleted while in use, because of this safeguard

RIOT/sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.h

Line 369 in 3ab7896

if (node->mode == _EMPTY) {

And _nib_offl_alloc has _DST mode set as long as the NCE is used:

RIOT/sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c

Line 533 in 3ab7896

dst->next_hop->mode |= _DST;

Still, it might be confusing to see an NCE that should have been deleted, but is instead kept merely because an on-link prefix used it to specify an interface.

[xnumad]

You could update the code like this or similar

Thank you, @fabian18, I did not know of this behavior.
Based on the code of the failing test, I added another test to cover specifically what this PR fixes. The test demonstrates how the current implementation is a footgun, as explicitly checking _PFX_ON_LINK flag (rather than just relying on the next hop) is unintuitive and was forgotten in existing code.
(There might be a better place for the test, but it at least also fits into the code where it is now.)

[benpicco]

CI doesn't like the commit

fix: offl-only: overwrite unspecified address

since it assumes it to be a fixup commit.
Either squash it or change the commit title to include the subsystem as a prefix.

[benpicco]

would that work?

Suggested change

	if (tmp->mode != _EMPTY) {
	if (tmp->mode == _EMPTY) {
	dst = tmp;
	continue;
	}

But tbh I don't know what's the difference between tmp->next_hop == NULL and tmp->mode == _EMPTY, I would have thought tmp->next_hop == NULL could also happen on a non-empty entry.

[xnumad]

Even for an on-link prefix, tmp->next_hop points to an NCE in order to refer to an interface, so I wouldn't know a case where tmp->next_hop == NULL while tmp->mode != _EMPTY.

[xnumad]

I think your suggested change would use the last free entry instead of the first one, when there is no exact matching entry. I think, while it wouldn't cause errors, it's an unusual allocation policy (compared to all other allocations in GNRC).

[benpicco]

        if (tmp->mode == _EMPTY) {
            if (dst == NULL) {
                dst = tmp;
            }
            continue;
        }

would also work, at the expense of one more branch that might be a bit unececary.
What I'm getting at was reducing the level of indentation if you return / continue early.

[fabian18]

I think every offlink entry has an onlink entry in our implementation.
Even if it only stores an interface ID and has an unspecified next hop..
In _nib_offl_alloc, if dst was NULL this means no offlink entry could be allocated and none matched.

[fabian18]

To be safe, an assert(tmp_node) when the prefix matches does not harm.

[xnumad]

if dst was NULL this means no offlink entry could be allocated and none matched

That is true only after all entries were iterated through. The change suggested by @benpicco is however in the loop still, in which case dst == NULL means no candidate has been found so far (in previous iterations).

[fabian18]

Too me it looks good.

[fabian18]

Then you can call _nib_ft_remove() 😉

diff --git a/tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c b/tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c
index 8d7dd5968c..8f0f856433 100644
--- a/tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c
+++ b/tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c
@@ -1459,8 +1459,6 @@ static void test_nib_offl_alloc__success_overwrite_unspecified(void)
  */
 static void test_nib_offl_alloc__next_hop_indicates_whether_onl(void)
 {
-    _nib_offl_entry_t *dst1;
-    void *state = NULL;
     static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
                                                  { .u64 = TEST_UINT64 } } };
     static const ipv6_addr_t pfx = { .u64 = { { .u8 = GLOBAL_PREFIX } } };
@@ -1470,17 +1468,17 @@ static void test_nib_offl_alloc__next_hop_indicates_whether_onl(void)
     static const ipv6_addr_t *offl_pfx = &pfx;
 
     /* Add off-link entry */
-    TEST_ASSERT_NOT_NULL((dst1 = _nib_ft_add(&next_hop, IFACE, offl_pfx,
-                                                 GLOBAL_PREFIX_LEN)));
+    _nib_offl_entry_t *dst1;
+    TEST_ASSERT_NOT_NULL((dst1 = _nib_ft_add(&next_hop, IFACE, offl_pfx, GLOBAL_PREFIX_LEN)));
 
     /* Add on-link entry */
     const unsigned pfx_len = GLOBAL_PREFIX_LEN; /* arbitrary */
 
     /* (calls _nib_offl_alloc) */
-    _nib_offl_entry_t* dst = _nib_pl_add(IFACE, onl_pfx, pfx_len, UINT32_MAX, UINT32_MAX);
+    _nib_offl_entry_t *dst;
+    TEST_ASSERT_NOT_NULL((dst = _nib_pl_add(IFACE, onl_pfx, pfx_len, UINT32_MAX, UINT32_MAX)));
     TEST_ASSERT(ipv6_addr_is_unspecified(&dst->next_hop->ipv6));
-
-    TEST_ASSERT_NOT_NULL(dst);
+    /* would normally be set by PIO flags in Router Advertisement */
     dst->flags |= _PFX_ON_LINK;
 
     /* Delete all off-link entries to next_hop */
@@ -1492,18 +1490,16 @@ static void test_nib_offl_alloc__next_hop_indicates_whether_onl(void)
             /*should not need to be checked for when next hop is already checked:*/
             /*&& !(route->flags & _PFX_ON_LINK)*/
             ) {
-            route->mode &= ~_PL;
-            route->next_hop->mode &= ~_DST;
-            _nib_offl_clear(route);
+            _nib_ft_remove(route);
             }
     }
 
     /* Expected result: On-link entries remain unaffected */
     gnrc_ipv6_nib_pl_t prefix;
+    void *state = NULL;
     TEST_ASSERT_MESSAGE(gnrc_ipv6_nib_pl_iter(IFACE, &state, &prefix),
-                    "No prefix list entry found");
-    TEST_ASSERT_MESSAGE(ipv6_addr_match_prefix(onl_pfx,
-                                &prefix.pfx) >= pfx_len,
+                        "No prefix list entry found");
+    TEST_ASSERT_MESSAGE(ipv6_addr_match_prefix(onl_pfx, &prefix.pfx) >= pfx_len,
                         "Unexpected prefix configured");
 }

[fabian18]

2024-08-31 12:18:03,285 # main(): This is RIOT! (Version: 2024.10-devel-25-g8b736-nib-next-hop-addr)
2024-08-31 12:18:03,285 # Help: Press s to start test, r to print it is ready
s
2024-08-31 12:18:04,190 # START
2024-08-31 12:18:04,192 # ........................................................................................................................................................
2024-08-31 12:18:04,192 # OK (152 tests)
2024-08-31 12:18:04,192 #