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.

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

ipv6/nib: 6LBR should not send RS on their downstream interface #19335

Merged
merged 1 commit into from May 23, 2023

Conversation

fabian18
Copy link
Contributor

Contribution description

I think the 6LBR is not supposed to send RS on its downstream 6LO interface.
Consequently, I think the 6LBR interface is also not supposed to handle RA on the downstream interface.
Code existed to exclude the border router but it was not fully working.

Testing procedure

diff --git a/sys/net/gnrc/network_layer/ndp/gnrc_ndp.c b/sys/net/gnrc/network_layer/ndp/gnrc_ndp.c
index ec9e18b3a7..cafd6dd644 100644
--- a/sys/net/gnrc/network_layer/ndp/gnrc_ndp.c
+++ b/sys/net/gnrc/network_layer/ndp/gnrc_ndp.c
@@ -441,7 +441,7 @@ void gnrc_ndp_rtr_sol_send(gnrc_netif_t *netif, const ipv6_addr_t *dst)
     if (dst == NULL) {
         dst = &ipv6_addr_all_routers_link_local;
     }
-    DEBUG("ndp: send router solicitation (iface: %" PRIkernel_pid
+    printf("ndp: send router solicitation (iface: %" PRIkernel_pid
           ", dst: %s)\n", netif->pid,
           ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
     gnrc_netif_acquire(netif);

In master you´ll see that the border router interface sends RS.
With this PR it does not.

Issues/PRs references

@fabian18
Copy link
Contributor Author

@miri64 what do you think about that?

@fabian18 fabian18 added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Feb 28, 2023
@miri64
Copy link
Member

miri64 commented Feb 28, 2023

I think the border router should set !RTR_ADV on start-up and honor that (what it apparently does not). While an apparent use case might not exist (yet) a border router is not forbidden by the standard to solicitate routers. From looking at the code it seems more that the if !RTR_ADV || "6LN, but no 6LBR" logic is rather broken at the moment.

@miri64
Copy link
Member

miri64 commented Feb 28, 2023

(or RTR_ADV is not unset at the startup of the border router)

@benpicco
Copy link
Contributor

Isn't the root issue that we set CONFIG_GNRC_IPV6_NIB_ARSM globally and not on a per-interface basis?

So a border router (with a non 6lo upstream interface) will always have this enabled. This also causes issues with 6lo nodes that don't have CONFIG_GNRC_IPV6_NIB_ARSM set which leads to workarounds like #16988

@miri64
Copy link
Member

miri64 commented Feb 28, 2023

Isn't the root issue that we set CONFIG_GNRC_IPV6_NIB_ARSM globally and not on a per-interface basis?

If the Address Resolution is causing RS (part of router discovery) to be sent then something is definitely wrong...

@benpicco
Copy link
Contributor

benpicco commented Mar 1, 2023

But how is RTR_ADV related to router solicitations?
It should only solicit routers on the upstream interface, disregarding whether it advertises itself as a router on the downstream interface or not.

@miri64
Copy link
Member

miri64 commented Mar 1, 2023

But how is RTR_ADV related to router solicitations?

I thought, that setting RTR_ADV would mean Router Solicitations would not be sent, but after reading the relevant parts of RFC 4861, I can not confirm that. But still, the logic seems broken. Router Discovery has no notion of upstream/downstream, as it is supposed to work for any scenario, not just edge routers. IMHO, the logic of those if-conditions should express “if !RTR_ADV || (6LN but not 6LBR): do something RS related” (as it was supposed in master), the way @fabian18 changed it now it says “if !6LBR && (!RTR_ADV || 6LN): do something RS related”, which doesn't reflect what the expression wants to, well, express.

Mind you, I am not against changing things here, I just think the expression as it is in this PR.


After some thinking about this, the RHS expression should express the following logic tabular

6LN !6LBR (6LN but not 6LBR)
0 0 0
0 1 0
1 0 1
1 1 0

So the negation of a conditional !(6LN -> !6LBR). In boolean logic, that's !(!6LN || !6LBR) and with De Morgan's law that's surprisingly 6LN && 6LBR. So could we, instead of “if !6LBR && (!RTR_ADV || 6LN): do something RS related” unintuitively say “if (!RTR_ADV && 6LN && 6LBR): do something RS related” to fix the logic?

@fabian18
Copy link
Contributor Author

fabian18 commented Mar 2, 2023

I think the truth table is wrong. As you wrote it, the two last rows of the last column must be swapped.
Checking for 6LN && 6LBR is unnecessary because a 6LBR is of course a 6LN.

RTR_ADV from my point of view only means: "I answer to RS". (and send multicast RA in non 6LN case)
Imagine some node is RTR_ADV. To maybe find a new router it could still be required to send an RS, I think.

@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 Mar 15, 2023
@riot-ci
Copy link

riot-ci commented Mar 15, 2023

Murdock results

✔️ PASSED

e0d64b1 ipv6/nib: 6LBR should not send RS on their downstream interface

Success Failures Total Runtime
6945 0 6946 11m:04s

Artifacts

@benpicco
Copy link
Contributor

This PR fixes a real problem when gnrc_ipv6_auto_subnets is used: When the border router wrongfully sends a router solicitation downstream and a 6lr node answers with a full router advertisement containing a Prefix Information Option - that the 6lbr than uses to create a subnet on it's upstream interface - which it definitely should not do.

Preventing the 6lbr from sending the useless router solicitation fixes that issue.

@fabian18
Copy link
Contributor Author

bors merge

@bors bors bot merged commit 3469fce into RIOT-OS:master May 23, 2023
26 checks passed
@bors
Copy link
Contributor

bors bot commented May 23, 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.

@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants