ospf6d: Limit the number of interface addresses being supported in ospfv3 #8622
Conversation
|
this fix addresses issue 5773 |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 arm8 part 1: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 1: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests Ubuntu 18.04 arm8 part 1: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 1: No useful log foundCLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18819/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsCLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
There was a problem hiding this comment.
Two other questions, neither of which might be important. :-)
Should there be some note in the docs about this limit?
What happens if the user reduces the MTU on an interface from jumbo to something smaller and there are too many addresses? Should there be a warning and the user not be allowed to reduce the MTU? Or should we print a warning and intentionally crash (since we're going to crash anyway)?
|
If you reduce the MTU we will go back to only advertising the first 100 addresses added on the interface. Yes I agree that this limit should be documented. I will add that. |
The code had no limits on addresses configured on an interface running ospf6d. The code would crash when more than 100 addresses were added. This change limits the number of interface address to 100 if mtu is set to the default value. If the mtu is set to a jumbo packet size or larger we will support 200 interface addresses. Signed-off-by: Lynne Morrison <lynne@voltanet.io>
Signed-off-by: Lynne Morrison <lynne@voltanet.io>
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO6U18AMD64-18936/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 6: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18936/artifact/TOPO6U18AMD64/ErrorLog/log_topotests.txt Topotests debian 10 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9DEB10AMD64-18936/test Topology Tests failed for Topotests debian 10 amd64 part 9: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18936/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests Ubuntu 18.04 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO6U18AMD64-18936/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 6: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18936/artifact/TOPO6U18AMD64/ErrorLog/log_topotests.txt Topotests debian 10 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9DEB10AMD64-18936/test Topology Tests failed for Topotests debian 10 amd64 part 9: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18936/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests debian 10 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9DEB10AMD64-18939/test Topology Tests failed for Topotests debian 10 amd64 part 9: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18939/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests debian 10 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9DEB10AMD64-18939/test Topology Tests failed for Topotests debian 10 amd64 part 9: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18939/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt |
|
ci:rerun |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18942/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings |
|
So I stumbled on this while doing other work and sorry to say but it's just wrong in several places and on several levels :(. To begin with, the entire approach is not correct. When there are too many addresses to fit into one LSA, a second intra-prefix LSA should just be added with the next batch of addresses. And another if that's still not enough. (Bonus points for an implementation that does this without fragment wandering.) But even if we wanted to limit addresses, the code has several issues; adding separate comments for that on the code itself. I'll also open a PR to revert this shortly. (Yes that might get us back to a crash, but whether this is better is debatable since it could [rarely] break the entire network rather than just one router.) |
| * size of OSPFv3 packet | ||
| */ | ||
| count++; | ||
| if (count >= max_addr_count) |
There was a problem hiding this comment.
This is absolutely the wrong place to do a check like this; oi->route_connected is just an internal table of prefixes. There's no semantic connection to LSAs just yet.
| for (route = ospf6_route_head(oi->route_connected); route; | ||
| route = ospf6_route_next(route)) { | ||
| /* connected prefix to advertise, number of interface addresses | ||
| * supported is based on MTU size of OSPFv3 packets |
There was a problem hiding this comment.
This may be the only place where limiting the number of addresses is correct; RFC5340 suggests only one Link LSA for each link. However, looking at the MTU is still wrong - https://datatracker.ietf.org/doc/html/rfc5340#appendix-A.1 clearly suggests that OSPFv3 should just rely on IPv6 fragmentation for larger packets. So the limit is 64k minus worst case headers.
| @@ -1068,8 +1080,14 @@ int ospf6_intra_prefix_lsa_originate_stub(struct thread *thread) | |||
| zlog_debug(" Interface %s:", oi->interface->name); | |||
|
|
|||
| /* connected prefix to advertise */ | |||
| for (route = ospf6_route_head(oi->route_connected); route; | |||
| route = ospf6_route_best_next(route)) { | |||
| if (oi->ifmtu >= OSPF6_JUMBO_MTU) | |||
There was a problem hiding this comment.
This is completely wrong here. The function is originating an Intra-Prefix LSA which is flooded through the entire area. The interface (whose addresses we're adding) 's MTU is irrelevant, if anything we would need to know the minimum MTU of any link in the entire area. But as noted above, even that does not matter since it's supposed to be fragmented.
Since we can originate multiple Intra-Prefix LSAs without problem, the limit here is either 64k or 1280 (not 1500) minus headers depending on whether we want to avoid fragmentation or not. Going with 1280 has the downside of emitting a larger number of LSAs; an argument could be made for some middle value (to reduce the number of fragments and thus the impact of a single lost fragment.)
| vty_out(vty, | ||
| "can not configure OSPFv3 on if %s, must have less than %d interface addresses but has %d addresses\n", | ||
| ifp->name, OSPF6_MAX_IF_ADDRS, ipv6_count); | ||
| return CMD_WARNING_CONFIG_FAILED; |
There was a problem hiding this comment.
Please never refuse config based on state. State can always be ordered differently, so any look at state while processing config can introduce a race. If the addresses are added slowly for whatever reasons, we would accept the config here, but if ospf6d is slow we refuse it. Also, the user might never see the warning if they configure ospf6d first.
| ipv6_count = connected_count_by_family(c->ifp, AF_INET6); | ||
| if (oi->ifmtu == OSPF6_DEFAULT_MTU && ipv6_count > OSPF6_MAX_IF_ADDRS) { | ||
| zlog_warn( | ||
| "Zebra Interface : %s has too many interface addresses %d only support %d, increase MTU", |
There was a problem hiding this comment.
For consistency, you theoretically would need to disable the interface when you hit this. But that's pretty bad too.
| #define OSPF6_MAX_IF_ADDRS 100 | ||
| #define OSPF6_MAX_IF_ADDRS_JUMBO 200 | ||
| #define OSPF6_DEFAULT_MTU 1500 | ||
| #define OSPF6_JUMBO_MTU 9000 |
There was a problem hiding this comment.
Neither 1500 nor 9000 are valid MTU numbers to hardcode here. You can either assume 1280 == IPv6 minimum, or 64k (minus some bytes) for fragmentation. For anything else, the only correct approach is to rely on actual MTU reported by the interface.
The code had no limits on addresses configured on an interface running
ospfv3. The code would crash when more than 100 addresses were added.
This change limits the number of interface address to 100 if mtu is set
to the default value. If the mtu is set to a jumbo packet size or larger
we will support 200 interface addresses.
Signed-off-by: Lynne Morrison lynne@voltanet.io