ndp: separate & clean next hop & l2 determination #3622

OlegHahm · 2015-08-13T16:27:27Z

Determining the next hop and the corresponding L2 address are split up
in two steps. No next hop look up is performed for link-local addresses
any more. Destination cache has been disabled.

OlegHahm · 2015-08-13T16:33:03Z

After discussing with @emmanuelsearch and @cgundogan, we could not find a use case where the destination cache would introduce any benefit.

OlegHahm · 2015-08-13T16:38:47Z

Also, putting link-local addresses in the FIB (or looking them up there) is rather useless (see also https://tools.ietf.org/html/rfc5889).

miri64 · 2015-08-13T16:42:31Z

I don't see the benefit of splitting the function up.

OlegHahm · 2015-08-13T16:44:24Z

Next-hop determination and looking a l2 address up are two different things.

OlegHahm · 2015-08-13T16:44:48Z

(The l2 lookup function is still too long anyway.)

miri64 · 2015-08-13T16:44:59Z

sys/net/network_layer/ng_ipv6/ng_ipv6.c

+        if (ipv6_addr_is_link_local(&(hdr->dst))) {
+            memcpy(&next_hop_ip, &(hdr->dst), sizeof(ipv6_addr_t));
+        }
+        else if (!ng_ndp_node_next_hop_ipv6_addr(&next_hop_ip, &iface,


If we go for this this way, this function needs to be wrapped in the same manner as ng_ndp_node_next_hop_l2addr() is in https://github.com/RIOT-OS/RIOT/pull/3622/files#diff-c0ab51e027bbe0cfdb3482d3e29d6f34R569.

Because next-hop determination for 6LoWPAN-ND is not the same as for NDP.

(in 6LoWPAN-ND it even is different for routers and hosts):

~~Hosts~~ Routers: https://tools.ietf.org/html/rfc6775#section-6.5.4

~~Routers~~ Hosts: https://tools.ietf.org/html/rfc6775#section-5.6

But this is not implemented yet anyway, but actually looking at the RFC I don't see a big difference for the next-hop determination itself.

miri64 · 2015-08-13T16:49:19Z

Next-hop determination and looking a l2 address up are two different things.

Yes, but there is no use case where they are used separately. They are always used together and always in the exact same order. So why not safe some ROM and put them together in one function (this was my original rational).

OlegHahm · 2015-08-13T16:56:55Z

For link-local addresses a FIB lookup is not needed (without destination cache).

miri64 · 2015-08-13T17:01:24Z

Yes, but on-link prefix determination and default router determination. Also the diff is kind of messy so I'm not sure, but can it be that you forgot on-link prefix determination?

miri64 · 2015-08-13T17:01:54Z

Next-hop determination for a given unicast destination operates as
follows. The sender performs a longest prefix match against the
Prefix List to determine whether the packet's destination is on- or
off-link. If the destination is on-link, the next-hop address is the
same as the packet's destination address. Otherwise, the sender
selects a router from the Default Router List (following the rules
described in Section 6.3.6).

OlegHahm · 2015-08-13T17:13:37Z

I read this.

OlegHahm · 2015-08-13T17:18:34Z

On-link determination in wireless networks is difficult anyway (see RFC 5889). The current implementation seems to look up a flag for an address configured to a local address for on-link determination which seems weird.

miri64 · 2015-08-13T17:33:14Z

On-link determination in wireless networks is difficult anyway (see RFC 5889).

We don't deal with just wireless networks anymore. As far as I read RFC 5889 prefixes should just not be configured to be on-link (so you are not to allowed the flag) for such networks. That's a situation we have to deal with, but not by omitting on-link prefix determination all-together but by proper prefix initialization.

The current implementation seems to look up a flag for an address configured to a local address for on-link determination which seems weird.

Why? In RIOT the prefix list and the local address list are the same (as proposed by RFC 4861) and on-link is a flag of this prefix. It is either set on the prefix' initialization (when a user adds an address to the interface - this is the point we need to change in regard to RFC 5889) or when it receives the prefix via a PIO with the on-link flag set. This and checking the neighbor cache are the only options in our current implementation if you take the definition in RFC 4861 in account

   on-link     - an address that is assigned to an interface on a
                 specified link.  A node considers an address to be on-
                 link if:

                    - it is covered by one of the link's prefixes (e.g.,
                      as indicated by the on-link flag in the Prefix
                      Information option), or

                    - a neighboring router specifies the address as the
                      target of a Redirect message, or

                    - a Neighbor Advertisement message is received for
                      the (target) address, or

                    - any Neighbor Discovery message is received from
                      the address.

miri64 · 2015-08-13T17:36:42Z

Yes I actually forgot to check the neighbor cache in the current implementation, but this is actually an argument FOR keeping the function together. Otherwise you would have to look into the neighbor cache twice to make a clean implementation: once when you determine the next hop and then for link-layer address determination.

OlegHahm · 2015-08-14T11:43:12Z

We don't deal with just wireless networks anymore. As far as I read RFC 5889 prefixes should just not be configured to be on-link (so you are not to allowed the flag) for such networks. That's a situation we have to deal with, but not by omitting on-link prefix determination all-together but by proper prefix initialization.

True, we're not dealing only with wireless networks, but for now most of the time. My argument (and I think also @emmanuelsearch's one) is that "on-link" is difficult to determine in wireless networks. Thinking about failing nodes, changing links and unidirectional links.

Why? In RIOT the prefix list and the local address list are the same (as proposed by RFC 4861) and on-link is a flag of this prefix.

Is this documented somewhere? The lookup as it is in the current implementation without any comment looks just weird.

Summa summarum, I would argue that at least for the wireless case on-link determination per prefix is pointless and should be omitted. So this should at least be configurable. @emmanuelsearch, what do you think?

miri64 · 2015-08-14T12:24:49Z

Is this documented somewhere? The lookup as it is in the current implementation without any comment looks just weird.

Seems not to be, but the fact alone, that the assigned addresses have also a prefix length assigned to them makes it a prefix list (otherwise it wouldn't be needed)

miri64 · 2015-08-14T12:27:28Z

Summa summarum, I would argue that at least for the wireless case on-link determination per prefix is pointless and should be omitted. So this should at least be configurable. @emmanuelsearch, what do you think?

Then make it configurable using a flag or something. We have at least one device regular used that isn't wireless: native.

emmanuelsearch · 2015-08-14T12:35:52Z

Concerning "on-link" and spontaneous wireless networks, I think about section 6.1. of RFC5889. Basically in our context, on-link means nothing useful, so I am of the opinion of (i) not using this concept and associated flags in wireless context and (ii) focus on using non-link-local address as much of the time in wireless context.

OlegHahm · 2015-08-14T12:39:37Z

We have at least one device regular used that isn't wireless: native.

Nit-picky: native is not device. More seriously: we want native to behave as much as possible as real devices, so I wouldn't make an exception for native. However, I agree that we might want to distinguish between wireless and wired (Ethernet, MS/TP...).

OlegHahm · 2015-08-23T23:16:22Z

Updated and rebased on #3697.

Now on-link prefix lookup is only performed for wired interfaces.

Determining the next hop and the corresponding L2 address are split up in two steps. No next hop look up is performed for link-local addresses any more. Destination cache has been disabled.

OlegHahm · 2015-08-31T21:42:13Z

rebased on current master

miri64 · 2015-08-31T23:58:09Z

Please consider https://github.com/RIOT-OS/RIOT/pull/3749/files#diff-9b0aedc5fd4543061c862e245fe43c75R109

OlegHahm · 2015-09-01T09:20:47Z

I considered as soon as I saw the PR, but since the address is memcopied now anyway, this doesn't apply.

cgundogan · 2015-10-27T14:43:35Z

needs rebase

Yonezawa-T2 · 2015-12-10T05:51:43Z

sys/net/gnrc/network_layer/ndp/node/gnrc_ndp_node.c

-            (next_hop_size == sizeof(ipv6_addr_t))) {
-            next_hop_ip = &next_hop_actual;
-        }
+    next_hop_ip = gnrc_ipv6_ext_rh_next_hop(hdr);


This only modify a local pointer variable. We should memcpy like this.

ipv6_addr *tmp = gnrc_ipv6_ext_rh_next_hop(hdr); if (tmp != NULL) { memcpy(next_hop_ip, tmp, sizeof(ipv6_addr_t)); return true; }

Alternatively, change signature like this:

bool gnrc_ndp_node_next_hop_ipv6_addr(ipv6_addr_t **next_hop_ip, ...) { *next_hop_ip = gnrc_ipv6_ext_rh_next_hop(hdr); ... }

call site:

ipv6_addr_t *next_hop_ip; gnrc_ndp_node_next_hop_ipv6_addr(&next_hop_ip, ...);

Thanks for the hint. In fact this PR is very outdated and I just left it open to remind me of re-introducing it as a new one anytime soon.

miri64 · 2016-01-26T13:32:35Z

Needs rebase.

OlegHahm · 2016-01-26T22:53:52Z

I will abandon this for now.

OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation NSTF labels Aug 13, 2015

OlegHahm assigned miri64 Aug 13, 2015

OlegHahm mentioned this pull request Aug 13, 2015

Multi-Hop ping6 broken #3597

Closed

miri64 reviewed Aug 13, 2015
View reviewed changes

miri64 mentioned this pull request Aug 13, 2015

gnrc: Fix #3597 (multi-hop ping) #3624

Merged

OlegHahm mentioned this pull request Aug 14, 2015

netdev: introduce additional flags #3632

Closed

OlegHahm force-pushed the fix_next_hop_determination branch from a0fb9dd to 8b15b9c Compare August 14, 2015 14:58

OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 23, 2015

OlegHahm added 11 commits August 31, 2015 23:40

netopt: introduced link type (wired or wireless)

791b86f

gnrc netif: add link-type indicator for IPv6 IFs

9a3f1f7

shell: show link-type for IPv6 interfaces

6d9edfb

ndp: separate & clean next hop & l2 determination

bb14d9e

Determining the next hop and the corresponding L2 address are split up in two steps. No next hop look up is performed for link-local addresses any more. Destination cache has been disabled.

SQUASH ME: disable debug

95e14e9

SQUASH ME: fix broken formatting

083bb09

SQUASH ME: memcpy instead of returning local pointer

cf51edf

SQUASH ME: adapted to new naming scheme

475b509

SQUASH ME cosmetics

1e2b3dc

SQUASH ME fixed missing FIB table parameter

a359f68

SQUASH ME: check for on-link prefix on wired medium

fb47756

OlegHahm force-pushed the fix_next_hop_determination branch from 6806edd to fb47756 Compare August 31, 2015 21:42

OlegHahm modified the milestone: Release NEXT MAJOR Sep 2, 2015

OlegHahm modified the milestone: Release 2015.12 Nov 28, 2015

This was referenced Dec 9, 2015

xbee: fixed several bugs #4445

Merged

examples: add example of border router with XBee on native platform. #4448

Closed

fixes several bugs on GNRC network stack #4447

Merged

Yonezawa-T2 reviewed Dec 10, 2015
View reviewed changes

OlegHahm closed this Jan 26, 2016

OlegHahm added the State: archived State: The PR has been archived for possible future re-adaptation label Jan 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ndp: separate & clean next hop & l2 determination #3622

ndp: separate & clean next hop & l2 determination #3622

OlegHahm commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

miri64 commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

miri64 Aug 13, 2015

OlegHahm Aug 13, 2015

miri64 Aug 13, 2015

miri64 Aug 13, 2015

OlegHahm Aug 14, 2015

miri64 commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

miri64 commented Aug 13, 2015

miri64 commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

miri64 commented Aug 13, 2015

miri64 commented Aug 13, 2015

OlegHahm commented Aug 14, 2015

miri64 commented Aug 14, 2015

miri64 commented Aug 14, 2015

emmanuelsearch commented Aug 14, 2015

OlegHahm commented Aug 14, 2015

OlegHahm commented Aug 23, 2015

OlegHahm commented Aug 31, 2015

miri64 commented Aug 31, 2015

OlegHahm commented Sep 1, 2015

cgundogan commented Oct 27, 2015

Yonezawa-T2 Dec 10, 2015

OlegHahm Dec 11, 2015

miri64 commented Jan 26, 2016

OlegHahm commented Jan 26, 2016

ndp: separate & clean next hop & l2 determination #3622

ndp: separate & clean next hop & l2 determination #3622

Conversation

OlegHahm commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

miri64 commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

miri64 Aug 13, 2015

Choose a reason for hiding this comment

OlegHahm Aug 13, 2015

Choose a reason for hiding this comment

miri64 Aug 13, 2015

Choose a reason for hiding this comment

miri64 Aug 13, 2015

Choose a reason for hiding this comment

OlegHahm Aug 14, 2015

Choose a reason for hiding this comment

miri64 commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

miri64 commented Aug 13, 2015

miri64 commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

OlegHahm commented Aug 13, 2015

miri64 commented Aug 13, 2015

miri64 commented Aug 13, 2015

OlegHahm commented Aug 14, 2015

miri64 commented Aug 14, 2015

miri64 commented Aug 14, 2015

emmanuelsearch commented Aug 14, 2015

OlegHahm commented Aug 14, 2015

OlegHahm commented Aug 23, 2015

OlegHahm commented Aug 31, 2015

miri64 commented Aug 31, 2015

OlegHahm commented Sep 1, 2015

cgundogan commented Oct 27, 2015

Yonezawa-T2 Dec 10, 2015

Choose a reason for hiding this comment

OlegHahm Dec 11, 2015

Choose a reason for hiding this comment

miri64 commented Jan 26, 2016

OlegHahm commented Jan 26, 2016