gnrc_ipv6_nib: clean up _resolve_addr() #18939

benpicco · 2022-11-20T19:49:14Z

Contribution description

I'm still trying to find a proper fix for #16988 (border router does ARSM on the 6lo interface) but still no avail.

But as a first step, clean up _resolve_addr() by getting rid of unececary pre-processor conditionals and moving logic blocks to separate functions.

This makes it easier to figure out what's actually going on.

There should be no changes to the logic.

Testing procedure

All tests should continue yielding the same results as before.

Issues/PRs references

benpicco · 2022-11-20T19:52:39Z

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

+        return false;
+    }
+
+    if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)) {


I have a feeling that this should in fact be

Suggested change

if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)) {

if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM) && !gnrc_netif_is_6lo(netif)) {

but I'll leave this PR to cleanups only.

No, it shoud still be possible to activate the ARSM optionally on 6Lo nodes.

(also resolution by address generation reversal is also a nice feature for non-6Lo-nodes to have (and will become necessary with SCHC at some point too).

No, it shoud still be possible to activate the ARSM optionally on 6Lo nodes.

But that would be a separate config then, right? Because right now we only have CONFIG_GNRC_IPV6_NIB_ARSM which globally turns on ARSM on all interfaces.

So the BR will do ARSM on the 6lo interface (because it needs it on the upstream interface) but the 6lo nodes do not.

In my opinion, CONFIG_GNRC_IPV6_NIB_ARSM should even be enabled for all 6LN. Because our CONFIG_GNRC_IPV6_NIB_ARSM is actually responsible for two things:

address resolution obviously
(Note: with private addresses, 6LN cannot reverse translate IPv6 addresses to L2 addresses, so they must look in the NC anyway. But this is a bit off-topic here currently.)

and Neighbor Unreachable Detection

6LN actually do NUD and additionally include an ARO option RFC6775.

Hosts send unicast NS messages to register their IPv6 addresses, and also to do NUD to verify that their default routers are still reachable.

For some things like joining the Solicited-node multicast address and multicast L2 address resolution, we just exclude 6LN interfaces.

Neighbor Unreachable Detection

hm might this be the reason for #17327

very hard to tell... To be honest I am not very familiar with RPL. I have to catch up here.

benpicco · 2022-11-20T19:55:40Z

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

+        return false;
+    }
+
+    if (!IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)) {


same here

Suggested change

if (!IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)) {

if (!IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM) || gnrc_netif_is_6lo(netif)) {

riot-ci · 2022-11-20T22:45:41Z

Murdock results

✔️ PASSED

64a2fbc gnrc_ipv6_nib: clean up _resolve_addr()

Success	Failures	Total	Runtime
6770	0	6770	14m:01s

Artifacts

Documentation preview

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

fabian18

Nice to see a clean up. I looked at it for a while and did not find a semantic change.

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

fabian18 · 2022-11-22T12:39:42Z

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

-        res = true;
+
+    /* directly resolve 6lo address */
+    if (_resolve_addr_from_ipv6(dst, netif, nce)) {


For consistency, this if clause could use a nice DEBUG() message too.

I wonder if this should be moved above _resolve_addr_from_nc()

could be, maybe, but not necessarily.

static bool _resolve_addr_from_nc(gnrc_netif_t *netif, gnrc_ipv6_nib_nc_t *nce, const ipv6_addr_t * dst) { /* lookup _nib_onl_entry_t *entry by dst and netif */ /* if exists get L2 address from NC */ /* else if 6LN and link-local do reverse translation */ }

And if _resolve_addr_from_nc() fails to resolve the L2 address, we know we don´t have an NC entry and we have to add one, or it existed but was UNREACHABLE. Also having (netif + dst) and entry as input is a bit redundant.
... I don´t know, maybe the clean up could propagate upwards, but not without changing too much I guess.

I'm not sure I understand what you want me to do 😅

Sorry I misread your last comment. I thought you wanted to merge _resolve_addr_from_nc() and _resolve_addr_from_ipv6() into one function. Then I thought about how this would look like.

Ok, moving _resolve_addr_from_ipv6() above _resolve_addr_from_nc() and keeping them as they are now,
would mean that every link-local destination address would be resolved immediately by reverse translation.
This would mean that link local addresses would not have to be registered. But this is somewhat required if the link local address is a private address (CGA or SLAAC pivacy extension) .

Let´s say R is the upstream router of host H. H implements the SLAAC privacy extension and builds a private link local address that it registers to R where due to an SLLA option an NCA is created there.
If R would not examine the NCE when it sends something to the private SLAAC address of H, it would put the wrong L2 address in the frame.

In fact, in my SEND branch, I added this:

/* If any IPv6 privacy extension is used, we cannot derive the L2 address from the IP address */ bool res = !IS_USED(MODULE_IPV6_CGA) && (netif != NULL) && gnrc_netif_is_6ln(netif) && ipv6_addr_is_link_local(dst);

to _resolve_addr_from_ipv6().

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

fabian18

I once again looked through the changes and found no change in semantics.
Are we going to merge the clean up or is there something more to discuss regarding this PR?

miri64 · 2023-01-09T12:53:45Z

My comments were addressed.

maribu

bors merge

bors · 2023-01-10T15:24:35Z

Build succeeded:

gnrc_ipv6_nib: factor out resolve from nc

0886ad4

benpicco requested review from miri64, haukepetersen, PeterKietzmann and cgundogan as code owners November 20, 2022 19:49

github-actions bot added Area: network Area: Networking Area: sys Area: System labels Nov 20, 2022

benpicco requested a review from fabian18 November 20, 2022 19:51

benpicco commented Nov 20, 2022

View reviewed changes

benpicco force-pushed the gnrc_nib_resolve_addr-cleanup branch from d925acb to 71a01df Compare November 20, 2022 19:55

benpicco commented Nov 20, 2022

View reviewed changes

benpicco added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Nov 20, 2022

gnrc_ipv6_nib: factor out enqueue on resolve

2e2414c

benpicco force-pushed the gnrc_nib_resolve_addr-cleanup branch from 71a01df to 14fc375 Compare November 20, 2022 20:00

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 20, 2022

miri64 reviewed Nov 21, 2022

View reviewed changes

sys/net/gnrc/network_layer/ipv6/nib/nib.c Outdated Show resolved Hide resolved

fabian18 reviewed Nov 22, 2022

View reviewed changes

benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 22, 2022

fabian18 reviewed Nov 29, 2022

View reviewed changes

sys/net/gnrc/network_layer/ipv6/nib/nib.c Outdated Show resolved Hide resolved

fabian18 reviewed Jan 9, 2023

View reviewed changes

gnrc_ipv6_nib: clean up _resolve_addr()

64a2fbc

benpicco force-pushed the gnrc_nib_resolve_addr-cleanup branch from 9cff238 to 64a2fbc Compare January 9, 2023 13:04

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 9, 2023

maribu approved these changes Jan 10, 2023

View reviewed changes

bors bot merged commit 53176f7 into RIOT-OS:master Jan 10, 2023

benpicco deleted the gnrc_nib_resolve_addr-cleanup branch January 10, 2023 15:25

jia200x added this to the Release 2023.04 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gnrc_ipv6_nib: clean up _resolve_addr() #18939

gnrc_ipv6_nib: clean up _resolve_addr() #18939

benpicco commented Nov 20, 2022 •

edited

Loading

benpicco Nov 20, 2022 •

edited

Loading

miri64 Nov 21, 2022

miri64 Nov 21, 2022

benpicco Nov 21, 2022

fabian18 Nov 22, 2022 •

edited

Loading

benpicco Nov 22, 2022

fabian18 Nov 22, 2022

benpicco Nov 20, 2022

riot-ci commented Nov 20, 2022 •

edited

Loading

fabian18 left a comment

fabian18 Nov 22, 2022

benpicco Nov 22, 2022

fabian18 Nov 22, 2022

benpicco Nov 28, 2022

fabian18 Nov 29, 2022

fabian18 left a comment

miri64 commented Jan 9, 2023

maribu left a comment

bors bot commented Jan 10, 2023

	if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)) {
	if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM) && !gnrc_netif_is_6lo(netif)) {

	if (!IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)) {
	if (!IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM) \|\| gnrc_netif_is_6lo(netif)) {

gnrc_ipv6_nib: clean up _resolve_addr() #18939

gnrc_ipv6_nib: clean up _resolve_addr() #18939

Conversation

benpicco commented Nov 20, 2022 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

benpicco Nov 20, 2022 • 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

fabian18 Nov 22, 2022 • 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

riot-ci commented Nov 20, 2022 • edited Loading

Murdock results

Artifacts

fabian18 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

fabian18 left a comment

Choose a reason for hiding this comment

miri64 commented Jan 9, 2023

maribu left a comment

Choose a reason for hiding this comment

bors bot commented Jan 10, 2023

benpicco commented Nov 20, 2022 •

edited

Loading

benpicco Nov 20, 2022 •

edited

Loading

fabian18 Nov 22, 2022 •

edited

Loading

riot-ci commented Nov 20, 2022 •

edited

Loading