Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gnrc_ipv6_nib: disable router advertisements on interface startup #19920

Merged
merged 2 commits into from
Sep 13, 2023
Merged

gnrc_ipv6_nib: disable router advertisements on interface startup #19920

merged 2 commits into from
Sep 13, 2023

Conversation

benpicco
Copy link
Contributor

Contribution description

Router Advertisements are enabled automatically for routing nodes as soon as they obtain a prefix (e.g. via DHCPv6, UHCP, …).

Enabling router advertisements without a prefix to advertise is wrong and causes no router solicitations to be send on an upstream interface.

Also from https://www.rfc-editor.org/rfc/rfc4861#section-6.2.1

Note that AdvSendAdvertisements MUST be FALSE by
default so that a node will not accidentally start
acting as a router unless it is explicitly
configured by system management to send Router
Advertisements.

Testing procedure

examples/gnrc_networking does now instantly obtain an address on a network with an upstream router.

examples/gnrc_border_router had this disabled already because an automatic prefix configuration method was used.

tests/net/gnrc_rpl shows that RPL operation is not affected.

Issues/PRs references

@benpicco
gnrc_ipv6_nib: disable router advertisements on interface startup
0572a95 
Router Advertisements are enabled automatically for routing nodes
as soon as they obtain a prefix (e.g. via DHCPv6, UHCP, …).

Enabling router advertisements without a prefix to advertise is
wrong and causes no router solicitations to be send on an upstream
interface.

Also from https://www.rfc-editor.org/rfc/rfc4861#section-6.2.1

	Note that AdvSendAdvertisements MUST be FALSE by
	default so that a node will not accidentally start
	acting as a router unless it is explicitly
	configured by system management to send Router
	Advertisements.
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Sep 11, 2023
@benpicco benpicco requested a review from miri64 September 11, 2023 14:10
@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 Sep 11, 2023
@benpicco benpicco requested a review from OlegHahm September 11, 2023 14:10
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Sep 11, 2023
@miri64
Copy link
Member

miri64 commented Sep 11, 2023

How is the RTR_ADV flag set now? When the interface has isRouter set and when it has a prefix assigned?

@benpicco
Copy link
Contributor Author

benpicco commented Sep 11, 2023 •
edited
Loading

How is the RTR_ADV flag set now?

gnrc_ipv6_nib_change_rtr_adv_iface() is called by dhcpv6_client_ia_pd, gnrc_uhcp, gnrc_ipv6_auto_subnets and gnrc_ipv6_static_addr when a prefix is added to a downstream/advertising interface. (That's why those modules would set CONFIG_GNRC_IPV6_NIB_ADV_ROUTER to 0 already)

If a prefix is added manually with ifconfig, the RTR_ADV flag now also has to be set manually.

@riot-ci
Copy link

riot-ci commented Sep 11, 2023 •
edited
Loading

Murdock results

✔️ PASSED

c337992 net/gnrc/rpl: enable RTR_ADV on RPL interface

Success Failures Total Runtime
7968 0 7968 14m:13s

Artifacts

@miri64
Copy link
Member

miri64 commented Sep 11, 2023

gnrc_ipv6_nib_change_rtr_adv_iface() is called by dhcpv6_client_ia_pd, gnrc_uhcp, gnrc_ipv6_auto_subnets and gnrc_ipv6_static_addr when a prefix is added to a downstream/advertising interface. (That's why those modules would set CONFIG_GNRC_IPV6_NIB_ADV_ROUTER to 0 already)

So not when a prefix is configured by a router advertisement? How will downstream nodes (e.g. in a RPL) have this flag set now?

@miri64
Copy link
Member

miri64 commented Sep 11, 2023

RPL might be a bad example, since it has its own prefix delegation mechanism, but the question stands: will a downstream router set this flag, once it obtains a prefix from upstream?

@benpicco
Copy link
Contributor Author

What good does it do if a downstream router sets this flag if it has no way to route the packets? (Ok, upstream routes will always work, but how will e.g. the border router know it has to route a packet to the downstream router via the next hop router?)

@miri64
Copy link
Member

miri64 commented Sep 11, 2023

What good does it do if a downstream router sets this flag if it has no way to route the packets? (Ok, upstream routes will always work, but how will e.g. the border router know it has to route a packet to the downstream router via the next hop router?)

That's what a routing protocol is for. But if it is not enabled for a router, the current state seems to be broken. This should be fixed, before this PR introduces that regression.

@benpicco
Copy link
Contributor Author

That's what a routing protocol is for.

But then IMHO it should still only be enabled if that routing protocol (which one? we only have RPL) is active.

Having routers send router advertisements for prefixes that they can't route just leads to chaos.

@miri64
Copy link
Member

miri64 commented Sep 11, 2023

Having routers send router advertisements for prefixes that they can't route just leads to chaos.

with prefixes, not for ;-).

The question is: Shouldn't let the routing protocol leave such a central functionality. This may well lead to technical debt with contributors wondering why routers don't advertise themselves as routers.

Also: We still don't have RPL routers that advertise themselves as routers, if I understand your previous statement correctly.

@benpicco
Copy link
Contributor Author

benpicco commented Sep 12, 2023 •
edited
Loading

Ok so RPL still needs RTR_ADV enabled - maybe that's all the heuristic we need.
It doesn't make sense to send router advertisements if we can't route things, so only enabling it on an interface where a routing protocol (RPL) is configured sounds like it would make sense.

miri64
miri64 approved these changes Sep 12, 2023
View reviewed changes
Copy link
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, but only if you handle the tickets that will inevitably come from routing protocol implementors/users ;-P

@benpicco benpicco added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Sep 12, 2023
@benpicco
Copy link
Contributor Author

benpicco commented Sep 12, 2023 •
edited
Loading

If there is some external user implementing an out-of-tree routing protocol, they can just call gnrc_ipv6_nib_change_rtr_adv_iface() - I'll mark it as an API change so it shows up in the release notes.

@miri64
Copy link
Member

miri64 commented Sep 12, 2023

they can just call gnrc_ipv6_nib_change_rtr_adv_iface()

Might be a bit clunky if it is a UDP-based routing protocol on top of sock (dusting of ye good olde AODV2), but let's address this, if it comes to it.

@miri64
Copy link
Member

miri64 commented Sep 12, 2023

bors merge

bors bot added a commit that referenced this pull request Sep 12, 2023
@bors @benpicco
Merge #19920
e8f7b45 
19920: gnrc_ipv6_nib: disable router advertisements on interface startup r=miri64 a=benpicco





Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
@bors
Copy link
Contributor

bors bot commented Sep 13, 2023

Build failed:

@miri64
Copy link
Member

miri64 commented Sep 13, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Sep 13, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit d9367b1 into RIOT-OS:master Sep 13, 2023
@benpicco benpicco deleted the gnrc_ipv6_nib-no_RTR_ADV branch September 13, 2023 11:22
@benpicco
Copy link
Contributor Author

benpicco commented Sep 20, 2023 •
edited
Loading

Looks like my confusion is not completely unwarranted:

Thus, multihop prefix distribution (the ABRO) and the 6LoWPAN Context Option (6CO) for distributing header compression contexts go hand in hand. If substitution is intended for one of them, then both of them MUST be substituted.

So when we distribute the prefix via RPL message, we wouldn't need to do multihop prefix distribution. And while compression contexts sure would be helpful on the RPL side (DAO contains a list of full 128 bit IPv6 addresses), the only reference I found is an expired draft - or did I miss something there?

But I suppose that means we should just set CONFIG_GNRC_RPL_WITHOUT_PIO by default then, to avoid partial substitution.

@MrKevinWeiss MrKevinWeiss added this to the Release 2023.10 milestone Nov 17, 2023
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

@OlegHahm OlegHahm Awaiting requested review from OlegHahm

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

Assignees
No one assigned
Labels
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 Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2023.10
Development

Successfully merging this pull request may close these issues.
4 participants
@benpicco @miri64 @riot-ci @MrKevinWeiss