ip6: punt received router advertisements to control plane - #658
Conversation
| // Grout does not process router advertisements itself. | ||
| // Punt them to the control plane so that a routing daemon | ||
| // (e.g. FRR) running on top of grout can consume them. BGP | ||
| // unnumbered peering relies on received RAs to discover the | ||
| // peer link-local next hop; without this the packets would | ||
| // be dropped as unsupported and the session would never come | ||
| // up. | ||
| next = ROUTER_ADVERT; | ||
| break; |
There was a problem hiding this comment.
Your approach is fine but I would prefer if we could add an intermediate node to check for the RA packets compliance with RFCs. See my other PR for reference:
f5309fe#diff-ae0101fec65d9c9f9e7645dff184ef9454a808762ad3162dbe93d129874eb37cR41-R55
| # This test checks BGP unnumbered (RFC 5549) peering between an FRR+grout setup | ||
| # and a pure FRR BGP peer. Unnumbered peering relies on IPv6 Router | ||
| # Advertisements to discover the peer link-local next hop. grout must punt the | ||
| # RAs it receives on a port to that port's control plane so the FRR instance | ||
| # running on top of grout can consume them; otherwise the session never leaves | ||
| # the Idle state. At the end of the configuration phase grout must be able to | ||
| # ping the loopback advertised by the "bgp-peer" namespace over the route | ||
| # learned through the unnumbered session. | ||
| # | ||
| # .-------..-------------. .-------------------. | ||
| # | zebra || grout | | netns "bgp-peer" | | ||
| # '-------'| | | .------. | | ||
| # .------. | .------------. .------------. | bgpd | | | ||
| # | bgpd | | | p0 | net_tap | x-p0 | '------' | | ||
| # '------' | | (LL) +-------------+ (LL) | .-------. | | ||
| # .------. '------------' '------------' | zebra | | | ||
| # | main | | | '-------' | | ||
| # '------' | | .----------. | | ||
| # | ping <------------------------------------>| lo | | | ||
| # '------------' | | 16.0.0.1 | | | ||
| # | '----------' | | ||
| # '-------------------' |
There was a problem hiding this comment.
Also, the issue we have at the moment is that the test you added will fail all the time until there is a proper fix in zebra.
I think my approach in FRRouting/frr#22773 is correct but I'd like some upstream reviews.
In the meantime, we "could" keep a local patch for FRR 487aac5 so that we can test it in CI.
There was a problem hiding this comment.
Sounds good, I can cherry pick your commit after rebasing :)
afa9a6d to
e8de0cd
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an IPv6 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/ip6/datapath/ndp_ra_input.c`:
- Around line 43-57: Update the Router Advertisement validation in the RA input
handler before l4_loopback_output is selected: parse the option sequence
following the fixed RA header, reject any option with a zero Length, and reject
any option whose encoded size exceeds the remaining payload so invalid RAs are
dropped rather than delivered. Add packet tests covering both malformed option
cases.
In
`@subprojects/packagefiles/frr/10.5-zebra-route-IPv4-link-local-neighbor-updates-through.patch`:
- Around line 58-70: The neighbor delete operation uses
dplane_rem_neigh_delete() which marks the delete as remote, but the subsequent
add operation uses dplane_local_neigh_add() which installs a local neighbor.
This class mismatch can leave the local neighbor state installed after refresh
or delete. In both
subprojects/packagefiles/frr/10.5-zebra-route-IPv4-link-local-neighbor-updates-through.patch
(lines 58-70) and
subprojects/packagefiles/frr/10.7-zebra-route-IPv4-link-local-neighbor-updates-through.patch
(lines 58-70), replace the dplane_rem_neigh_delete() call with a local or
class-neutral neighbor delete operation that matches the local neighbor class
used in the dplane_local_neigh_add() call below it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d1b20a51-3cd9-4742-9ece-7b7581ad4e85
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (10)
modules/ip6/datapath/icmp6_input.cmodules/ip6/datapath/meson.buildmodules/ip6/datapath/ndp_ra_input.csmoke/bgp_unnumbered_frr_test.shsubprojects/frr-10.5.wrapsubprojects/frr-10.6.wrapsubprojects/frr-10.7.wrapsubprojects/frr-master.wrapsubprojects/packagefiles/frr/10.5-zebra-route-IPv4-link-local-neighbor-updates-through.patchsubprojects/packagefiles/frr/10.7-zebra-route-IPv4-link-local-neighbor-updates-through.patch
| // Validation of Router Advertisements | ||
| // https://www.rfc-editor.org/rfc/rfc4861#section-6.1.2 | ||
| // | ||
| // - The IP Hop Limit field has a value of 255, i.e., the packet | ||
| // could not possibly have been forwarded by a router. | ||
| ASSERT_NDP(d->hop_limit == 255); | ||
| // - ICMP Checksum is valid. (already checked in icmp6_input) | ||
| // | ||
| // - ICMP Code is 0. | ||
| ASSERT_NDP(icmp6->code == 0); | ||
| // - ICMP length (derived from the IP length) is 16 or more | ||
| // octets: 4 bytes ICMP header + 12 bytes RA fields. | ||
| ASSERT_NDP(d->len >= sizeof(*icmp6) + sizeof(struct icmp6_router_advert)); | ||
| // - The IP source address is a link-local address. | ||
| ASSERT_NDP(rte_ipv6_addr_is_linklocal(&d->src)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid RA options before control-plane delivery.
An RA with a valid 16-byte fixed header followed by an option whose Length is zero passes all current checks and reaches l4_loopback_output. RFC 4861 requires receivers to discard RAs with a zero-length option. Parse the option sequence after the RA header. Reject a zero Length and an option that exceeds the remaining payload before selecting LOOPBACK. Add packet tests for both cases. (rfc-editor.org)
The PR objective requires invalid RAs to be dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/ip6/datapath/ndp_ra_input.c` around lines 43 - 57, Update the Router
Advertisement validation in the RA input handler before l4_loopback_output is
selected: parse the option sequence following the fixed RA header, reject any
option with a zero Length, and reject any option whose encoded size exceeds the
remaining payload so invalid RAs are dropped rather than delivered. Add packet
tests covering both malformed option cases.
rjarry
left a comment
rjarry
left a comment
There was a problem hiding this comment.
Could you split in 3 commits? That way, the source tree always succeeds build/test at every step.
- Punt RAs to control plane representors
- Add zebra patch
- Add smoke test
Thanks!
And I also noticed that I need to re-generated the svg. Will do! |
grout does not process IPv6 router advertisements itself. Until now the icmp6_input node dispatched them to the unsupported edge, so they were dropped in the data path. A routing daemon running on top of grout needs to see these packets. BGP unnumbered peering (RFC 5549) in particular relies on received router advertisements to discover the peer's link-local next hop; when the RAs are dropped the session never leaves the "Idle (Waiting for peer address)" state. Add a ndp_ra_input node that validates received router advertisements as described in RFC 4861 section 6.1.2 (hop limit 255, ICMP code 0, minimum length and link-local source address) and punts the valid ones to the port control plane representor via l4_loopback_output. Invalid advertisements are dropped in ndp_ra_input_inval. Closes: DPDK#657 Co-authored-by: Robin Jarry <rjarry@redhat.com> Assisted-by: GitHub Copilot CLI:claude-opus-5 Signed-off-by: Christopher Dziomba <christopher.dziomba@telekom.de>
FRR's if_nbr_mac_to_ipv4ll_neigh_update() calls kernel_neigh_update() directly, bypassing the dplane abstraction. This crashes zebra when using dplane_grout because the netlink socket is not initialized. Carry patches for FRR 10.5-10.6 and 10.7+ that route these neighbor updates through the dplane API instead. Link: FRRouting/frr#22773 Signed-off-by: Robin Jarry <rjarry@redhat.com>
0483fcc to
500babf
Compare
Check BGP unnumbered peering (RFC 5549) between an FRR+grout instance and a pure FRR peer over a net_tap port. Unnumbered peering relies on IPv6 router advertisements to discover the peer link-local next hop, so the session can only be established once grout punts the received advertisements to the port control plane representor. Advertising IPv4 prefixes over such a session additionally requires the zebra neighbor update patch. Both routers only advertise an IPv4 loopback prefix, the ports carry no IPv4 address at all. Assert that the prefix advertised by the peer is installed in the FIB with an IPv6 link-local next hop and that IPv4 traffic is forwarded between both loopbacks over that next hop. Co-authored-by: Robin Jarry <rjarry@redhat.com> Assisted-by: GitHub Copilot CLI:claude-opus-5 Signed-off-by: Christopher Dziomba <christopher.dziomba@telekom.de>
500babf to
e91d212
Compare
|
The CI failure here is not caused by this series. The smoke test added in the last commit is the first one to configure an address on the
Fixed separately in #681. |
2 participants
ip6: BGP unnumbered peering via Router Advertisement punt
Problem
BGP unnumbered peering (RFC 5549) relies on IPv6 Router Advertisements to
discover the peer's link-local next hop. grout was dropping all received RAs
in
icmp6_input(dispatched to theunsupportededge), so a routing daemonsuch as FRR running on top of grout would never receive them. The BGP session
would remain stuck in
Idle (Waiting for peer address)indefinitely.A second issue caused zebra to crash when programming a link-local neighbor
learned over such a session:
if_nbr_mac_to_ipv4ll_neigh_update()callskernel_neigh_update()directly, bypassing the dplane abstraction, whichdoes not work with
dplane_grout.Solution
ip6: punt received router advertisements to control planeAdd a new
ndp_ra_inputgraph node that validates incoming RouterAdvertisements per RFC 4861 §6.1.2:
Valid packets are punted to the control plane via the existing
l4_loopback_outputpath. Invalid packets are dropped viandp_ra_input_inval. A smoke test verifies an unnumbered eBGP session comesup end-to-end between an FRR+grout instance and a pure FRR peer.
frr: patch zebra neighbor update crash with dplane_groutCarry patches for FRR 10.5–10.7+ that route link-local neighbor updates
through the dplane API instead of calling
kernel_neigh_update()directly.(based on FRRouting/frr#22773)
Closes
Closes #657
Summary