gnrc/nib: gnrc_ipv6_nib_get_next_hop_l2addr(): only assume neighbor cache entries to always be on-link by benpicco · Pull Request #16671 · RIOT-OS/RIOT

benpicco · 2021-07-21T21:35:38Z

Contribution description

Consider the following: A node tries to forward a packet to another host for which it does not know the route yet. It assumes it to be on-link and starts a neighbor solicitation, putting the node address in the destination cache.

Later the route is known (via a second hop) but the host is still in the NIB.

The result is that gnrc_ipv6_nib_get_next_hop_l2addr() ends up in the "nib: %s is in NC or on-link, start address resolution" case and does not attempt to resolve the route.

This results in the host remaining unreachable even though now a route is present.

The fix should make sense since only neighbor cache entries are guaranteed to be on-link.

Testing procedure

Issues/PRs references

miri64

TBH it looks to me like you want to remove the neighbor cache altogether with this PR and I don't really understand how this makes sense. Even with your explanation.

sys/net/gnrc/network_layer/ipv6/nib/_nib-arsm.c

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

miri64 · 2021-07-22T09:09:10Z

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

-                  ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
+            DEBUG("nib: %s is %s, start address resolution\n",
+                  ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)),
+                  node ? "in NC" : "on-link");


If an entry is in the NC it is by definition on-link. Not sure why the distinction is made in this debug message...

I just found it useful for debugging to know whether _on_link() returned true or if node was not NULL.

benpicco · 2021-07-22T09:48:37Z

Ok, so I think the 'original patch' would be

--- a/sys/net/gnrc/network_layer/ipv6/nib/nib.c
+++ b/sys/net/gnrc/network_layer/ipv6/nib/nib.c
@@ -220,7 +220,7 @@ int gnrc_ipv6_nib_get_next_hop_l2addr(const ipv6_addr_t *dst,
         /* consider neighbor cache entries first */
         unsigned iface = (node == NULL) ? 0 : _nib_onl_get_if(node);
 
-        if ((node != NULL) || _on_link(dst, &iface)) {
+        if (((node != NULL) && (nce->mode & _NC)) || _on_link(dst, &iface)) {
             DEBUG("nib: %s is on-link or in NC, start address resolution\n",
                   ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
             /* on-link prefixes return their interface */

Then I was wondering if it even makes sense for _nib_onl_get() to return non-NC entries as all on-link entries should also be in the neighbor cache.

If I'm wrong I can also add a _nib_onl_nc_get() and simplify some of the code that manually checks for the _NC flag now.

miri64 · 2021-07-22T10:11:52Z

Then I was wondering if it even makes sense for _nib_onl_get() to return non-NC entries as all on-link entries should also be in the neighbor cache.

Yes, have a look at the original model. _onl_entry_ts (not to confuse with on-link entries like the term is defined in RFC 4861) are just an abstract component in the NIB. They may represent a neighbor cache (when the _NC flag is set) but they also can just be the next-hop component of a route or prefix list entry. This was done to safe space as most of the times those next hops at some point also end up in the neighbor cache (but don't have to, e.g. if address resolution for that next hop is never triggered). That those kind of entries are not returned should be guaranteed since they should be _EMPTY, so using non-_NC here would definitely be wrong (as this might then return a next-hop for which reachability can not be guaranteed yet e.g.).

Regarding that particular line, it might be the case, that we only want the neighbor cache entry here, indeed, but not non-NC entries...

Consider the following: A node tries to forward a packet to another host for which it does not know the route yet. It assumes it to be on-link and starts a neighbor solicitation, putting the node address in the destinatio cache. Later the route is known (via a second hop) but the host is still in the NIB. The result is that gnrc_ipv6_nib_get_next_hop_l2addr() ends up in the "nib: %s is in NC or on-link, start address resolution" case and does not attempt to resolve the route. This results in the host remaining unreachable even though now a route is present.

benpicco · 2021-07-22T11:38:46Z

Ok I kept _nib_onl_get() as is and added _nib_onl_nc_get() instead.

If too controversial, I can drop the last two commits 😉

miri64 · 2021-07-22T12:27:36Z

It looks alright, but I am still wondering what type of on-link entry you get that this happens. Did you check (you could just output the flags in a debug line)?

The result is that gnrc_ipv6_nib_get_next_hop_l2addr() ends up in the "nib: %s is in NC or on-link, start address resolution" case and does not attempt to resolve the route.

This results in the host remaining unreachable even though now a route is present.

Also why wasn't

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

Lines 89 to 94 in 5ad58b1

    
           nce = _nib_onl_get(&ipv6->src, netif->pid); 
        
           /* NIB is full */ 
        
           if (nce == NULL) { 
        
               return SIXLOWPAN_ND_STATUS_NC_FULL; 
        
           }

updated?

miri64 · 2021-07-22T12:29:44Z

I guess the PR title should also be updated now ;-).

benpicco · 2021-07-22T12:33:31Z

It looks alright, but I am still wondering what type of on-link entry you get that this happens

I suspect it's _DC, but will check

Also why wasn't RIOT/sys/net/gnrc/network_layer/ipv6/nib/_nib-6lr.c updated?

It's not checking for nce->mode & _NC, that's what I was searching for - but maybe it should?

benpicco · 2021-07-22T13:21:02Z

It's _DST:

2021-07-22 15:18:34,837 # nib: Getting on-link node entry (addr = 2001:16b8:45d7:59fb:a43e:9dff:fe26:7a2e, iface = 0)
2021-07-22 15:18:34,837 #   Found 0x2000224c
2021-07-22 15:18:34,838 # nib: 2001:16b8:45d7:59fb:a43e:9dff:fe26:7a2e is in NC, start address resolution
2021-07-22 15:18:34,838 # nce->mode: 40
2021-07-22 15:18:34,839 # nib: Getting on-link node entry (addr = 2001:16b8:45d7:59fb:a43e:9dff:fe26:7a2e, iface = 7)
2021-07-22 15:18:34,839 #   Found 0x2000224c

miri64 · 2021-07-22T14:22:24Z

Ah right! That mode is used when an _offl_entry_t is referring to that, so I think after all, your change is valid.

benpicco changed the title ~gnrc/nib: only consider neighbor cache entries in _nib_onl_get() ~ gnrc/nib: only consider neighbor cache entries in gnrc_ipv6_nib_get_next_hop_l2addr() 1 hour ago

Just from the title I know wonder: so no FIB lookup happens now in gnrc_ipv6_nib_get_next_hop_l2addr()? I think you need to clarify ;-)

miri64 · 2021-07-22T17:02:21Z

On the other hand: why is there a global address as next hop in the first place? Typically, (while not necessarily) they are link-local.

benpicco · 2021-07-22T17:31:48Z

On the other hand: why is there a global address as next hop in the first place? Typically, (while not necessarily) they are link-local.

#16665 👀

miri64 · 2021-07-22T18:43:08Z

On the other hand: why is there a global address as next hop in the first place? Typically, (while not necessarily) they are link-local.

#16665 eyes

I don't see the relation between source address selection and next-hops of routes...

benpicco · 2021-07-23T16:23:01Z

If the node takes the source address of the packet it got from the next hop as it's next-hop-node, then it will do just that.

miri64 · 2021-07-29T08:35:33Z

If the node takes the source address of the packet it got from the next hop as it's next-hop-node, then it will do just that.

Not really sure how to parse this sentence? Why would a packet come from the next hop? My understanding of routing is that packets go to a next hop. And why does this decide the source address of a packet sent by the host at hand? Forwarding a packet should not change the addresses of a packet.

benpicco · 2021-07-29T09:13:12Z

This is with #16568. If the Router Advertisement contains a RIO, the node that receives it will use the source address of the RA as the next hop to reach the the network advertised in the RIO.

This however is wholly unrelated to this PR.

miri64 · 2021-07-29T09:22:06Z

This is with #16568. If the Router Advertisement contains a RIO, the node that receives it will use the source address of the RA as the next hop to reach the the network advertised in the RIO.

Then those next hops should not be global, as RA source addresses MUST be link-local. If that's not the case, this is the bug that needs fixing.

miri64 · 2021-07-29T09:26:33Z

That a router advertisement has a link-local address as source should be assured by

RIOT/sys/net/gnrc/network_layer/ndp/gnrc_ndp.c

Lines 493 to 502 in feac187

    
                   if (src == NULL) { 
        
                       /* get address from source selection algorithm. 
        
                        * Only link local addresses may be used (RFC 4861 section 4.1) */ 
        
                       src = gnrc_netif_ipv6_addr_best_src(netif, dst, true); 
        
                       if (src == NULL) { 
        
                           DEBUG("ndp rtr: no VALID source address found for RA\n"); 
        
                           break; 
        
                       } 
        
                   }

unless src was initialized on call of course, which isn't the case in current master:

$ git grep "rtr_adv_send" | grep -v "^tests"
sys/include/net/gnrc/ndp.h:void gnrc_ndp_rtr_adv_send(gnrc_netif_t *netif, const ipv6_addr_t *src,
sys/net/gnrc/network_layer/ipv6/nib/_nib-router.c:    gnrc_ndp_rtr_adv_send(netif, NULL, dst, final, ext_opts);
sys/net/gnrc/network_layer/ndp/gnrc_ndp.c:void gnrc_ndp_rtr_adv_send(gnrc_netif_t *netif, const ipv6_addr_t *src,

miri64 · 2021-07-29T09:28:48Z

RAs with non-link-local addresses should also be rejected via this check

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

Lines 597 to 614 in feac187

    
           if (!(ipv6_addr_is_link_local(&ipv6->src)) || 
        
               (ipv6->hl != NDP_HOP_LIMIT) || (rtr_adv->code != 0U) || 
        
               (icmpv6_len < sizeof(ndp_rtr_adv_t)) || 
        
               (!gnrc_netif_is_6ln(netif) && 
        
                (byteorder_ntohs(rtr_adv->ltime) > NDP_RTR_ADV_LTIME_SEC_MAX))) { 
        
               DEBUG("nib: Received router advertisement is invalid. " 
        
                     "Discarding silently\n"); 
        
               DEBUG("     - IP Hop Limit: %u (should be %u)\n", ipv6->hl, 
        
                     NDP_HOP_LIMIT); 
        
               DEBUG("     - ICMP code: %u (should be 0)\n", rtr_adv->code); 
        
               DEBUG("     - ICMP length: %u (should > %u)\n", (unsigned)icmpv6_len, 
        
                     (unsigned)sizeof(ndp_rtr_adv_t)); 
        
               DEBUG("     - Source address: %s (should be link-local)\n", 
        
                     ipv6_addr_to_str(addr_str, &ipv6->src, sizeof(addr_str))); 
        
               DEBUG("     - Router lifetime: %u (should be <= 9000 on non-6LN)\n", 
        
                     byteorder_ntohs(rtr_adv->ltime)); 
        
               return; 
        
           }

benpicco · 2021-07-29T10:30:02Z

tbh I'm not sure anymore with which state of my PRs I got to this case, however this PR should be useful regardless as it only does two things:

adds a missing node->mode & _NC check in gnrc_ipv6_nib_get_next_hop_l2addr() to ensure the entry is on-link
adds a _nib_onl_nc_get() helper function to get rid of all those sprawling (entry != NULL) && (entry->mode & _NC) checks that are easy to forget / get wrong

miri64 · 2021-07-29T10:35:41Z

adds a missing node->mode & _NC check in gnrc_ipv6_nib_get_next_hop_l2addr() to ensure the entry is on-link

Not sure this is the right way to go. Only because an entry is not in the neighbor cache (yet), does not mean it is not on-link. A routing protocol (or for that matter a user using the nib route command) could have added that entry and the only thing missing for that entry to become a neighbor cache entry might be address resolution which would be triggered by sending to that neighbor.

adds a _nib_onl_nc_get() helper function to get rid of all those sprawling (entry != NULL) && (entry->mode & _NC) checks that are easy to forget / get wrong

That I don't argue with :-).

miri64 · 2021-07-29T10:40:01Z

Another reason could be, that the node simply just cached out from the neighbor cache (but still is kept in the _onl_entry_t list for other purposes) e.g. because the sending node tries to reach a lot of neighbors in a short time frame, leading to a round robin situation in the neighbor cache.

benpicco · 2021-07-29T10:56:40Z

But in that case we would still do the _on_link(dst, &iface) check. (see 11e9b7c)
The problem is that the _on_link(dst, &iface) check is not performed if the destination is already an _offl_entry_t.

miri64 · 2021-07-29T13:38:57Z

But in that case we would still do the _on_link(dst, &iface) check.
The problem is that the _on_link(dst, &iface) check is not performed if the destination is already an _offl_entry_t.

This somehow contradicts the title of this PR, but maybe only that needs adaption. Apart from that, I think the claim is valid. If there is no NC, there is no link-layer address.

benpicco · 2021-08-02T19:55:07Z

Does this make for a better PR title? 😉

benpicco · 2021-08-11T10:09:57Z

@miri64 ping 😉

miri64

ACK, changes make sense now.

benpicco requested review from PeterKietzmann, cgundogan, haukepetersen and miri64 as code owners July 21, 2021 21:35

benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jul 21, 2021

github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jul 21, 2021

benpicco force-pushed the gnrc/nib-_nib_onl_get-fix branch from 437c37c to e33f429 Compare July 21, 2021 22:34

miri64 requested changes Jul 22, 2021

View reviewed changes

gnrc_ipv6_nib: improve debug output

ac0b363

benpicco force-pushed the gnrc/nib-_nib_onl_get-fix branch 2 times, most recently from 419007d to 53efcd4 Compare July 22, 2021 11:36

benpicco added 3 commits July 22, 2021 13:36

gnrc_ipv6_nib: add _nib_onl_nc_get()

c189ed5

gnrc_ipv6_nib: make use of _nib_onl_nc_get()

5ad58b1

benpicco force-pushed the gnrc/nib-_nib_onl_get-fix branch from 53efcd4 to 5ad58b1 Compare July 22, 2021 11:37

benpicco changed the title ~~gnrc/nib: only consider neighbor cache entries in _nib_onl_get()~~ gnrc/nib: only consider neighbor cache entries in gnrc_ipv6_nib_get_next_hop_l2addr() Jul 22, 2021

benpicco changed the title ~~gnrc/nib: only consider neighbor cache entries in gnrc_ipv6_nib_get_next_hop_l2addr()~~ gnrc/nib: only consider neighbor cache entries as on-link in gnrc_ipv6_nib_get_next_hop_l2addr() Jul 22, 2021

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 27, 2021

benpicco changed the title ~~gnrc/nib: only consider neighbor cache entries as on-link in gnrc_ipv6_nib_get_next_hop_l2addr()~~ gnrc/nib: only consider neighbor cache entries as guaranteed on-link in gnrc_ipv6_nib_get_next_hop_l2addr() Jul 29, 2021

benpicco mentioned this pull request Aug 3, 2021

gnrc_ipv6_simple_subnets: auto-configuration for nested subnets on a simple tree topology #16536

Merged

benpicco changed the title ~~gnrc/nib: only consider neighbor cache entries as guaranteed on-link in gnrc_ipv6_nib_get_next_hop_l2addr()~~ gnrc/nib: gnrc_ipv6_nib_get_next_hop_l2addr(): only consider neighbor cache entries as guaranteed on-link Aug 4, 2021

benpicco changed the title ~~gnrc/nib: gnrc_ipv6_nib_get_next_hop_l2addr(): only consider neighbor cache entries as guaranteed on-link~~ gnrc/nib: gnrc_ipv6_nib_get_next_hop_l2addr(): only consider neighbor cache entries to be guaranteed on-link Aug 4, 2021

benpicco changed the title ~~gnrc/nib: gnrc_ipv6_nib_get_next_hop_l2addr(): only consider neighbor cache entries to be guaranteed on-link~~ gnrc/nib: gnrc_ipv6_nib_get_next_hop_l2addr(): only consider neighbor cache entries to always be on-link Aug 4, 2021

benpicco changed the title ~~gnrc/nib: gnrc_ipv6_nib_get_next_hop_l2addr(): only consider neighbor cache entries to always be on-link~~ gnrc/nib: gnrc_ipv6_nib_get_next_hop_l2addr(): only assume neighbor cache entries to always be on-link Aug 4, 2021

miri64 approved these changes Aug 11, 2021

View reviewed changes

miri64 merged commit de3e5f9 into RIOT-OS:master Aug 11, 2021

benpicco deleted the gnrc/nib-_nib_onl_get-fix branch August 11, 2021 10:17

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

Conversation

benpicco commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miri64 Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

benpicco Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

benpicco commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Jul 22, 2021

Uh oh!

miri64 commented Jul 22, 2021

Uh oh!

miri64 commented Jul 22, 2021

Uh oh!

benpicco commented Jul 22, 2021

Uh oh!

benpicco commented Jul 22, 2021

Uh oh!

miri64 commented Jul 22, 2021

Uh oh!

miri64 commented Jul 22, 2021

Uh oh!

benpicco commented Jul 22, 2021

Uh oh!

miri64 commented Jul 22, 2021

Uh oh!

benpicco commented Jul 23, 2021

Uh oh!

miri64 commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Jul 29, 2021

Uh oh!

miri64 commented Jul 29, 2021

Uh oh!

miri64 commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jul 29, 2021

Uh oh!

benpicco commented Jul 29, 2021

Uh oh!

miri64 commented Jul 29, 2021

Uh oh!

miri64 commented Jul 29, 2021

Uh oh!

benpicco commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Aug 11, 2021

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

benpicco commented Jul 21, 2021 •

edited

Loading

benpicco commented Jul 22, 2021 •

edited

Loading

miri64 commented Jul 22, 2021 •

edited

Loading

miri64 commented Jul 29, 2021 •

edited

Loading

miri64 commented Jul 29, 2021 •

edited

Loading

benpicco commented Jul 29, 2021 •

edited

Loading

miri64 commented Jul 29, 2021 •

edited

Loading

benpicco commented Aug 2, 2021 •

edited

Loading