gnrc_ipv6_nib: consider largest prefix match when deciding if host on-link by benpicco · Pull Request #16557 · RIOT-OS/RIOT

benpicco · 2021-06-15T17:01:03Z

Contribution description

If the fib contains a route to a subnet via another host, it can not be on-link.

Consider fib entries when deciding whether an address is on-link.
If a route via another host is a stronger match than an on-link prefix, the address must be off-link.

Testing procedure

see #8823 (comment)

this required manually adding a routing entry for the next hop via nib route add 5 2001:16b8:45b5:9afa::/63 fe80::5884:3bff:fe4f:a903 (~~is it possible to use NDP for this?~~ yes, see #16568)

2021-06-15 19:01:41,048 # 2001:16b8:45b5:9afa::/63 via fe80::5884:3bff:fe4f:a903 dev #5
2021-06-15 19:01:41,051 # 2001:16b8:45b5:9af8::/62 dev #5
2021-06-15 19:01:41,054 # 2001:16b8:45b5:9a00::/64 dev #6
2021-06-15 19:01:41,058 # default via fe80::de39:6fff:fe6a:6980 dev #6

Here packets for the 2001:16b8:45b5:9afa::/63 network should be routed through fe80::5884:3bff:fe4f:a903.
On master hosts in this network are considered on-link as 2001:16b8:45b5:9af8::/62 is already a match. A neighbor solicitation on interface #5 for any host in 2001:16b8:45b5:9afa::/63 will however not yield any results as those are behind another hop. Sending fails with a host unreachable message.

Issues/PRs references

#8823 (comment)

miri64 · 2021-06-15T17:11:05Z

is it possible to use NDP for this?)

No, appart from prefix list and the default router list (which are a sub-view of the FIB), the FIB is not filled by NDP. That's the job of a routing protocol.

MrKevinWeiss · 2021-06-22T12:55:21Z

I may have some pull to get @cgundogan or @PeterKietzmann to review. Bug fixes before the release are always nice.

fjmolinas · 2021-07-06T08:27:07Z

I may have some pull to get @cgundogan or @PeterKietzmann to review. Bug fixes before the release are always nice.

@MrKevinWeiss where you able to use your "pull" :)

MrKevinWeiss · 2021-08-02T12:29:22Z

Seems like not really, I will try again next time I see them in the office.

benpicco · 2021-08-06T14:48:30Z

@miri64 what's your 2¢ on this?

miri64 · 2021-08-07T11:53:23Z

TBH I am not quite sure why the distinction for the FIB is needed. If the prefix is not in the prefix list or if it is not marked as on-link (which is your true condition for on_link as well), it should not be on-link. So why the extra check for the FIB? A prefix that is in there and not in the prefix list, will be off-link by the check against the prefix list, a prefix that is in the prefix list but not marked as on-link will also be off-link via the check against the prefix list. And a FIB entry for which the prefix is marked as on-link in the prefix list, must also be on-link (which as far as I can see your code reflects)... So really not sure why the FIB check is needed. On-link / off-link is per RFC decided by the prefix list (+ if the address is link-local), so adding the FIB (even if the FIB is out of scope of the NDP RFC) seems off also from a design standpoint.

miri64 · 2021-08-07T11:56:08Z

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

+                continue;
+            }
+
            *iface = _nib_onl_get_if(entry->next_hop);


BTW there might be an (unrelated) bug here. As for 6Lo-ND only link-local addresses are on-link, best_pfx should only be set if *iface is not a 6LN interface (this case is checked above, but only when *iface was initialized before the call.

Huh, if the WPAN got a prefix from the border router, all addresses with that prefix should be on-link too, no?

Ah I just remembered: No the prefix should not be advertised as on-link in the first place: https://datatracker.ietf.org/doc/html/rfc6775#section-5.4

(see also https://datatracker.ietf.org/doc/html/rfc6775#section-6.1)

miri64 · 2021-08-07T11:58:32Z

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

 #endif  /* CONFIG_GNRC_IPV6_NIB_6LN */
    while ((entry = _nib_offl_iter(entry))) {
-        if ((entry->mode & _PL) && (entry->flags & _PFX_ON_LINK) &&
-            (ipv6_addr_match_prefix(dst, &entry->pfx) >= entry->pfx_len) &&


Maybe the problem is that this should rather be a == or <= and that is why you get the wrong prefixes reported as on-link?

benpicco · 2021-08-07T12:36:23Z

This is for the cascading subnets use-case (#16536):

Here 2001:db8::/60 is on-link for the main network. One host however creates it's own subnet from that: 2001:db8:0:8::/61.

Now addresses like 2001:db8:0:8:6cdc:64a3:def5:4ef1 are in 2001:db8::/60 and would be considered on-link. If Node A attempts to do a neighbor solicitation for 2001:db8:0:8:6cdc:64a3:def5:4ef1 it will fail as this host is not on-link.

That's why when the router created the subnet, it also sent a router advertisement with a RIO for the new prefix (#16568).

This ends up creating a forwarding table entry that looks like

2001:db8:0:8::/61 via 2001:db8::c8f4:13ff:fece:3f43

(I'm only using global addresses in the example for simplicity's sake). Now with this patch node A will see that the fib entry for 2001:db8:0:8::/61 is a stronger match than 2001:db8::/60 when sending to 2001:db8:0:8:6cdc:64a3:def5:4ef1 and no longer consider the destination on-link but route it via the router node.

miri64 · 2021-08-07T13:43:50Z

Now with this patch node A will see that the fib entry for 2001:db8:0:8::/61 is a stronger match than 2001:db8::/60

Mhh this sounds to me more like the matching algorithm is wrong here. Maybe the way to go would here to first find the best matching entry in the prefix list (read _offl_entry_t list for once here) and only then determine if that prefix entry is marked as on-link instead of doing it on-the-fly like we do now. This sounds to me like a much more clean approach, as technically you also would need to consider the Destination Cache here as "off-link" (basically any entry that is not in the prefix list and marked as on-link) and that sounds like a mess...

benpicco · 2021-08-07T14:27:31Z

So are you suggesting something like this?

static bool _on_link(const ipv6_addr_t *dst, unsigned *iface)
{
    _nib_offl_entry_t *entry = NULL;
    _nib_offl_entry_t *match = NULL;

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_6LN)
    if (*iface != 0) {
        if (gnrc_netif_is_6ln(gnrc_netif_get_by_pid(*iface))) {
            return ipv6_addr_is_link_local(dst);
        }
    }
#endif  /* CONFIG_GNRC_IPV6_NIB_6LN */

    while ((entry = _nib_offl_iter(entry))) {
        if ((ipv6_addr_match_prefix(dst, &entry->pfx) >= entry->pfx_len) &&
            ((match == NULL) || (entry->pfx_len > match->pfx_len))) {
            match = entry;
        }
    }

    if (match) {
        *iface = _nib_onl_get_if(match->next_hop);
        return match->flags & _PFX_ON_LINK;
    }
    return ipv6_addr_is_link_local(dst);
}

And since we are always checking for link-local in the end, why not do it first?

static bool _on_link(const ipv6_addr_t *dst, unsigned *iface)
{
    _nib_offl_entry_t *entry = NULL;
    _nib_offl_entry_t *match = NULL;

    if (ipv6_addr_is_link_local(dst)) {
        return true;
    }

    while ((entry = _nib_offl_iter(entry))) {
        if ((ipv6_addr_match_prefix(dst, &entry->pfx) >= entry->pfx_len) &&
            ((match == NULL) || (entry->pfx_len > match->pfx_len))) {
            match = entry;
        }
    }

    if (match) {
        *iface = _nib_onl_get_if(match->next_hop);
        return match->flags & _PFX_ON_LINK;
    }
    return false;
}

miri64 · 2021-08-09T10:03:15Z

And since we are always checking for link-local in the end, why not do it first?

static bool _on_link(const ipv6_addr_t *dst, unsigned *iface)
{
    _nib_offl_entry_t *entry = NULL;
    _nib_offl_entry_t *match = NULL;

    if (ipv6_addr_is_link_local(dst)) {
        return true;
    }

    while ((entry = _nib_offl_iter(entry))) {
        if ((ipv6_addr_match_prefix(dst, &entry->pfx) >= entry->pfx_len) &&
            ((match == NULL) || (entry->pfx_len > match->pfx_len))) {
            match = entry;
        }
    }

    if (match) {
        *iface = _nib_onl_get_if(match->next_hop);
        return match->flags & _PFX_ON_LINK;
    }
    return false;
}

Yepp, that looks good.

miri64 · 2021-08-09T10:04:54Z

        return match->flags & _PFX_ON_LINK;

Ah wait, the check if the entry is in the prefix list should stay. Not sure, if the flags are cleared, when match->mode & _PL is cleared.

miri64 · 2021-08-09T11:15:28Z

Does it fix the problem you encountered?

benpicco · 2021-08-09T14:49:06Z

Yes with this #16536 is working (I discovered a minor bug in gnrc_netif_ipv6_add_prefix() in the process)

miri64

The change looks sensible now and I trust your testing. Please squash (and change the commit message to reflect the changed directive of the PR).

…-link If the fib contains a route to a subnet via another host, it can not be on-link. Consider fib entries when deciding whether an address is on-link. If a route via another host is a stronger match than an on-link prefix, the address must be off-link.

miri64

ACK

benpicco requested review from PeterKietzmann, cgundogan, haukepetersen and miri64 as code owners June 15, 2021 17:01

benpicco added Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jun 15, 2021

github-actions bot added the Area: sys Area: System label Jun 15, 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 Jun 17, 2021

benpicco mentioned this pull request Jun 17, 2021

gnrc_ipv6_nib: handle route information option and add config to add to final RAs #16568

Merged

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

benpicco mentioned this pull request Jun 22, 2021

gnrc_ipv6_nib: consider all local interfaces when looking for address #16569

Merged

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 22, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

benpicco force-pushed the gnrc_ipv6_nib-fib_on-link branch from d95c014 to 76a2617 Compare July 22, 2021 13:32

benpicco mentioned this pull request Aug 3, 2021

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

Merged

miri64 reviewed Aug 7, 2021

View reviewed changes

miri64 changed the title ~~gnrc_ipv6_nib: consider fib when deciding if host on-link~~ gnrc_ipv6_nib: consider largest prefix match when deciding if host on-link Aug 9, 2021

miri64 reviewed Aug 9, 2021

View reviewed changes

benpicco force-pushed the gnrc_ipv6_nib-fib_on-link branch from cbb1d7b to 452500b Compare August 9, 2021 14:57

miri64 approved these changes Aug 9, 2021

View reviewed changes

miri64 merged commit c86bd74 into RIOT-OS:master Aug 9, 2021

benpicco deleted the gnrc_ipv6_nib-fib_on-link branch August 9, 2021 16:59

miri64 mentioned this pull request Aug 14, 2021

gnrc/nib: consolidate prefix removal code in _nib_offl_remove_prefix() #16729

Merged

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

benpicco mentioned this pull request Feb 17, 2022

Revert "gnrc_ipv6_nib: consider all local interfaces when looking for… #17676

Merged

Conversation

benpicco commented Jun 15, 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 commented Jun 15, 2021

Uh oh!

MrKevinWeiss commented Jun 22, 2021

Uh oh!

fjmolinas commented Jul 6, 2021

Uh oh!

MrKevinWeiss commented Aug 2, 2021

Uh oh!

benpicco commented Aug 6, 2021

Uh oh!

miri64 commented Aug 7, 2021

Uh oh!

miri64 Aug 7, 2021

Choose a reason for hiding this comment

Uh oh!

benpicco Aug 7, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Aug 7, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Aug 7, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

miri64 commented Aug 7, 2021

Uh oh!

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

Uh oh!

Uh oh!

miri64 commented Aug 9, 2021

Uh oh!

miri64 commented Aug 9, 2021

Uh oh!

miri64 commented Aug 9, 2021

Uh oh!

benpicco commented Aug 9, 2021

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benpicco commented Jun 15, 2021 •

edited

Loading

miri64 Aug 7, 2021 •

edited

Loading

benpicco commented Aug 7, 2021 •

edited

Loading

benpicco commented Aug 7, 2021 •

edited

Loading