Skip to content

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

Merged
miri64 merged 3 commits intoRIOT-OS:masterfrom
benpicco:gnrc_ipv6_nib-rio
Aug 3, 2021
Merged

gnrc_ipv6_nib: handle route information option and add config to add to final RAs#16568
miri64 merged 3 commits intoRIOT-OS:masterfrom
benpicco:gnrc_ipv6_nib-rio

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Jun 17, 2021 •
edited
Loading

Contribution description

This adds the ability to parse route information options in router and neighbor advertisements.

(Adding the option also the neighbor advertisements was proposed by draft-templin-6man-rio-redirect-08.txt and comes in handy for #16536)

Testing procedure

Issues/PRs references

requires #16557 to be useful

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jun 17, 2021
@benpicco benpicco added Type: new feature The issue requests / The PR implemements a new feature for RIOT and removed Area: sys Area: System labels Jun 17, 2021
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 18, 2021 •
edited
Loading

As this is based on a draft that was never published, please make this an optional feature (e.g. via pseudo-module) and mark both function and module as @experimental. According to the IANA, ICMPv6 type 24 is unassigned, but this might change. Sorry, did not finish my first coffee yet and should have read your original post more carefully. This is an ICMPv6 option which is specified in RFC 4191. Only the usage in NA is proposed by the draft. The latter should be made optional via pseudo-module.

@benpicco benpicco mentioned this pull request Jun 21, 2021
Merged
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 21, 2021

Should the route information option also be behind a pseudomodule? We didn't parse this before but I see my Fritz!Box adding it to it's RAs

2021-06-21 12:34:56,731 # 2001:16b8:4564:4f00::/64 dev #6
2021-06-21 12:34:56,737 # 2001:16b8:4564:4f00::/56 via fe80::de39:6fff:fe6a:6980 dev #6

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 21, 2021

Should the route information option also be behind a pseudomodule?

I am not sure how you mean that. IMHO reading the RIO from an NA should be enough to be optional, but of course from a memory saving perspective parsing the RIO in the first place could be optional as well.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@github-actions github-actions bot added the Area: sys Area: System label Jun 21, 2021
@benpicco 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 22, 2021
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 24, 2021

hm looks like we are overflowing ROM on some boards now.
Should I make this optional or should I just add those boards to Makefile.ci?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 24, 2021

Should I make this optional or should I just add those boards to Makefile.ci?

Let's make it an optional pseudo-module then, similar to gnrc_ipv6_nib_dns (but keep the NA handling as a separate level of optionality with the switch).

@github-actions github-actions bot added Area: build system Area: Build system Area: examples Area: Example Applications labels Jun 24, 2021
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 24, 2021

Ok, I added gnrc_ipv6_nib_rio

miri64
miri64 reviewed Jun 24, 2021
View reviewed changes
miri64
miri64 previously approved these changes Jun 24, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. I used my home setup to test (OpenWRT's odhcpd also sends RIOs in its RAs). Without the module:

XXXX:XXXX:XXXX:1f10::/64 dev #6
XXXX:XXXX:XXXX:1f10::/62 dev #7
default* via fe80::7810:c2ff:fe67:b226 dev #6

with the module

XXXX:XXXX:XXXX:1f14::/62 dev #7
XXXX:XXXX:XXXX:1f10::/64 dev #6
XXXX:XXXX:XXXX:1f00::/56 via fe80::7810:c2ff:fe67:b226 dev #6
default* via fe80::7810:c2ff:fe67:b226 dev #6

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 24, 2021

Not sure how to test the NA stuff though.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 24, 2021

Please squash.

@benpicco benpicco force-pushed the gnrc_ipv6_nib-rio branch from 248bc74 to 18e88fd Compare June 24, 2021 14:50
miri64
miri64 reviewed Jun 24, 2021
View reviewed changes
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 25, 2021

Turns out we can do without the RIO inside the neighbor advertisements, we just have to delay setting gnrc_ipv6_nib_change_rtr_adv_iface(upstream_netif, false); and thereby sending the 'final' RA.

This is actually useful for DHCPv6 too, if we don't send the RIO the upstream router may think the nodes on the downstream subnet are on-link.

@benpicco benpicco dismissed miri64’s stale review June 27, 2021 16:49

added new commits

@benpicco benpicco force-pushed the gnrc_ipv6_nib-rio branch from d3744bb to 63fbfaa Compare June 27, 2021 16:52
benpicco
benpicco commented Jul 7, 2021
View reviewed changes
benpicco
benpicco commented Jul 7, 2021
View reviewed changes
@github-actions github-actions bot removed the Area: examples Area: Example Applications label Jul 7, 2021
miri64
miri64 reviewed Jul 8, 2021
View reviewed changes
miri64
miri64 reviewed Jul 8, 2021
View reviewed changes
@benpicco benpicco force-pushed the gnrc_ipv6_nib-rio branch 2 times, most recently from bc386ee to 7c96e96 Compare July 12, 2021 17:27
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@benpicco benpicco force-pushed the gnrc_ipv6_nib-rio branch from 7c96e96 to c9408a0 Compare July 22, 2021 13:51
@benpicco benpicco force-pushed the gnrc_ipv6_nib-rio branch from c9408a0 to 586f13e Compare July 22, 2021 13:54
@benpicco benpicco added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jul 22, 2021
@benpicco benpicco mentioned this pull request Jul 22, 2021
Merged
@benpicco benpicco removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jul 22, 2021
@benpicco benpicco force-pushed the gnrc_ipv6_nib-rio branch from 586f13e to 90d836e Compare July 23, 2021 12:47
@benpicco benpicco mentioned this pull request Jul 29, 2021
Merged
miri64
miri64 reviewed Aug 2, 2021
View reviewed changes
benpicco
benpicco commented Aug 2, 2021
View reviewed changes
@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 2, 2021
@benpicco benpicco mentioned this pull request Aug 2, 2021
Merged
miri64
miri64 requested changes Aug 2, 2021
View reviewed changes
@miri64 miri64 changed the title gnrc_ipv6_nib: handle route information option gnrc_ipv6_nib: handle route information option and add config to add to final RAs Aug 2, 2021
miri64
miri64 approved these changes Aug 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Please squash!

@benpicco
net/gnrc/ipv6/nib: add option to include RIO with final RA
71ae768 
Sending a RA with ltime = 0 does not get us added to the default router
list, but the options (and therefore the RIO) are still parsed.

This even appears to work with Linux as a receiver.
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 2, 2021
@benpicco benpicco force-pushed the gnrc_ipv6_nib-rio branch from 9351ffd to 71ae768 Compare August 2, 2021 19:45
@miri64 miri64 merged commit 8492bd7 into RIOT-OS:master Aug 3, 2021
@benpicco benpicco deleted the gnrc_ipv6_nib-rio branch August 3, 2021 14:09
@benpicco benpicco mentioned this pull request Aug 13, 2021
Merged
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2021.10

Development

Successfully merging this pull request may close these issues.

3 participants

@benpicco @miri64 @MrKevinWeiss