nib: implement public NIB functions up to link-local AR #7014

miri64 · 2017-05-05T11:54:47Z

Implements link-local AR-capabilities for 6Lo and normal NDP with NDP model, including address registration for 6Lo (though not in effect, since there is no router discovery yet).

Still missing:

tests
stack integration

Depends on ~~#6380~~ (closed), ~~#6988~~ (merged), ~~#7064~~ (merged) and ~~#7179~~ (merged) (and their respective dependencies)

This PR is part of the network layer remodelling effort:

miri64 · 2017-05-16T08:09:29Z

Rebased to current master, #6988, and #7064.

miri64 · 2017-05-22T15:02:06Z

No longer WIP, but due to the link-local only nature and some lingering bugs in evtimer still experimental.

If you want to test with gnrc_networking set module gnrc_ipv6_nib (for normal IPv6) or gnrc_ipv6_nib_6ln (for 6Lo-ND). Its mutually exclusive to old NDP and has higher priority, so there shouldn't be any conflict.

cgundogan · 2017-05-29T09:40:39Z

If you want to test with gnrc_networking set module gnrc_ipv6_nib (for normal IPv6) or gnrc_ipv6_nib_6ln (for 6Lo-ND). Its mutually exclusive to old NDP and has higher priority, so there shouldn't be any conflict.

I did this, but I noticed that ncache does not show the entries. Furthermore, when I ping (native) between two nodes using link-local addresses, then the first NS/NA is performed successfully and the ping works as expected. A second ping attempt fails however until I reboot the nodes. Is that in accordance with what you classified as lingering bugs in evtimer?

miri64 · 2017-05-29T09:56:10Z

did this, but I noticed that ncache does not show the entries.

The ncache command isn't implemented with NIB so this doesn't surprise me and my plan is to write a new command altogether for the nib (similar to Linux' ip command). However, this PR is big enough as is, so I wanted to do this in a follow up.

Furthermore, when I ping (native) between two nodes using link-local addresses, then the first NS/NA is performed successfully and the ping works as expected. A second ping attempt fails however until I reboot the nodes. Is that in accordance with what you classified as lingering bugs in evtimer?

Will look into that.

miri64 · 2017-06-02T16:11:30Z

the first NS/NA is performed successfully and the ping works as expected. A second ping attempt fails however until I reboot the nodes.

"Fixed" but there is still some issue, that after a node gets to PROBE state that the other node doesn't answer. Will debug [edit]next week[/edit].

miri64 · 2017-06-06T10:49:03Z

Fixed + added shell command nib neigh to inspect the neighbor cache.

miri64 · 2017-06-10T22:37:33Z

added shell command nib neigh to inspect the neighbor cache.

(I moved the shell command to #6988, when I rebase it, the commit will disappear)

…OT-OS#7014

miri64 · 2017-06-10T23:15:26Z

Rebased and adopted to current master and #6988.

miri64 · 2017-06-13T09:49:19Z

Added #7179 as dependency.

cgundogan · 2017-10-06T21:02:00Z

I think there are more instances where the doc should've used third person at the beginning of the sentence. Not just those that I pointed out.

miri64 · 2017-10-06T21:26:32Z

And rebased again ^^

…IOT-OS#7456

cgundogan

second round .. not finished yet.

cgundogan · 2017-10-07T07:33:11Z

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

+        if ((nce == NULL) || !(nce->mode & _NC) ||
+            (memcmp(&nce->eui64, &aro->eui64, sizeof(aro->eui64)) == 0)) {
+#if GNRC_IPV6_NIB_CONF_MULTIHOP_DAD
+            /* TODO */


Could you add a TODO to the doxygen? It should be obvious somewhere that multihop DAD is not working currently.

Will do in sys/include/net/gnrc/ipv6/nib.h

cgundogan · 2017-10-07T07:40:24Z

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

+#if GNRC_IPV6_NIB_CONF_MULTIHOP_DAD
+            /* TODO */
+#endif
+            if (byteorder_ntohs(aro->ltime) != 0) {


4 levels of indentation .. if it's no hassle, could you try to remove some nestedness here?

cgundogan · 2017-10-07T08:03:38Z

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

+ * @param[in] nbr_sol   The neighbor solicitation carrying the original ARO
+ *                      (handed over as @ref icmpv6_hdr_t, since it is just
+ *                      handed to @ref _handle_aro()).
+ * @param[in] aro       The original riginal ARO


~~riginal~~

cgundogan · 2017-10-07T08:08:24Z

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

+{
+    gnrc_pktsnip_t *reply_aro = NULL;
+
+    if (aro != NULL) {


why not assert this? Is it possible that – by design – NULL is passed to this function as aro? If not by design, then this is an unexpected behaviour and should be handled on the caller side.

Yes, if the message does not contain an ARO this function is called regardless (to keep 6LR-mixins minimal), to check that fact.

cgundogan · 2017-10-07T08:12:09Z

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

+          ipv6_addr_to_str(addr_str, &nbr->ipv6, sizeof(addr_str)),
+          (unsigned)iface->retrans_time);
+    assert((netif != NULL) && (iface != NULL));
+#if GNRC_IPV6_NIB_CONF_ARSM


Hm, strange. This file is called _nib-arsm.c. Why shouldn't GNRC_IPV6_NIB_CONF_ARSM be defined on this code path?

Because sending Neighbor Solicitations is (originally) related to address resolution, but also required for other stuff (e.g. 6Lo address registration and SLAAC). Welcome to NDP hell ;-).

(the ARSM-only part starts in l172)

cgundogan · 2017-10-07T08:19:47Z

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

+    if ((nce != NULL) && (nce->mode & _NC) &&
+        ((nce->l2addr_len != l2addr_len) ||
+         (memcmp(nce->l2addr, sl2ao + 1, nce->l2addr_len) != 0)) &&
+        /* a 6LR MUST NOT modify an existing NCE based on an SLLAO in an RS


cgundogan · 2017-10-07T08:21:22Z

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

+            nce->info &= ~GNRC_IPV6_NIB_NC_INFO_IS_ROUTER;
+        }
+#if GNRC_IPV6_NIB_CONF_ARSM
+        /* a 6LR MUST NOT modify an existing NCE based on an SLLAO in an RS


cgundogan · 2017-10-07T08:29:03Z

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

+#endif
+}
+
+void _handle_sl2ao(kernel_pid_t iface, const ipv6_hdr_t *ipv6,


I have to admit, I don't like this function. It's too complex and I spent a lot of time to understand what's going on here – and I am quite certain I will spent the same amount of time a year later from now. Please try to remove the nested if - else hell. The Clean Code approach would be to split this function into several smaller (inlined) ones with well-documented and obvious names. I am not sure whether that's the solution for us, but the current implementation is very unreadable and IMO unmaintainable in the future.

I don't like it either, but that's exactly how the RFC reads. Welcome to NDP hell ;-)

Will try to fix it though.

cgundogan · 2017-10-07T08:57:25Z

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

+    if (tl2ao != NULL) {
+        l2addr_len = _get_l2addr_len(netif, tl2ao);
+        if (l2addr_len == 0U) {
+            DEBUG("nib: Unexpected TLLAO length. Ignoring TLLAO\n");


cgundogan · 2017-10-07T09:01:44Z

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

+    if (nib_netif->reach_time == 0) {
+        /* (re-)initialize PNRG with current system time as seed to get
+         * better randomization in reachable time */
+        random_init(xtimer_now_usec());


There's no need to re-seed the PRNG iff the start seed is distinct on all nodes. And the latter is what we should aim for.

And btw, you break here the PRNG for all applications that use this random interface with the same PRNG state.

There's no need to re-seed the PRNG iff the start seed is distinct on all nodes. And the latter is what we should aim for.

But it is not the case now. Also: this is a very standard approach to get a distinct random number.

And btw, you break here the PRNG for all applications that use this random interface with the same PRNG state.

If re-seeding breaks your PNRG then your PNRG is broken

Sorry, let me rephrase:

You are using the system/global PRNG state here. The moment you re-seed, the PRNG for all applications that make use of the system/global state do not produce deterministic results anymore. A PRNG is supposed to yield deterministic results.

Oh and btw: random_init() is not thread-safe. So please let the seeding be performed at auto_init() once, like it's the case now, or provide a separate PRNG state for NIB that can be re-seeded without global effects.

or provide a separate PRNG state for NIB that can be re-seeded without global effects.

and even then I doubt that re-seeding provides "better randomization".

and even then I doubt that re-seeding provides "better randomization".

Why are you doubting that? Currently the PNRG is always seeded with 0, there are plans to use the luid as seed (making it different but deterministic for different nodes). This reseed uses the reception time of the first received solicited NA from all neighbors, which is always naturally random, due to the different air times of both NS and NA, so it is by definition "better randomization" than using the PNRG with a deterministic seed.

What about the other points?

random_init() is not thread-safe

you modify the global PRNG state which is potentially used by more thread than just the network stack

I propose that you factor out this re-seeding procedure into another PR and we continue the discussion there, so that we can continue here?

cgundogan

Third round. All in all it's okay. I have reported some minor-ish styling issues.

cgundogan · 2017-10-07T11:31:32Z

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

+     *   and 6LN
+     * - join solicited nodes group of link-local address here for address
+     *   resolution here
+     * - join all router group of link-local address here on router node here


I'm not fixing a note to myself :P

I was just pointing it out .. you can decide what you do with it (:

cgundogan · 2017-10-07T11:31:38Z

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

+     * - join solicited nodes group of link-local address here for address
+     *   resolution here
+     * - join all router group of link-local address here on router node here
+     * - become an router advertising interface here on non-6LR here */


I'm not fixing a note to myself :P

cgundogan · 2017-10-07T11:32:24Z

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

+    mutex_lock(&_nib_mutex);
+    nib_iface = _nib_iface_get(iface);
+    assert(nib_iface != NULL);
+    /* TODO:


question: should we put this into doxygen to make this TODO more obviouvs?

These are implemented in #7456 and #7479, so why?

cgundogan · 2017-10-07T11:37:43Z

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

+    int res = 0;
+
+    mutex_lock(&_nib_mutex);
+    do {    /* XXX: hidden goto ;-) */


Since when do we use these hidden gotos? Frankly said, I do not like this idea. It 1) gives another level of indentation, 2) is an unconditional jump the same way as goto (so the same disadvantages apply) and 3) bloats the code by two more lines. I am sure you can re-arrange the code to not use this hidden goto and it still results in a more readable version (:

Since when do we use these hidden gotos? Frankly said, I do not like this idea.

Since @gebart proposed them as an alternative to "finally-gotos".

gives another level of indentation,

Since when is there this hatred for levels of indentations? We are not the Linux kernel (where this Coding Convention lead to very weird code btw).

I am sure you can re-arrange the code to not use this hidden goto and it still results in a more readable version (:

I find it quite readable.

mutex_lock(&_nib_mutex); if (!ipv6_addr_is_link_local(dst)) { /* TODO: Off-link next hop determination */ mutex_unlock(&_nib_mutex); return -EHOSTUNREACH; } if ((iface == KERNEL_PID_UNDEF) || !_resolve_addr(dst, iface, pkt, nce, _nib_onl_get(dst, iface))) { mutex_unlock(&_nib_mutex); return -EHOSTUNREACH; } mutex_unlock(&_nib_mutex); return 0;

Isn't this so much simpler to read without any level of nestedness?!

This adds 2 more function calls (mutex_unlock()), 6 in #7479 (mutex_unlock() + gnrc_netif2_release()), and even a more simpler version [edit]with only one additional mutex_unlock()[/edit]

int res = 0; mutex_lock(&_nib_mutex); if (!ipv6_addr_is_link_local(dst)) { /* TODO: Prefix-based on-link determination */ /* TODO: Off-link next hop determination */ mutex_unlock(&_nib_mutex); return -EHOSTUNREACH; } if ((iface == KERNEL_PID_UNDEF) || !_resolve_addr(dst, iface, pkt, nce, _nib_onl_get(dst, iface))) { res = -EHOSTUNREACH; } mutex_unlock(&_nib_mutex); return res;

Adds 24 byte of ROM more on Cortex-M0.

cgundogan · 2017-10-07T11:38:54Z

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

+
+void gnrc_ipv6_nib_handle_pkt(kernel_pid_t iface, const ipv6_hdr_t *ipv6,
+                              const icmpv6_hdr_t *icmpv6, size_t icmpv6_len)
+{


So many TODOS that could end up in a nicely documented fashion => doxygen (:

I'm not sure if putting TODOs in doxygen is productive here...

cgundogan · 2017-10-07T11:40:59Z

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

+    else {
+        reply_flags |= NDP_NBR_ADV_FLAGS_O;
+    }
+    /* discard const qualifier */


where is it discarded?

cgundogan · 2017-10-07T11:42:35Z

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

+    }
+    else {
+        reply_flags |= NDP_NBR_ADV_FLAGS_O;
+    }


else { reply_flags |= NDP_NBR_ADV_FLAGS_O; } } else { reply_flags |= NDP_NBR_ADV_FLAGS_O; }

you were able to remove such code duplication in another PR, can you also apply this here, please?

There it was at the end of a function, so I was just able to early exit. This is not possible here.

I could use a "hidden goto" instead ;-)

cgundogan · 2017-10-07T11:44:54Z

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

+        return;
+    }
+    /* pre-check option length */
+    FOREACH_OPT(nbr_sol, opt, tmp_len) {


nice idea (:

cgundogan · 2017-10-07T11:46:30Z

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

+        return;
+    }
+    /* pre-check option length */
+    FOREACH_OPT(nbr_adv, opt, tmp_len) {


miri64 · 2017-10-08T02:36:59Z

Addressed comments.

cgundogan · 2017-10-08T13:29:48Z

This PR seems to be in good state. The only – critically and blocking – discussion is about the re-seeding that I am strictly against. The discussion can be found in #7014 (comment) @tcschmidt @emmanuelsearch any opinions on this discussion?

cgundogan · 2017-10-09T13:54:40Z

fixup commit looks good – pleasae squash! (:

miri64 · 2017-10-09T13:57:42Z

Squashed.

cgundogan

ACK

miri64 · 2017-10-09T14:03:18Z

Fixed and squashed errors reported by Murdock.

miri64 · 2017-10-09T14:04:11Z

(gnrc_ipv6_nib_6ln had a variable defined it did not need, gnrc_ipv6_nib was to large for 3 of the smaller boards).

cgundogan · 2017-10-10T08:07:29Z

Fix looks sane, please squash.

miri64 · 2017-10-10T08:12:27Z

Squashed

miri64 assigned cgundogan, emmanuelsearch and kaspar030 May 5, 2017

miri64 requested review from cgundogan, emmanuelsearch and kaspar030 May 5, 2017 11:54

miri64 force-pushed the gnrc_ipv6_nib/feat/ndp-link-local branch from 78d83d9 to 4ee3005 Compare May 9, 2017 07:54

miri64 force-pushed the gnrc_ipv6_nib/feat/ndp-link-local branch from 4ee3005 to 4770f82 Compare May 16, 2017 08:08

miri64 mentioned this pull request May 18, 2017

gnrc_ipv6_nib: initial import of internal NIB functions #6325

Merged

miri64 changed the title ~~nib: implement public NIB functions up to link-local AR [WIP]~~ nib: implement public NIB functions up to link-local AR May 22, 2017

miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label May 22, 2017

cgundogan requested a review from tcschmidt May 26, 2017 13:05

miri64 force-pushed the gnrc_ipv6_nib/feat/ndp-link-local branch from a439a94 to 3d380ae Compare June 10, 2017 23:14

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 10, 2017

Merge branch 'gnrc_ipv6_nib/feat/nc-component' (RIOT-OS#6988) into RI…

92027f4

…OT-OS#7014

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 10, 2017

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

671feac

miri64 force-pushed the gnrc_ipv6_nib/feat/ndp-link-local branch from 64e0830 to e99e28c Compare June 13, 2017 09:50

gnrc_ipv6_nib: add configuration pseudo-modules

d30216d

miri64 force-pushed the gnrc_ipv6_nib/feat/ndp-link-local branch from 3f7bd1d to e0038c5 Compare October 6, 2017 21:26

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 6, 2017

Merge branch 'gnrc_ipv6_nib/feat/ndp-link-local' (RIOT-OS#7014) into R…

c2ab125

…IOT-OS#7456

cgundogan requested changes Oct 7, 2017

View reviewed changes

miri64 force-pushed the gnrc_ipv6_nib/feat/ndp-link-local branch from 199551b to 58aad0d Compare October 9, 2017 13:57

cgundogan approved these changes Oct 9, 2017

View reviewed changes

miri64 force-pushed the gnrc_ipv6_nib/feat/ndp-link-local branch from 58aad0d to c2a6984 Compare October 9, 2017 14:03

cgundogan 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 Oct 9, 2017

miri64 force-pushed the gnrc_ipv6_nib/feat/ndp-link-local branch from c2a6984 to 5f93ff0 Compare October 9, 2017 14:15

miri64 added 4 commits October 10, 2017 10:12

nib: implement public NIB functions up to link-local AR

1b36cdf

tests: add tests for host NDP behavior

545f54c

tests: add tests for 6LN NDP behavior

00b63e9

gnrc: integrate NIB for testing

6a7de28

miri64 force-pushed the gnrc_ipv6_nib/feat/ndp-link-local branch from 0540bea to 6a7de28 Compare October 10, 2017 08:12

miri64 merged commit 8d941d5 into RIOT-OS:master Oct 10, 2017

miri64 deleted the gnrc_ipv6_nib/feat/ndp-link-local branch October 10, 2017 08:30

miri64 mentioned this pull request Oct 17, 2017

CI: Errorneous Makefile passed Murdock somehow #7741

Closed

miri64 mentioned this pull request Mar 21, 2018

dhcpv6: Initial implementation of a client (with IA_PD support) #8796

Merged

15 tasks

nib: implement public NIB functions up to link-local AR #7014

nib: implement public NIB functions up to link-local AR #7014

Conversation

miri64 commented May 5, 2017 • edited by cgundogan Loading

miri64 commented May 16, 2017 • edited Loading

miri64 commented May 22, 2017

cgundogan commented May 29, 2017

miri64 commented May 29, 2017

miri64 commented Jun 2, 2017 • edited Loading

miri64 commented Jun 6, 2017

miri64 commented Jun 10, 2017

miri64 commented Jun 10, 2017

miri64 commented Jun 13, 2017

cgundogan commented Oct 6, 2017

miri64 commented Oct 6, 2017

cgundogan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgundogan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgundogan Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

miri64 Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Oct 8, 2017

cgundogan commented Oct 8, 2017

cgundogan commented Oct 9, 2017

miri64 commented Oct 9, 2017

cgundogan left a comment

Choose a reason for hiding this comment

miri64 commented Oct 9, 2017

miri64 commented Oct 9, 2017

cgundogan commented Oct 10, 2017

miri64 commented Oct 10, 2017

miri64 commented May 5, 2017 •

edited by cgundogan

Loading

miri64 commented May 16, 2017 •

edited

Loading

miri64 commented Jun 2, 2017 •

edited

Loading

cgundogan Oct 8, 2017 •

edited

Loading

miri64 Oct 9, 2017 •

edited

Loading