Blackhole #291
Blackhole #291
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
christophefontaine
force-pushed
the
blackhole
branch
from
0ec617e to
92c37e2
Compare
christophefontaine
force-pushed
the
blackhole
branch
from
92c37e2 to
d9fe390
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds two nexthop types GR_NH_T_BLACKHOLE and GR_NH_T_REJECT and propagates support across the stack. Changes include: API enum and name mapping; CLI parsing to permit "blackhole"/"reject" nexthop add; nexthop creation/deletion logic (separate blackhole and L3 paths, relaxed ifindex validation for blackhole); grout <-> FRR translation to map blackhole/reject to FRR blackhole types; route deletion to use actual nexthop type; control-plane registration and nexthop creation acceptance; datapath registrations and new drop nodes (ip_blackhole/ip6_blackhole and admin-prohibited drops); and ARP/ICMP/ICMP6/NDP output graphs extended with BLACKHOLE/REJECT edges. CLI route listing now prints "blackhole" or "reject" for those nexthops. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
modules/ip/cli/route.c (1)
131-139: route4_get: handle blackhole nexthops (avoid bogus “via … lladdr …” output).When the nexthop is BLACKHOLE, the current output prints “via lladdr iface …”, which is misleading and may use unset fields. Print “via blackhole” and skip MAC/iface in that case.
Apply this diff:
resp = resp_ptr; - printf(IP4_F " via " IP4_F " lladdr " ETH_F, &req.dest, &resp->nh.ipv4, &resp->nh.mac); + if (resp->nh.type == GR_NH_T_BLACKHOLE) { + printf(IP4_F " via blackhole", &req.dest); + } else { + printf(IP4_F " via " IP4_F " lladdr " ETH_F, + &req.dest, &resp->nh.ipv4, &resp->nh.mac); + } if (resp->nh.nh_id != GR_NH_ID_UNSET) printf(" id %u", resp->nh.nh_id); - if (iface_from_id(c, resp->nh.iface_id, &iface) == 0) - printf(" iface %s", iface.name); - else - printf(" iface %u", resp->nh.iface_id); + if (resp->nh.type != GR_NH_T_BLACKHOLE) { + if (iface_from_id(c, resp->nh.iface_id, &iface) == 0) + printf(" iface %s", iface.name); + else + printf(" iface %u", resp->nh.iface_id); + }modules/ip/control/route.c (1)
267-275: Null deref + wrong route type: use exact lookup and handle missing route.
- Potential null dereference: nh can be NULL; nh->type will crash.
- Using rib4_lookup (LPM) may pick a different route than the one being deleted; passing its type to rib4_delete can spuriously return EINVAL.
Fix by using rib4_lookup_exact and handling the missing case.
Apply this diff:
- nh = rib4_lookup(req->vrf_id, req->dest.ip); - ret = rib4_delete(req->vrf_id, req->dest.ip, req->dest.prefixlen, nh->type); + nh = rib4_lookup_exact(req->vrf_id, req->dest.ip, req->dest.prefixlen); + if (nh == NULL) { + ret = -ENOENT; + } else { + ret = rib4_delete(req->vrf_id, req->dest.ip, req->dest.prefixlen, nh->type); + }
♻️ Duplicate comments (1)
frr/rt_grout.c (1)
828-833: Don’t assign iface_id for BLACKHOLE nexthops.Current
elsebranch runs for BLACKHOLE and copiesifindexintoiface_id, which is meaningless for blackholes and can propagate an invalid interface ID. Set to an explicit UNDEF and gate the error on non-BLACKHOLE types only.Apply this diff:
- if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { - gr_log_err("impossible to add/del nexthop in grout that does not have an ifindex"); - return ZEBRA_DPLANE_REQUEST_FAILURE; - } else { - gr_nh->iface_id = nh->ifindex; - } + if (nh->type == NEXTHOP_TYPE_BLACKHOLE) { + gr_nh->iface_id = GR_IFACE_ID_UNDEF; + } else if (!nh->ifindex) { + gr_log_err("impossible to add/del nexthop in grout that does not have an ifindex"); + return ZEBRA_DPLANE_REQUEST_FAILURE; + } else { + gr_nh->iface_id = nh->ifindex; + }
🧹 Nitpick comments (7)
modules/ip6/cli/route.c (1)
85-98: Add a default case in AF switch to avoid empty NEXT_HOP on unexpected AFsFuture-proof the listing by handling unknown AF values.
Apply:
- else - switch (route->nh.af) { + else + switch (route->nh.af) { case GR_AF_UNSPEC: if (iface_from_id(c, route->nh.iface_id, &iface) < 0) scols_line_sprintf(line, 2, "%u", route->nh.iface_id); else scols_line_sprintf(line, 2, "%s", iface.name); break; case GR_AF_IP4: scols_line_sprintf(line, 2, IP4_F, &route->nh.ipv4); break; case GR_AF_IP6: scols_line_sprintf(line, 2, IP6_F, &route->nh.ipv6); break; + default: + scols_line_sprintf(line, 2, "af=%u", route->nh.af); + break; }Additionally, consider updating route6_get to display “blackhole” instead of “via ” when resp->nh.type == GR_NH_T_BLACKHOLE:
// Inside route6_get(), after resp = resp_ptr; if (resp->nh.type == GR_NH_T_BLACKHOLE) { printf(IP6_F " blackhole", &req.dest); } else { printf(IP6_F " via " IP6_F " lladdr " ETH_F, &req.dest, &resp->nh.ipv6, &resp->nh.mac); if (resp->nh.nh_id != GR_NH_ID_UNSET) printf(" id %u", resp->nh.nh_id); if (iface_from_id(c, resp->nh.iface_id, &iface) == 0) printf(" iface %s", iface.name); else printf(" iface %u", resp->nh.iface_id); } printf("\n");modules/ip6/datapath/ip6_input.c (1)
190-190: Drop node registered for blackholeGR_DROP_REGISTER(ip6_blackhole) complements the nexthop-type registration. Good.
Consider adding a unit test ensuring BLACKHOLE nexthops enqueue to the dynamically attached edge:
- Mock gr_node_attach_parent("ip6_input", "ip6_blackhole") to return a sentinel edge (e.g., 42).
- Stub fib6_lookup to return a nexthop with type = GR_NH_T_BLACKHOLE.
- Verify rte_node_enqueue_x1 receives next == 42.
I can draft this test in the existing GROUT_UNIT_TEST harness if useful.
modules/ip/cli/route.c (1)
82-98: Good addition: renders blackhole next hop explicitly. Consider a safe default in AF switch.Printing "blackhole" for GR_NH_T_BLACKHOLE is correct. Minor hardening: add a default in the AF switch to avoid empty output if a new AF appears in the future.
Apply this diff to add a default case:
else switch (route->nh.af) { case GR_AF_UNSPEC: if (iface_from_id(c, route->nh.iface_id, &iface) < 0) scols_line_sprintf(line, 2, "%u", route->nh.iface_id); else scols_line_sprintf(line, 2, "%s", iface.name); break; case GR_AF_IP4: scols_line_sprintf(line, 2, IP4_F, &route->nh.ipv4); break; case GR_AF_IP6: scols_line_sprintf(line, 2, IP6_F, &route->nh.ipv6); break; + default: + scols_line_set_data(line, 2, "?"); + break; }modules/ip/datapath/ip_input.c (1)
161-162: Wiring BLACKHOLE to a dedicated drop node is correct. Add a unit test.Registration to ip_blackhole integrates cleanly with nh_type_edges. Recommend adding a small unit test asserting that a BLACKHOLE nexthop routes to ip_blackhole.
I can draft a minimal cmocka test mirroring existing patterns to verify the ip_blackhole edge. Want me to add it?
modules/infra/cli/nexthop.c (1)
62-66: Good defaults; ensure AF is explicit for blackhole path.Defaulting to L3 is fine. When selecting blackhole later, explicitly set af = GR_AF_UNSPEC to avoid any accidental reuse of AF.
See suggested change in the blackhole branch below.
frr/rt_grout.c (1)
193-201: Initialize all fields for BLACKHOLE nexthops (defensive clarity).The fields are effectively zeroed by callers today, but setting
ifindexexplicitly improves self-containment and avoids future surprises when this helper is reused.Apply this diff:
if (gr_nh->type == GR_NH_T_BLACKHOLE) { nh->vrf_id = gr_nh->vrf_id; nh->type = NEXTHOP_TYPE_BLACKHOLE; nh->bh_type = BLACKHOLE_NULL; + nh->ifindex = 0; *nh_family = AF_UNSPEC; nh->weight = 1; return 0; }modules/infra/api/nexthop.c (1)
158-160: Deletion guard is correct; also gate AF ops for BLACKHOLE.Allowing BLACKHOLE deletion is fine. However, the unconditional AF ops delete right after this can assert if AF=UNSPEC or is unnecessary for blackhole.
Outside this hunk, wrap the AF ops section:
// Only run AF ops delete for L3 nexthops if (nh->type == GR_NH_T_L3) { ops = nexthop_af_ops_get(nh->af); assert(ops != NULL); ops->del(nh); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (12)
frr/rt_grout.c(3 hunks)modules/infra/api/gr_nexthop.h(2 hunks)modules/infra/api/nexthop.c(2 hunks)modules/infra/cli/nexthop.c(3 hunks)modules/infra/control/nexthop.c(2 hunks)modules/ip/cli/route.c(1 hunks)modules/ip/control/route.c(1 hunks)modules/ip/datapath/ip_input.c(2 hunks)modules/ip6/cli/route.c(1 hunks)modules/ip6/control/route.c(1 hunks)modules/ip6/datapath/ip6_input.c(2 hunks)modules/l2/meson.build(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
modules/ip/cli/route.c (2)
modules/infra/cli/iface.c (1)
iface_from_id(130-143)modules/infra/control/gr_iface.h (1)
iface(15-21)
modules/ip6/cli/route.c (3)
cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/iface.c (1)
iface_from_id(130-143)modules/infra/control/gr_iface.h (1)
iface(15-21)
modules/infra/api/nexthop.c (2)
main/gr_module.h (1)
api_out(16-19)modules/infra/control/gr_iface.h (1)
iface(15-21)
modules/infra/cli/nexthop.c (3)
cli/gr_cli.h (1)
arg_u32(70-76)cli/ecoli.c (2)
arg_str(85-95)with_help(15-24)modules/infra/api/nexthop.c (1)
nh_add(42-137)
🔇 Additional comments (7)
modules/infra/api/gr_nexthop.h (2)
153-155: Type name mapping updated — consistent with existing styleReturning "BLACKHOLE" matches casing used for other types ("DNAT", "L3"). LGTM.
33-34: BLACKHOLE nexthop: ABI-safe addition — verifiedScanned switches and references for nexthop type; GR_NH_T_BLACKHOLE is handled where needed.
Files confirmed handling:
- modules/infra/api/gr_nexthop.h (enum + gr_nh_type_name -> "BLACKHOLE")
- modules/infra/control/nexthop.c (switch cases include GR_NH_T_BLACKHOLE)
- modules/infra/api/nexthop.c (API checks allow/guard BLACKHOLE)
- modules/infra/cli/nexthop.c (CLI can create BLACKHOLE nh)
- modules/ip/datapath/ip_input.c & modules/ip6/datapath/ip6_input.c (register blackhole datapath)
- frr/rt_grout.c (convert to/from FRR NEXTHOP_TYPE_BLACKHOLE)
- modules/ip/cli/route.c & modules/ip6/cli/route.c (CLI display checks)
No missing switch/case handling found in the repo scan — change looks ABI-safe and integrated.
modules/ip6/cli/route.c (1)
82-85: Correct: render blackhole in NEXT_HOP columnExplicitly printing “blackhole” for GR_NH_T_BLACKHOLE improves UX and matches IPv4 side. Good.
modules/ip6/datapath/ip6_input.c (1)
155-155: Properly routes BLACKHOLE nexthops to ip6_blackholeRegistration via ip6_input_register_nexthop_type is the right integration point.
modules/infra/control/nexthop.c (2)
315-321: Type registration for BLACKHOLE looks correct.BLACKHOLE accepted alongside existing types; validation logic remains consistent. No issues.
335-344: Allowing BLACKHOLE in nexthop_new is correct.Creation path recognizes BLACKHOLE without altering AF validation for other types. Looks good.
modules/ip/datapath/ip_input.c (1)
194-195: Drop node registration for ip_blackhole is consistent.Matches the IPv4 drop-node pattern used elsewhere. No concerns.
christophefontaine
force-pushed
the
blackhole
branch
from
d9fe390 to
5c8eac9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
modules/ip6/control/route.c (1)
169-176: Set GR_NH_F_GATEWAY in rib6_insert (like IPv4) and remove the unconditional set in route6_add to avoid UAFIPv6 currently sets GR_NH_F_GATEWAY in route6_add after rib6_insert returns. If rib6_insert failed (or returned -EEXIST and you later coerce to success with exist_ok), nh may have been decref’d/freed or not the one installed, leading to incorrect state or UAF. Mirror IPv4 behavior by setting the flag inside rib6_insert upon successful insertion, and delete the set in route6_add.
Apply:
@@ int rib6_insert(...){ - rte_rib6_set_nh(rn, nh_ptr_to_id(nh)); + rte_rib6_set_nh(rn, nh_ptr_to_id(nh)); + nh->flags |= GR_NH_F_GATEWAY; o = rte_rib6_get_ext(rn); *o = origin;And in route6_add:
- nh->flags |= GR_NH_F_GATEWAY; + // Flag is set on successful insertion in rib6_insert()modules/ip/control/route.c (1)
267-275: Fix potential NULL deref; use exact lookup to validate deletion type; gate flag clear on successSame issue as IPv6: rib4_lookup() is LPM and may return NULL or a different entry; dereferencing nh->type can crash and/or pass the wrong type to rib4_delete. Use rib4_lookup_exact() and clear GR_NH_F_GATEWAY only on successful deletion.
Apply:
- nh = rib4_lookup(req->vrf_id, req->dest.ip); - ret = rib4_delete(req->vrf_id, req->dest.ip, req->dest.prefixlen, nh->type); - if (ret == -ENOENT && req->missing_ok) - ret = 0; - - if (nh && nh->ref_count == 1) - nh->flags &= ~GR_NH_F_GATEWAY; + nh = rib4_lookup_exact(req->vrf_id, req->dest.ip, req->dest.prefixlen); + if (nh == NULL) { + ret = -errno; + if (ret == -ENOENT && req->missing_ok) + ret = 0; + return api_out(-ret, 0); + } + ret = rib4_delete(req->vrf_id, req->dest.ip, req->dest.prefixlen, nh->type); + if (ret == -ENOENT && req->missing_ok) + ret = 0; + + if (ret == 0 && nh->ref_count == 1) + nh->flags &= ~GR_NH_F_GATEWAY;
♻️ Duplicate comments (1)
modules/ip6/control/route.c (1)
291-302: Fix NULL deref and use exact lookup for deletion type (same as earlier review); gate flag clear on success onlyrib6_lookup() is LPM and may return NULL; dereferencing nh->type will crash. You must use rib6_lookup_exact() to fetch the exact entry and its type, and only clear GR_NH_F_GATEWAY if deletion succeeded.
Apply:
- nh = rib6_lookup(req->vrf_id, GR_IFACE_ID_UNDEF, &req->dest.ip); - ret = rib6_delete( - req->vrf_id, GR_IFACE_ID_UNDEF, &req->dest.ip, req->dest.prefixlen, nh->type - ); - if (ret == -ENOENT && req->missing_ok) - ret = 0; - - if (nh && nh->ref_count == 1) - nh->flags &= ~GR_NH_F_GATEWAY; + nh = rib6_lookup_exact( + req->vrf_id, GR_IFACE_ID_UNDEF, &req->dest.ip, req->dest.prefixlen + ); + if (nh == NULL) { + ret = -errno; + if (ret == -ENOENT && req->missing_ok) + ret = 0; + return api_out(-ret, 0); + } + ret = rib6_delete( + req->vrf_id, GR_IFACE_ID_UNDEF, &req->dest.ip, req->dest.prefixlen, nh->type + ); + if (ret == -ENOENT && req->missing_ok) + ret = 0; + + if (ret == 0 && nh->ref_count == 1) + nh->flags &= ~GR_NH_F_GATEWAY;
🧹 Nitpick comments (4)
modules/ip/datapath/ip_input.c (2)
190-195: Drop node registration for ip_blackhole is correctThis ensures the parent attach won’t reference a missing node at runtime. Consider adding a simple unit test to assert that GR_NH_T_BLACKHOLE maps to a non-FORWARD edge.
I can add a focused unit test under GROUT_UNIT_TEST to mock gr_node_attach_parent and validate the mapping. Want me to open a follow-up?
34-41: Optional: guard against out-of-range type valuesip_input_register_nexthop_type assumes type < 256. Add a bounds check to fail fast if an invalid type is passed.
void ip_input_register_nexthop_type(gr_nh_type_t type, const char *next_node) { LOG(DEBUG, "ip_input: nexthop type=%u -> %s", type, next_node); - if (type == 0) + if (type == 0) ABORT("invalid nexthop type=%u", type); + if (type >= RTE_DIM(nh_type_edges)) + ABORT("nexthop type out of range: %u", type); if (nh_type_edges[type] != FORWARD) ABORT("next node already registered for nexthop type=%u", type); nh_type_edges[type] = gr_node_attach_parent("ip_input", next_node); }modules/ip/cli/route.c (2)
82-98: Print fallback for unknown AF and keep BLACKHOLE renderingBLACKHOLE rendering is correct. Add a default case so we don’t emit an empty cell if a future AF appears.
else switch (route->nh.af) { case GR_AF_UNSPEC: if (iface_from_id(c, route->nh.iface_id, &iface) < 0) scols_line_sprintf(line, 2, "%u", route->nh.iface_id); else scols_line_sprintf(line, 2, "%s", iface.name); break; case GR_AF_IP4: scols_line_sprintf(line, 2, IP4_F, &route->nh.ipv4); break; case GR_AF_IP6: scols_line_sprintf(line, 2, IP6_F, &route->nh.ipv6); break; + default: + scols_line_set_data(line, 2, "?"); + break; }
120-139: CLI get: consider handling BLACKHOLE in “route get” outputWhen the nexthop type is BLACKHOLE, printing “via lladdr ” is misleading. Consider a special-case to print “via blackhole” instead.
I can propose a small patch to switch on resp->nh.type in route4_get and render “blackhole” accordingly. Want me to draft it?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
frr/rt_grout.c(3 hunks)modules/infra/api/gr_nexthop.h(2 hunks)modules/infra/api/nexthop.c(2 hunks)modules/infra/cli/nexthop.c(3 hunks)modules/infra/control/nexthop.c(2 hunks)modules/ip/cli/route.c(1 hunks)modules/ip/control/route.c(1 hunks)modules/ip/datapath/ip_input.c(2 hunks)modules/ip6/cli/route.c(1 hunks)modules/ip6/control/route.c(1 hunks)modules/ip6/datapath/ip6_input.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- modules/infra/api/gr_nexthop.h
- modules/ip6/datapath/ip6_input.c
- modules/infra/control/nexthop.c
- modules/ip6/cli/route.c
- frr/rt_grout.c
- modules/infra/api/nexthop.c
- modules/infra/cli/nexthop.c
🧰 Additional context used
🧬 Code Graph Analysis (1)
modules/ip/cli/route.c (3)
cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/iface.c (1)
iface_from_id(130-143)modules/infra/control/gr_iface.h (1)
iface(15-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
🔇 Additional comments (1)
modules/ip/datapath/ip_input.c (1)
159-162: BLACKHOLE wiring to drop node looks goodRegistering GR_NH_T_BLACKHOLE to route to ip_blackhole is the right datapath behavior and consistent with IPv6.
christophefontaine
force-pushed
the
blackhole
branch
from
5c8eac9 to
c49e5e8
Compare
christophefontaine
force-pushed
the
blackhole
branch
from
c49e5e8 to
916a2c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
modules/infra/api/gr_nexthop.h (1)
153-155: Name mapping uses lowercase “blackhole” as requestedMatches CLI output conventions and resolves the prior feedback about lowercase.
frr/rt_grout.c (2)
828-833: Don’t propagate ifindex for BLACKHOLE nexthopsAssigning iface_id for a blackhole nexthop is misleading; set it to 0/UNDEF and only copy ifindex for non-blackhole types.
Apply this diff:
- if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { - gr_log_err("impossible to add/del nexthop in grout that does not have an ifindex"); - return ZEBRA_DPLANE_REQUEST_FAILURE; - } else { - gr_nh->iface_id = nh->ifindex; - } + if (nh->type == NEXTHOP_TYPE_BLACKHOLE) { + gr_nh->iface_id = 0; // no interface for blackhole + } else if (!nh->ifindex) { + gr_log_err("impossible to add/del nexthop in grout that does not have an ifindex"); + return ZEBRA_DPLANE_REQUEST_FAILURE; + } else { + gr_nh->iface_id = nh->ifindex; + }
852-854: Normalize AF and clear L3 addresses for BLACKHOLE nexthopsKeep semantics tight: force AF to UNSPEC and zero L3 address storage for blackhole types.
Apply this diff:
case NEXTHOP_TYPE_BLACKHOLE: - gr_nh->type = GR_NH_T_BLACKHOLE; + gr_nh->type = GR_NH_T_BLACKHOLE; + gr_nh->af = GR_AF_UNSPEC; + gr_nh->ipv4 = 0; + memset(&gr_nh->ipv6, 0, sizeof(gr_nh->ipv6)); break;
🧹 Nitpick comments (1)
frr/rt_grout.c (1)
193-201: Fully initialize nexthop for BLACKHOLE to avoid stale fieldsSet ifindex to 0 and clear gate to prevent accidental use of residual data when the object is later copied.
Apply this diff:
if (gr_nh->type == GR_NH_T_BLACKHOLE) { - nh->vrf_id = gr_nh->vrf_id; - nh->type = NEXTHOP_TYPE_BLACKHOLE; - nh->bh_type = BLACKHOLE_NULL; - *nh_family = AF_UNSPEC; - nh->weight = 1; + nh->vrf_id = gr_nh->vrf_id; + nh->type = NEXTHOP_TYPE_BLACKHOLE; + nh->bh_type = BLACKHOLE_NULL; + nh->ifindex = 0; + memset(&nh->gate, 0, sizeof(nh->gate)); + *nh_family = AF_UNSPEC; + nh->weight = 1; return 0; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
frr/rt_grout.c(3 hunks)modules/infra/api/gr_nexthop.h(2 hunks)modules/infra/api/nexthop.c(2 hunks)modules/infra/cli/nexthop.c(3 hunks)modules/infra/control/nexthop.c(2 hunks)modules/ip/cli/route.c(1 hunks)modules/ip/control/route.c(1 hunks)modules/ip/datapath/ip_input.c(2 hunks)modules/ip6/cli/route.c(1 hunks)modules/ip6/control/route.c(1 hunks)modules/ip6/datapath/ip6_input.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- modules/infra/control/nexthop.c
- modules/ip/control/route.c
- modules/ip6/control/route.c
- modules/infra/api/nexthop.c
- modules/ip6/cli/route.c
- modules/ip6/datapath/ip6_input.c
- modules/ip/cli/route.c
- modules/infra/cli/nexthop.c
- modules/ip/datapath/ip_input.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
🔇 Additional comments (1)
modules/infra/api/gr_nexthop.h (1)
33-34: BLACKHOLE nexthop type addition looks goodEnum entry added without re-numbering existing values. No ABI concerns given it's appended after DNAT.
christophefontaine
force-pushed
the
blackhole
branch
from
916a2c0 to
1fbca9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
frr/rt_grout.c (2)
828-833: Don’t assign iface_id for BLACKHOLE nexthopsThe unconditional else assigns iface_id even for NEXTHOP_TYPE_BLACKHOLE. That couples a drop nexthop to a (possibly invalid) interface and can confuse downstream logic that keys on iface_id.
Apply this diff to avoid setting iface_id for BLACKHOLE and keep semantics clean:
- if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { + if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { gr_log_err("impossible to add/del nexthop in grout that does not have an ifindex"); return ZEBRA_DPLANE_REQUEST_FAILURE; - } else { - gr_nh->iface_id = nh->ifindex; - } + } else if (nh->type != NEXTHOP_TYPE_BLACKHOLE) { + gr_nh->iface_id = nh->ifindex; + } else { + /* BLACKHOLE has no interface */ + gr_nh->iface_id = 0; + }
852-854: Normalize AF and clear addresses for BLACKHOLE nexthopsFor BLACKHOLE, force AF to UNSPEC and clear L3 fields so no gateway/addr/iface state leaks through. This also aligns expectations across the stack.
Apply this diff:
case NEXTHOP_TYPE_BLACKHOLE: gr_nh->type = GR_NH_T_BLACKHOLE; + gr_nh->af = GR_AF_UNSPEC; + gr_nh->iface_id = 0; + gr_nh->ipv4 = 0; + memset(&gr_nh->ipv6, 0, sizeof(gr_nh->ipv6)); break;modules/infra/api/nexthop.c (3)
178-186: Don’t call AF ops->del for BLACKHOLEDeleting a BLACKHOLE nexthop should not invoke AF-specific del handlers.
Apply this diff:
ops = nexthop_af_ops_get(nh->af); assert(ops != NULL); - ops->del(nh); + if (nh->type != GR_NH_T_BLACKHOLE) + ops->del(nh);
42-46: Sanitize BLACKHOLE nexthop fields (AF/addresses/iface) to avoid L3 leakageBLACKHOLE nexthops should not carry an AF, interface, or L3 address. Set AF=UNSPEC, zero addresses, and clear iface_id here to keep semantics consistent and prevent AF-specific ops from triggering later.
Apply this diff:
static int nh_add_blackhole(struct gr_nexthop *base) { + base->af = GR_AF_UNSPEC; + base->iface_id = 0; + base->ipv4 = 0; + memset(&base->ipv6, 0, sizeof(base->ipv6)); base->state = GR_NH_S_REACHABLE; base->flags |= GR_NH_F_STATIC; return 0; }
87-110: Skip AF add/del ops for BLACKHOLE to prevent /32-/128 churnThe create/update flows always call ops->add/ops->del. For BLACKHOLE, there should be no AF-specific route programming. Guard these calls to avoid unintended /32-/128 routes and churn.
Apply these diffs:
@@ if (nh == NULL) { nh = nexthop_new(&base); if (nh == NULL) return api_out(errno, 0); - ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - ret = ops->add(nh); + if (base.type == GR_NH_T_BLACKHOLE) { + ret = 0; // No AF-specific programming for blackhole + } else { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + ret = ops->add(nh); + } @@ - if (need_update) { + if (need_update && nh->type != GR_NH_T_BLACKHOLE) { // Address family or address has changed. // Delete the old /32 or /128 route. ops = nexthop_af_ops_get(nh->af); assert(ops != NULL); nexthop_incref(nh); // Prevent ops->del from freeing the nexthop. ops->del(nh); } @@ - if (need_update) { + if (need_update && nh->type != GR_NH_T_BLACKHOLE) { // Re-add the new /32 or /128 route. ops = nexthop_af_ops_get(nh->af); assert(ops != NULL); ret = ops->add(nh); nexthop_decref(nh); // ops->add called nexthop_incref if successful. } else { ret = 0; }Also applies to: 121-149
🧹 Nitpick comments (1)
frr/rt_grout.c (1)
193-200: Good FRR→BH translation; consider zeroing ancillary fields for safetyMapping GR_NH_T_BLACKHOLE to NEXTHOP_TYPE_BLACKHOLE with AF_UNSPEC and a sane weight looks good. As a minor hardening step, also explicitly set nh->ifindex = 0 and clear gate addresses to avoid any accidental downstream use of uninitialized fields.
Apply this diff within the BLACKHOLE branch:
if (gr_nh->type == GR_NH_T_BLACKHOLE) { nh->vrf_id = gr_nh->vrf_id; nh->type = NEXTHOP_TYPE_BLACKHOLE; nh->bh_type = BLACKHOLE_NULL; *nh_family = AF_UNSPEC; nh->weight = 1; + nh->ifindex = 0; + memset(&nh->gate, 0, sizeof(nh->gate)); return 0; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
frr/rt_grout.c(3 hunks)modules/infra/api/nexthop.c(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
modules/infra/api/nexthop.c (3)
modules/infra/control/gr_iface.h (1)
iface(15-21)modules/infra/cli/nexthop.c (1)
nh_add(61-102)main/gr_module.h (1)
api_out(16-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
🔇 Additional comments (2)
modules/infra/api/nexthop.c (2)
51-69: AF/addr validation logic for L3 is soundChecks for IPv4/IPv6 presence, UNSPEC constraints, and ENOPROTOOPT on invalid AF look correct and readable.
71-85: Interface/VRF binding and MAC handling look correctResolving iface, inheriting vrf_id, and promoting to REACHABLE+STATIC when MAC is provided is consistent. The UNSPEC+MAC guard is a good catch.
christophefontaine
force-pushed
the
blackhole
branch
from
1fbca9e to
f6953ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
modules/infra/api/nexthop.c (2)
112-125: Skip AF ops for BLACKHOLE/REJECT.Avoid programming AF-specific routes for blackhole/reject. This prevents /32-/128 insertion and ARP noise.
Apply this diff:
if (nh == NULL) { nh = nexthop_new(&base); if (nh == NULL) return api_out(errno, 0); - ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - ret = ops->add(nh); + if (nh->type == GR_NH_T_BLACKHOLE || nh->type == GR_NH_T_REJECT) { + ret = 0; /* no AF programming */ + } else { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + ret = ops->add(nh); + }
133-150: Guard del/add on update for BLACKHOLE/REJECT.Wrap ops->del and ops->add so we don’t touch AF routes for blackhole/reject during updates.
Apply this diff:
if (need_update) { // Address family or address has changed. // Delete the old /32 or /128 route. - ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - nexthop_incref(nh); // Prevent ops->del from freeing the nexthop. - ops->del(nh); + if (nh->type != GR_NH_T_BLACKHOLE && nh->type != GR_NH_T_REJECT) { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + nexthop_incref(nh); // Prevent ops->del from freeing the nexthop. + ops->del(nh); + } } // Update fields after deleting the route. if ((ret = nexthop_update(nh, &base)) < 0) { if (need_update) - nexthop_decref(nh); + nexthop_decref(nh); goto end; } if (need_update) { // Re-add the new /32 or /128 route. - ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - ret = ops->add(nh); - nexthop_decref(nh); // ops->add called nexthop_incref if successful. + if (nh->type != GR_NH_T_BLACKHOLE && nh->type != GR_NH_T_REJECT) { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + ret = ops->add(nh); + nexthop_decref(nh); // ops->add called nexthop_incref if successful. + } else { + ret = 0; + } } else { ret = 0; }
♻️ Duplicate comments (2)
frr/rt_grout.c (1)
828-833: Don’t assign iface_id for BLACKHOLE nexthops.The else branch still assigns gr_nh->iface_id for NEXTHOP_TYPE_BLACKHOLE, which is semantically wrong and risks propagating a bogus interface ID.
Apply this diff to avoid setting iface_id for blackhole:
- if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { + if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { gr_log_err("impossible to add/del nexthop in grout that does not have an ifindex"); return ZEBRA_DPLANE_REQUEST_FAILURE; - } else { - gr_nh->iface_id = nh->ifindex; - } + } else if (nh->type != NEXTHOP_TYPE_BLACKHOLE) { + gr_nh->iface_id = nh->ifindex; + } else { + gr_nh->iface_id = 0; /* or GR_IFACE_ID_UNDEF */ + }modules/infra/api/nexthop.c (1)
42-46: BLACKHOLE path must sanitize AF/addresses and iface.As-is, clients can send af=IPv4/IPv6 with type=BLACKHOLE and the add path will later program AF-specific ops, causing unintended /32-/128 nexthops and ARP churn (matches the ARP error you observed). Force AF=UNSPEC, clear addresses, and clear iface.
Apply this diff:
static int nh_add_blackhole(struct gr_nexthop *base) { - base->state = GR_NH_S_REACHABLE; - base->flags |= GR_NH_F_STATIC; + /* Blackhole/reject: no L3 gw, no iface, no address */ + base->af = GR_AF_UNSPEC; + base->iface_id = 0; /* or GR_IFACE_ID_UNDEF if available */ + base->ipv4 = 0; + memset(&base->ipv6, 0, sizeof(base->ipv6)); + base->state = GR_NH_S_REACHABLE; + base->flags |= GR_NH_F_STATIC; return 0; }
🧹 Nitpick comments (1)
modules/infra/api/gr_nexthop.h (1)
33-35: New nexthop enum values added — verify ABI/wire stability.Inserting GR_NH_T_BLACKHOLE and GR_NH_T_REJECT in the middle of gr_nh_type_t shifts subsequent numeric values. If this enum is serialized over the wire or persisted, this can cause incompatibilities when components are version-skewed. Consider pinning explicit values for all entries (or at least ensuring both ends are always built from the same headers) to avoid drift.
Would you like a patch to assign explicit numeric values and document compatibility expectations?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
frr/rt_grout.c(3 hunks)modules/infra/api/gr_nexthop.h(2 hunks)modules/infra/api/nexthop.c(2 hunks)modules/infra/cli/nexthop.c(3 hunks)modules/infra/control/nexthop.c(2 hunks)modules/ip/cli/route.c(1 hunks)modules/ip/control/route.c(1 hunks)modules/ip/datapath/ip_input.c(2 hunks)modules/ip6/cli/route.c(1 hunks)modules/ip6/control/route.c(1 hunks)modules/ip6/datapath/ip6_input.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- modules/ip/control/route.c
- modules/ip6/cli/route.c
- modules/ip/cli/route.c
- modules/ip6/control/route.c
- modules/ip/datapath/ip_input.c
- modules/infra/cli/nexthop.c
🧰 Additional context used
🧬 Code Graph Analysis (2)
frr/rt_grout.c (1)
modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(144-160)
modules/infra/api/nexthop.c (3)
modules/infra/control/gr_iface.h (1)
iface(15-21)modules/infra/cli/nexthop.c (1)
nh_add(61-107)main/gr_module.h (1)
api_out(16-19)
🔇 Additional comments (6)
modules/infra/api/gr_nexthop.h (1)
154-158: Type names for blackhole/reject look good.Returning "blackhole" and "reject" aligns with CLI/log expectations and prior feedback about lowercase.
modules/infra/control/nexthop.c (2)
315-317: Allowing BLACKHOLE/REJECT in type ops registration is correct.The guard still enforces ops != NULL and one of free/equal set, which is fine.
341-343: Accepting BLACKHOLE/REJECT in nexthop_new is appropriate.No behavioral changes beyond validation, consistent with API extensions.
modules/ip6/datapath/ip6_input.c (2)
155-156: Correct wiring: BLACKHOLE → drop, REJECT → dest_unreach.The mapping is consistent with expected semantics and mirrors IPv4. Good.
190-191: Drop node registration for ip6_blackhole added — good.Completes the wiring for type→edge mapping.
frr/rt_grout.c (1)
193-201: FRR translation for BLACKHOLE/REJECT is correct.Setting NEXTHOP_TYPE_BLACKHOLE with bh_type and AF_UNSPEC short-circuits L3 handling cleanly.
christophefontaine
force-pushed
the
blackhole
branch
from
f6953ec to
88efa38
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
modules/infra/api/nexthop.c (1)
118-126: Use -errno on allocation failure and skip AF ops for BLACKHOLE/REJECT.
- Return -errno on nexthop_new failure.
- BLACKHOLE/REJECT should not program per-AF routes; guard ops->add.
Apply:
nh = nexthop_new(&base); if (nh == NULL) - return api_out(errno, 0); - ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - ret = ops->add(nh); + return api_out(-errno, 0); + if (nh->type == GR_NH_T_BLACKHOLE || nh->type == GR_NH_T_REJECT) { + ret = 0; + } else { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + ret = ops->add(nh); + }
♻️ Duplicate comments (6)
modules/infra/cli/nexthop.c (2)
72-80: Blackhole/Reject: set AF to UNSPEC and allow VRF selection (avoid silent VRF 0).Currently these branches only set type and jump to send. Make AF explicit and parse optional [vrf VRF] so users aren’t confined to VRF 0 implicitly. This also aligns the CLI with the API’s blackhole semantics.
Apply:
if (arg_str(p, "blackhole") != NULL) { req.nh.type = GR_NH_T_BLACKHOLE; + req.nh.af = GR_AF_UNSPEC; + if (arg_u16(p, "VRF", &req.nh.vrf_id) < 0 && errno != ENOENT) + return CMD_ERROR; goto send; } if (arg_str(p, "reject") != NULL) { req.nh.type = GR_NH_T_REJECT; + req.nh.af = GR_AF_UNSPEC; + if (arg_u16(p, "VRF", &req.nh.vrf_id) < 0 && errno != ENOENT) + return CMD_ERROR; goto send; }
262-271: Expose VRF in CLI grammar for blackhole/reject.The grammar doesn’t accept a VRF for these types, making it impossible to target non-default VRFs from CLI.
Apply:
- "nexthop [id ID] ([address IP] iface IFACE [mac MAC])|blackhole|reject", + "nexthop [id ID] (([address IP] iface IFACE [mac MAC])|blackhole [vrf VRF]|reject [vrf VRF])", @@ - with_help("Output interface.", ec_node_dyn("IFACE", complete_iface_names, NULL)), - with_help("Blackhole nexthop.", ec_node_str("blackhole", "blackhole")), - with_help("Reject nexthop sending ICMP UNREACH.", ec_node_str("reject", "reject")) + with_help("Output interface.", ec_node_dyn("IFACE", complete_iface_names, NULL)), + with_help("Blackhole nexthop.", ec_node_str("blackhole", "blackhole")), + with_help("Reject nexthop sending ICMP UNREACH.", ec_node_str("reject", "reject")), + with_help("L3 routing domain ID (blackhole/reject only).", ec_node_uint("VRF", 0, UINT16_MAX - 1, 10))modules/infra/api/nexthop.c (4)
133-137: Guard AF ops del/add during updates for BLACKHOLE/REJECT.Skip route teardown/re-add for types that don’t own L3 host routes.
Apply:
- ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - nexthop_incref(nh); // Prevent ops->del from freeing the nexthop. - ops->del(nh); + if (nh->type != GR_NH_T_BLACKHOLE && nh->type != GR_NH_T_REJECT) { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + nexthop_incref(nh); // Prevent ops->del from freeing the nexthop. + ops->del(nh); + } @@ - ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - ret = ops->add(nh); - nexthop_decref(nh); // ops->add called nexthop_incref if successful. + if (nh->type != GR_NH_T_BLACKHOLE && nh->type != GR_NH_T_REJECT) { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + ret = ops->add(nh); + nexthop_decref(nh); // ops->add called nexthop_incref if successful. + } else { + ret = 0; + }Also applies to: 145-150
42-46: Sanitize BLACKHOLE/REJECT base fields to avoid leaking L3 semantics.Force AF to UNSPEC, clear addresses and iface_id. Otherwise, stale AF/addr/iface may slip through and trigger AF ops unexpectedly.
Apply:
static int nh_add_blackhole(struct gr_nexthop *base) { - base->state = GR_NH_S_REACHABLE; - base->flags |= GR_NH_F_STATIC; + base->af = GR_AF_UNSPEC; + base->iface_id = 0; /* or GR_IFACE_ID_UNDEF */ + base->ipv4 = 0; + memset(&base->ipv6, 0, sizeof(base->ipv6)); + base->state = GR_NH_S_REACHABLE; + base->flags |= GR_NH_F_STATIC; return 0; }
179-181: Cannot delete REJECT nexthops; include GR_NH_T_REJECT.REJECT is omitted from the allowed types, making them effectively undeletable via API.
Apply:
- if ((nh->type != GR_NH_T_L3 && nh->type != GR_NH_T_BLACKHOLE) + if ((nh->type != GR_NH_T_L3 && nh->type != GR_NH_T_BLACKHOLE && nh->type != GR_NH_T_REJECT) || (nh->flags & addr_flags) == addr_flags || nh->ref_count > 1) return api_out(EBUSY, 0);Optional: also skip ops->del for BLACKHOLE/REJECT (no AF routes to tear down).
51-85: Normalize error semantics: return negative errno from nh_add_l3.The dispatcher later does api_out(-ret, 0). Returning positive errno here produces inconsistent status codes.
Apply:
switch (base->af) { case GR_AF_IP4: - if (base->ipv4 == 0) - return EDESTADDRREQ; + if (base->ipv4 == 0) + return -EDESTADDRREQ; break; case GR_AF_IP6: - if (rte_ipv6_addr_is_unspec(&base->ipv6)) - return EDESTADDRREQ; + if (rte_ipv6_addr_is_unspec(&base->ipv6)) + return -EDESTADDRREQ; break; case GR_AF_UNSPEC: - if (base->ipv4 || !rte_ipv6_addr_is_unspec(&base->ipv6)) - return EINVAL; + if (base->ipv4 || !rte_ipv6_addr_is_unspec(&base->ipv6)) + return -EINVAL; base->flags |= GR_NH_F_LINK | GR_NH_F_STATIC; break; default: - return ENOPROTOOPT; + return -ENOPROTOOPT; } @@ iface = iface_from_id(base->iface_id); if (iface == NULL) - return errno; + return -errno; @@ if (!rte_is_zero_ether_addr(&base->mac)) { - if (base->af == GR_AF_UNSPEC) - return EINVAL; + if (base->af == GR_AF_UNSPEC) + return -EINVAL;
🧹 Nitpick comments (1)
modules/ip/datapath/arp_output_request.c (1)
66-69: Separate REJECT edge (map to existing ip_blackhole for now)No
ip_rejectdrop node is registered in the codebase, so Option B isn’t possible yet. Introduce a dedicatedREJECTedge but route it to the existingip_blackhole; you can switch to"ip_reject"once a drop node is added.
- Add
REJECTin the enum beforeERRORand updateEDGE_COUNT.- Handle
GR_NH_T_REJECTinarp_output_request.cas its own branch.- In
.next_nodes, mapREJECT→"ip_blackhole"(change to"ip_reject"later).enum { OUTPUT = 0, BLACKHOLE, + REJECT, ERROR, EDGE_COUNT, }; @@ -66,9 +66,13 @@ - if (nh->type == GR_NH_T_BLACKHOLE || nh->type == GR_NH_T_REJECT) { - edge = BLACKHOLE; - goto next; - } + if (nh->type == GR_NH_T_BLACKHOLE) { + edge = BLACKHOLE; + goto next; + } + if (nh->type == GR_NH_T_REJECT) { + edge = REJECT; + goto next; + } .next_nodes = { [OUTPUT] = "eth_output", [BLACKHOLE] = "ip_blackhole", + [REJECT] = "ip_blackhole", [ERROR] = "arp_output_error", },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
frr/rt_grout.c(3 hunks)modules/infra/api/gr_nexthop.h(2 hunks)modules/infra/api/nexthop.c(2 hunks)modules/infra/cli/nexthop.c(3 hunks)modules/infra/control/nexthop.c(2 hunks)modules/ip/cli/route.c(1 hunks)modules/ip/control/route.c(1 hunks)modules/ip/datapath/arp_output_request.c(3 hunks)modules/ip/datapath/icmp_output.c(3 hunks)modules/ip/datapath/ip_input.c(2 hunks)modules/ip6/cli/route.c(1 hunks)modules/ip6/control/route.c(1 hunks)modules/ip6/datapath/icmp6_output.c(3 hunks)modules/ip6/datapath/ip6_input.c(2 hunks)modules/ip6/datapath/ndp_ns_output.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- modules/infra/api/gr_nexthop.h
- modules/ip6/datapath/ip6_input.c
- modules/ip/cli/route.c
- modules/ip6/control/route.c
- frr/rt_grout.c
- modules/ip/datapath/ip_input.c
- modules/ip6/cli/route.c
🧰 Additional context used
🧬 Code Graph Analysis (2)
modules/ip/datapath/icmp_output.c (2)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(461-488)
modules/infra/cli/nexthop.c (4)
cli/gr_cli.h (1)
arg_u32(70-76)cli/ecoli.c (2)
arg_str(85-95)with_help(15-24)modules/infra/api/nexthop.c (1)
nh_add(87-158)modules/infra/cli/iface.c (1)
complete_iface_names(69-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
🔇 Additional comments (6)
modules/ip6/datapath/icmp6_output.c (1)
69-72: Confirm REJECT semantics: drop vs generating an errorRouting ICMPv6 to BLACKHOLE for both BLACKHOLE and REJECT is consistent and prevents loops, but for REJECT some stacks emit an ICMPv6 admin-prohibited. If your intended behavior is to silently drop on REJECT too, this is fine. Otherwise, consider a distinct REJECT edge/node that signals the reason.
Would you like me to wire a dedicated ip6_reject node (with a distinct drop reason and/or ICMP emission) mirroring ip6_blackhole?
Also applies to: 92-93
modules/infra/control/nexthop.c (1)
315-317: Add type support in registration: LGTMAccepting GR_NH_T_BLACKHOLE and GR_NH_T_REJECT in nexthop_type_ops_register is correct and consistent with the new types.
modules/ip6/datapath/ndp_ns_output.c (1)
73-76: NDP solicit suppression on BLACKHOLE/REJECT: LGTM
- Early divert to BLACKHOLE for BLACKHOLE/REJECT NH types avoids pointless NS traffic.
- Trace behavior (0-length on non-OUTPUT) matches the ICMP paths and keeps tracing lightweight.
- Next-node wiring to ip6_blackhole is correct.
Also applies to: 112-119, 136-137
modules/ip/datapath/icmp_output.c (1)
55-58: IPv4 ICMP to BLACKHOLE/REJECT: semantics check and tracing
- Diverting to ip_blackhole for BLACKHOLE and REJECT prevents error loops — good.
- Added zero-length trace on exit is consistent with other nodes.
If REJECT should convey “administratively prohibited” rather than silent drop, consider a dedicated reject path.
Do you want a distinct ip_reject node (separate drop reason and/or ICMP emission) for GR_NH_T_REJECT?
Also applies to: 64-66, 81-82
modules/ip/datapath/arp_output_request.c (1)
23-28: LGTM: Early bail-out and BLACKHOLE wiring are correct.Short-circuiting ARP emission for BLACKHOLE/REJECT and wiring the new edge to ip_blackhole is consistent with the broader design and avoids spurious address resolution.
Also applies to: 131-134
modules/infra/cli/nexthop.c (1)
62-66: Sensible defaults for nh_add request..exist_ok=true, origin=USER, and default type=L3 are reasonable. Works well with the early “blackhole/reject” short-circuit below.
christophefontaine
force-pushed
the
blackhole
branch
from
88efa38 to
a2da240
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
modules/ip6/control/route.c (1)
302-304: Potential use-after-free on nh after rib6_delete
rib6_delete()decrefs and may freenh. Dereferencingnhafterwards (to clearGR_NH_F_GATEWAY) risks UAF. Also, clearing the flag when ref_count == 1 is moot: the object is likely freed.Either drop this block or compute any needed state before deletion and ensure you don’t touch
nhafterrib6_delete.Apply one of:
- Minimal safe fix (remove post-delete deref):
- if (nh && nh->ref_count == 1) - nh->flags &= ~GR_NH_F_GATEWAY;
- Or, if you must mutate flags, capture pre-delete refcount and only clear if it will remain alive (pre_refcount > 1), then clear before calling rib6_delete(). Keep it gated on (ret == 0) and avoid touching nh after delete.
♻️ Duplicate comments (4)
modules/infra/api/gr_nexthop.h (1)
154-157: Type-to-string mapping added correctlyBoth new types are handled and return the expected lowercase strings. This aligns with previous naming guidance.
modules/ip6/control/route.c (1)
293-298: Use exact RIB lookup before delete; current LPM-based type can cause EINVAL
route6_delderivesnh_typeusingrib6_lookup(LPM).rib6_delete()later performs an exact lookup and validates type (Line 218). If a covering route’s nexthop type differs from the exact route’s type, deletion fails with EINVAL.Switch to
rib6_lookup_exact()and pass the exact route’s type.Apply:
- nh = rib6_lookup(req->vrf_id, GR_IFACE_ID_UNDEF, &req->dest.ip); - ret = rib6_delete( - req->vrf_id, - GR_IFACE_ID_UNDEF, - &req->dest.ip, - req->dest.prefixlen, - nh ? nh->type : GR_NH_T_L3 - ); + nh = rib6_lookup_exact( + req->vrf_id, GR_IFACE_ID_UNDEF, &req->dest.ip, req->dest.prefixlen + ); + if (nh == NULL) { + ret = -errno; + if (ret == -ENOENT && req->missing_ok) + ret = 0; + return api_out(-ret, 0); + } + ret = rib6_delete( + req->vrf_id, + GR_IFACE_ID_UNDEF, + &req->dest.ip, + req->dest.prefixlen, + nh->type + );frr/rt_grout.c (1)
852-857: Sanitize BLACKHOLE/REJECT nexthops (clear residual L3 fields)Type, AF, and iface are set. For extra safety and to avoid leaking stale data across the API, explicitly zero L3 address fields.
Apply:
case NEXTHOP_TYPE_BLACKHOLE: gr_nh->type = nh->bh_type == BLACKHOLE_REJECT ? GR_NH_T_REJECT : GR_NH_T_BLACKHOLE; gr_nh->af = GR_AF_UNSPEC; gr_nh->iface_id = GR_IFACE_ID_UNDEF; + gr_nh->ipv4 = 0; + memset(&gr_nh->ipv6, 0, sizeof(gr_nh->ipv6)); gr_log_debug("add nexthop id %u with type %s", nh_id, gr_nh_type_name(gr_nh)); break;modules/infra/api/nexthop.c (1)
87-111: Return code polarity bug in dispatcher (positive errno returned).Default case and the early error return use positive errno (EINVAL/ret), while the function tail returns api_out(-ret, 0). This creates inconsistent API semantics.
Apply this diff:
switch (base.type) { case GR_NH_T_BLACKHOLE: case GR_NH_T_REJECT: ret = nh_add_blackhole(&base); break; case GR_NH_T_L3: case GR_NH_T_SR6_OUTPUT: case GR_NH_T_SR6_LOCAL: case GR_NH_T_DNAT: ret = nh_add_l3(&base); break; default: - return api_out(EINVAL, 0); + return api_out(-EINVAL, 0); } - if (ret) - return api_out(ret, 0); + if (ret) + return api_out(-ret, 0);Follow-up: elsewhere in this function, there are still positive errno returns (e.g., return api_out(errno, 0) on nexthop_new failure). Consider normalizing those to negative for consistency.
🧹 Nitpick comments (2)
modules/ip/cli/route.c (1)
82-100: Show special NH types: correct and user-friendlyThe “NEXT_HOP” rendering for BLACKHOLE/REJECT is clear and consistent with IPv6.
If you want to avoid duplicating literals, you can rely on gr_nh_type_name() for non-L3 types:
- if (route->nh.type == GR_NH_T_BLACKHOLE) - scols_line_sprintf(line, 2, "blackhole"); - else if (route->nh.type == GR_NH_T_REJECT) - scols_line_sprintf(line, 2, "reject"); - else + if (route->nh.type != GR_NH_T_L3) + scols_line_sprintf(line, 2, "%s", gr_nh_type_name(&route->nh)); + else switch (route->nh.af) { ... }frr/rt_grout.c (1)
828-833: Avoid assigning iface_id for BLACKHOLE nexthops in the generic branchYou later override iface_id for BLACKHOLE, but setting it here first is confusing. Narrow the assignment to non-BLACKHOLE types.
Apply:
- if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { + if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { gr_log_err("impossible to add/del nexthop in grout that does not have an ifindex"); return ZEBRA_DPLANE_REQUEST_FAILURE; - } else { - gr_nh->iface_id = nh->ifindex; - } + } else if (nh->type != NEXTHOP_TYPE_BLACKHOLE) { + gr_nh->iface_id = nh->ifindex; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
frr/rt_grout.c(3 hunks)modules/infra/api/gr_nexthop.h(2 hunks)modules/infra/api/nexthop.c(2 hunks)modules/infra/cli/nexthop.c(3 hunks)modules/infra/control/nexthop.c(2 hunks)modules/ip/cli/route.c(1 hunks)modules/ip/control/route.c(1 hunks)modules/ip/datapath/arp_output_request.c(3 hunks)modules/ip/datapath/icmp_output.c(3 hunks)modules/ip/datapath/ip_input.c(2 hunks)modules/ip6/cli/route.c(1 hunks)modules/ip6/control/route.c(1 hunks)modules/ip6/datapath/icmp6_output.c(3 hunks)modules/ip6/datapath/ip6_input.c(2 hunks)modules/ip6/datapath/ndp_ns_output.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- modules/ip/datapath/arp_output_request.c
- modules/ip/control/route.c
- modules/infra/control/nexthop.c
- modules/ip6/datapath/ip6_input.c
- modules/ip6/datapath/ndp_ns_output.c
- modules/ip/datapath/icmp_output.c
- modules/ip/datapath/ip_input.c
- modules/ip6/cli/route.c
- modules/infra/cli/nexthop.c
🧰 Additional context used
🧬 Code Graph Analysis (2)
modules/ip/cli/route.c (3)
cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/iface.c (1)
iface_from_id(130-143)modules/infra/control/gr_iface.h (1)
iface(15-21)
modules/infra/api/nexthop.c (3)
modules/infra/control/gr_iface.h (1)
iface(15-21)modules/infra/cli/nexthop.c (1)
nh_add(61-107)main/gr_module.h (1)
api_out(16-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
🔇 Additional comments (5)
modules/infra/api/gr_nexthop.h (1)
33-35: New nexthop types: placement and naming look goodAdding BLACKHOLE/REJECT after DNAT avoids re-numbering existing values. Names are lowercase, consistent with prior feedback.
modules/ip6/datapath/icmp6_output.c (2)
19-19: Edge added for BLACKHOLEEdge enum and count update are correct.
92-92: Next node mapping updated correctlyBLACKHOLE edge maps to ip6_blackhole; consistent with enum change.
frr/rt_grout.c (1)
193-200: Correct translation of grout BLACKHOLE/REJECT to FRR nexthopsShort-circuiting to NEXTHOP_TYPE_BLACKHOLE with proper bh_type and AF_UNSPEC is correct.
modules/infra/api/nexthop.c (1)
48-85: L3 path looks good (validation and errno polarity).The AF validation, negative errno returns, VRF inference via iface, and state/flags transitions are consistent. No functional issues spotted here.
christophefontaine
force-pushed
the
blackhole
branch
from
a2da240 to
43d9518
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
modules/ip6/datapath/icmp6_output.c (1)
70-76: Propagate nh to drop paths for richer diagnostics (optional)Setting o->nh before the early goto lets downstream (or trace/telemetry) see the original nh type. Harmless if unused.
- if (nh->type == GR_NH_T_BLACKHOLE) { - edge = BLACKHOLE; - goto next; - } else if (nh->type == GR_NH_T_REJECT) { - edge = REJECT; - goto next; - } + if (nh->type == GR_NH_T_BLACKHOLE) { + o = ip6_output_mbuf_data(mbuf); + o->nh = nh; + edge = BLACKHOLE; + goto next; + } else if (nh->type == GR_NH_T_REJECT) { + o = ip6_output_mbuf_data(mbuf); + o->nh = nh; + edge = REJECT; + goto next; + }frr/rt_grout.c (1)
828-833: Don’t assign iface_id for BLACKHOLE in the generic branchThe else branch sets iface_id even when nh->type is BLACKHOLE; it’s later overwritten in the switch, but this is confusing and brittle. Assign iface_id only for non-BLACKHOLE types.
Apply:
- if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { + if (nh->type != NEXTHOP_TYPE_BLACKHOLE && !nh->ifindex) { gr_log_err("impossible to add/del nexthop in grout that does not have an ifindex"); return ZEBRA_DPLANE_REQUEST_FAILURE; - } else { - gr_nh->iface_id = nh->ifindex; } + if (nh->type != NEXTHOP_TYPE_BLACKHOLE) + gr_nh->iface_id = nh->ifindex;modules/infra/api/nexthop.c (2)
42-48: Sanitize BLACKHOLE/REJECT: clear IP and MAC tooAF/iface are neutralized, but IP/MAC fields may carry user-supplied data. Clear them to avoid leaking L3 semantics.
static int nh_add_blackhole(struct gr_nexthop *base) { base->state = GR_NH_S_REACHABLE; base->flags |= GR_NH_F_STATIC; base->af = GR_AF_UNSPEC; base->iface_id = GR_IFACE_ID_UNDEF; + base->ipv4 = 0; + memset(&base->ipv6, 0, sizeof(base->ipv6)); + memset(&base->mac, 0, sizeof(base->mac)); return 0; }
89-113: Unify error code polarity: return negative errnoDefault case and the early error return still pass positive errno to api_out, while the function epilogue uses negative. Align both for consistency.
switch (base.type) { case GR_NH_T_BLACKHOLE: case GR_NH_T_REJECT: ret = nh_add_blackhole(&base); break; case GR_NH_T_L3: case GR_NH_T_SR6_OUTPUT: case GR_NH_T_SR6_LOCAL: case GR_NH_T_DNAT: ret = nh_add_l3(&base); break; default: - return api_out(EINVAL, 0); + return api_out(-EINVAL, 0); } - if (ret) - return api_out(ret, 0); + if (ret) + return api_out(-ret, 0);
🧹 Nitpick comments (3)
modules/ip6/cli/route.c (1)
82-100: Add a defensive default in the AF switch (minor)Future AF values would currently leave the column unset. A tiny default keeps output predictable.
switch (route->nh.af) { case GR_AF_UNSPEC: if (iface_from_id(c, route->nh.iface_id, &iface) < 0) scols_line_sprintf(line, 2, "%u", route->nh.iface_id); else scols_line_sprintf(line, 2, "%s", iface.name); break; case GR_AF_IP4: scols_line_sprintf(line, 2, IP4_F, &route->nh.ipv4); break; case GR_AF_IP6: scols_line_sprintf(line, 2, IP6_F, &route->nh.ipv6); break; + default: + scols_line_set_data(line, 2, "-"); + break; }modules/ip/cli/route.c (1)
82-100: Add a default case in the AF switch (minor hardening)Prevents an empty column if a new/unexpected AF appears.
switch (route->nh.af) { case GR_AF_UNSPEC: if (iface_from_id(c, route->nh.iface_id, &iface) < 0) scols_line_sprintf(line, 2, "%u", route->nh.iface_id); else scols_line_sprintf(line, 2, "%s", iface.name); break; case GR_AF_IP4: scols_line_sprintf(line, 2, IP4_F, &route->nh.ipv4); break; case GR_AF_IP6: scols_line_sprintf(line, 2, IP6_F, &route->nh.ipv6); break; + default: + scols_line_set_data(line, 2, "-"); + break; }frr/rt_grout.c (1)
852-857: Nit: also clear L3 addresses for BLACKHOLE/REJECTYou already force AF_UNSPEC and iface_id=UNDEF. For completeness and to prevent future leakage if the struct stops being zero-initialized elsewhere, also zero L3 address fields.
case NEXTHOP_TYPE_BLACKHOLE: gr_nh->type = nh->bh_type == BLACKHOLE_REJECT ? GR_NH_T_REJECT : GR_NH_T_BLACKHOLE; gr_nh->af = GR_AF_UNSPEC; gr_nh->iface_id = GR_IFACE_ID_UNDEF; + gr_nh->ipv4 = 0; + memset(&gr_nh->ipv6, 0, sizeof(gr_nh->ipv6)); gr_log_debug("add nexthop id %u with type %s", nh_id, gr_nh_type_name(gr_nh)); break;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
frr/rt_grout.c(3 hunks)modules/infra/api/gr_nexthop.h(2 hunks)modules/infra/api/nexthop.c(2 hunks)modules/infra/cli/nexthop.c(3 hunks)modules/infra/control/nexthop.c(2 hunks)modules/ip/cli/route.c(1 hunks)modules/ip/control/route.c(1 hunks)modules/ip/datapath/arp_output_request.c(3 hunks)modules/ip/datapath/icmp_output.c(4 hunks)modules/ip/datapath/ip_input.c(2 hunks)modules/ip6/cli/route.c(1 hunks)modules/ip6/control/route.c(1 hunks)modules/ip6/datapath/icmp6_output.c(4 hunks)modules/ip6/datapath/ip6_input.c(2 hunks)modules/ip6/datapath/ndp_ns_output.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- modules/ip/control/route.c
- modules/ip/datapath/arp_output_request.c
- modules/infra/cli/nexthop.c
- modules/ip/datapath/ip_input.c
- modules/ip6/datapath/ip6_input.c
- modules/infra/api/gr_nexthop.h
- modules/infra/control/nexthop.c
- modules/ip6/control/route.c
🧰 Additional context used
🧬 Code Graph Analysis (5)
modules/ip6/cli/route.c (3)
cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/iface.c (1)
iface_from_id(130-143)modules/infra/control/gr_iface.h (1)
iface(15-21)
modules/ip/cli/route.c (5)
cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/control/iface.c (1)
iface_from_id(215-222)modules/infra/cli/iface.c (1)
iface_from_id(130-143)modules/infra/control/worker_test.c (1)
iface_from_id(40-42)modules/infra/control/gr_iface.h (1)
iface(15-21)
frr/rt_grout.c (1)
modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(144-160)
modules/ip/datapath/icmp_output.c (2)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(461-488)
modules/infra/api/nexthop.c (3)
modules/infra/control/gr_iface.h (1)
iface(15-21)modules/infra/cli/nexthop.c (1)
nh_add(61-107)main/gr_module.h (1)
api_out(16-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
🔇 Additional comments (11)
modules/ip6/cli/route.c (1)
82-100: NEXT_HOP renders “blackhole”/“reject” correctly — LGTMStraightforward and readable handling before AF-based formatting. Matches the v4 CLI behavior.
modules/ip/cli/route.c (1)
82-100: Consistent NEXT_HOP formatting for blackhole/reject — LGTMBehavior mirrors IPv6 CLI; nice consistency.
modules/ip/datapath/icmp_output.c (1)
56-62: Correct REJECT/BLACKHOLE dispatch and node wiring — LGTM
- Early diverging to BLACKHOLE/REJECT avoids loops and keeps semantics clean.
- Next-node mappings and drop registration are in place.
- Trace hook on all paths is a nice touch for observability.
Also applies to: 85-87, 99-99
modules/ip6/datapath/ndp_ns_output.c (2)
73-76: NS suppressed on BLACKHOLE/REJECT — LGTMDropping NDP solicit when the nexthop is blackhole/reject is the sane choice; wiring to ip6_blackhole is appropriate.
Also applies to: 135-137
112-119: Trace behavior is precise — LGTM0-byte trace for non-OUTPUT and payload snapshot for OUTPUT keeps traces useful without noise.
modules/ip6/datapath/icmp6_output.c (2)
70-76: Separate edges for BLACKHOLE/REJECT — LGTMClear split with dedicated admin-prohibited path matches expected REJECT semantics.
Also applies to: 96-98
110-112: Admin-prohibited drop path registered — LGTMNode registration completes the wiring; consistent with the IPv4 ICMP output.
frr/rt_grout.c (1)
193-200: FRR blackhole translation is correct and minimalGood short-circuit mapping to FRR’s blackhole nexthop with AF_UNSPEC and weight set. This avoids unintended L3 handling downstream.
modules/infra/api/nexthop.c (3)
53-71: L3 AF validation and errno polarity look goodIPv4/IPv6/UNSPEC checks are correct and return negative errno values as expected. Nice.
73-87: Interface/VRF propagation and MAC gating are soundiface lookup to derive vrf_id and state/MAC semantics are consistent with expectations.
181-183: REJECT allowed in deletion guard — goodIncluding GR_NH_T_REJECT unblocks deletion of reject nexthops. LGTM.
Add blackhole nexthop to discards packets instead of forwarding them. Useful for blocking unwanted traffic or mitigating attacks without sending ICMP unreachable responses. Similar to the blackhole nexthop, the reject nexthop sends an icmp message "destination unreachable" to the source. Signed-off-by: Christophe Fontaine <[email protected]>
Using the new blackole nexthop, remove current limitation
about FRR not able to configure a blackhole route.
Blackhole and Reject nexthops are now sync from/to zebra.
vtysh# show nexthop-group rib
ID: 19 (zebra)
RefCnt: 2
Uptime: 00:00:39
VRF: default(IPv6)
Nexthop Count: 1
Valid, Installed
unreachable (blackhole), weight 1
ID: 20 (zebra)
RefCnt: 2
Uptime: 00:00:39
VRF: default(IPv6)
Nexthop Count: 1
Valid
unreachable (ICMP unreachable), weight 1
grout# show nexthop
VRF ID TYPE FAMILY IP MAC IFACE STATE FLAGS ORIGIN
0 19 blackhole Unspec 00:00:00:00:00:00 ? reachable static gateway zebra
0 20 reject Unspec 00:00:00:00:00:00 ? reachable static gateway zebra
Signed-off-by: Christophe Fontaine <[email protected]>
christophefontaine
force-pushed
the
blackhole
branch
from
43d9518 to
4d6f837
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
modules/infra/control/nexthop.c (1)
315-317: New BLACKHOLE/REJECT types accepted: ensure type_ops are registered and guard UNSPEC keyingThe added cases look correct, but two follow-ups:
- Make sure nexthop_type_ops_register is actually called somewhere for GR_NH_T_BLACKHOLE and GR_NH_T_REJECT (even with no-op equal/free), else type-specific hooks will remain unset.
- set_nexthop_key() ABORTs on GR_AF_UNSPEC. If a BLACKHOLE/REJECT nexthop carries a non-zero IP, nexthop_update() will still try to key it and hit the ABORT. Upstream sanitation must zero addresses for BLACKHOLE/REJECT (see api/nexthop.c comment). Optionally, defensively skip keying when af == GR_AF_UNSPEC:
Example (outside this hunk):
if (nh->af != GR_AF_UNSPEC && (nh->ipv4 != 0 || !rte_ipv6_addr_is_unspec(&nh->ipv6))) { set_nexthop_key(...); ... }Run to verify registrations exist:
#!/bin/bash set -euo pipefail echo "== Check nexthop type_ops registration for BLACKHOLE/REJECT ==" rg -n -C2 -P 'nexthop_type_ops_register\s*\(\s*GR_NH_T_BLACKHOLE' rg -n -C2 -P 'nexthop_type_ops_register\s*\(\s*GR_NH_T_REJECT'Also applies to: 341-343
modules/infra/api/nexthop.c (1)
42-47: BLACKHOLE/REJECT must clear IP and MAC; otherwise control path can ABORTAF is forced to GR_AF_UNSPEC but IPv4/IPv6/MAC aren’t cleared. nexthop_update() will attempt to key AF_UNSPEC entries if any address is non-zero, and set_nexthop_key() ABORTs on UNSPEC. Also avoids leaking L3 semantics (ARP/NDP churn).
Apply:
static int nh_add_blackhole(struct gr_nexthop *base) { base->state = GR_NH_S_REACHABLE; base->flags |= GR_NH_F_STATIC; base->af = GR_AF_UNSPEC; base->iface_id = GR_IFACE_ID_UNDEF; + base->ipv4 = 0; + memset(&base->ipv6, 0, sizeof(base->ipv6)); + memset(&base->mac, 0, sizeof(base->mac)); return 0; }
🧹 Nitpick comments (10)
modules/ip6/cli/route.c (1)
82-100: Optional: use gr_nh_type_name() for consistency and add a default case.Avoid duplicating literals ("blackhole"/"reject") and future-proof the switch by adding a default. This also keeps IPv4/IPv6 CLI consistent with the API name mapping.
- if (route->nh.type == GR_NH_T_BLACKHOLE) - scols_line_sprintf(line, 2, "blackhole"); - else if (route->nh.type == GR_NH_T_REJECT) - scols_line_sprintf(line, 2, "reject"); - else - switch (route->nh.af) { + if (route->nh.type == GR_NH_T_BLACKHOLE || route->nh.type == GR_NH_T_REJECT) { + scols_line_sprintf(line, 2, "%s", gr_nh_type_name(route->nh.type)); + } else { + switch (route->nh.af) { case GR_AF_UNSPEC: if (iface_from_id(c, route->nh.iface_id, &iface) < 0) scols_line_sprintf(line, 2, "%u", route->nh.iface_id); else scols_line_sprintf(line, 2, "%s", iface.name); break; case GR_AF_IP4: scols_line_sprintf(line, 2, IP4_F, &route->nh.ipv4); break; case GR_AF_IP6: scols_line_sprintf(line, 2, IP6_F, &route->nh.ipv6); break; - } + default: + scols_line_set_data(line, 2, ""); + } + }Note: If not already in scope, you may need to include the header that declares gr_nh_type_name().
modules/ip/cli/route.c (1)
82-100: Optional: de-duplicate literals with gr_nh_type_name() and handle unknown AF.Mirror the IPv6 suggestion: leverage the API mapping and add a default to the AF switch.
- if (route->nh.type == GR_NH_T_BLACKHOLE) - scols_line_sprintf(line, 2, "blackhole"); - else if (route->nh.type == GR_NH_T_REJECT) - scols_line_sprintf(line, 2, "reject"); - else - switch (route->nh.af) { + if (route->nh.type == GR_NH_T_BLACKHOLE || route->nh.type == GR_NH_T_REJECT) { + scols_line_sprintf(line, 2, "%s", gr_nh_type_name(route->nh.type)); + } else { + switch (route->nh.af) { case GR_AF_UNSPEC: if (iface_from_id(c, route->nh.iface_id, &iface) < 0) scols_line_sprintf(line, 2, "%u", route->nh.iface_id); else scols_line_sprintf(line, 2, "%s", iface.name); break; case GR_AF_IP4: scols_line_sprintf(line, 2, IP4_F, &route->nh.ipv4); break; case GR_AF_IP6: scols_line_sprintf(line, 2, IP6_F, &route->nh.ipv6); break; - } + default: + scols_line_set_data(line, 2, ""); + } + }modules/ip/datapath/icmp_output.c (1)
56-63: Consider early short-circuit for BLACKHOLE/REJECT before building the IPv4 header.You prepend and fill the IP header before checking nh->type. For BLACKHOLE/REJECT, that work is wasted since packets go to drop nodes. Move the nh->type checks before rte_pktmbuf_prepend to save cycles on those paths.
A minimal restructuring:
- Do fib4_lookup first.
- If nh is NULL → NO_ROUTE; if BLACKHOLE/REJECT → corresponding edge; else prepend/fill IP header and continue.
This keeps behavior identical while avoiding unnecessary work on drop paths.
modules/ip/datapath/arp_output_request.c (1)
67-75: Early exit on BLACKHOLE/REJECT is correct; minor style nit and optional trace.
- Style: chain the second check as else-if to avoid a redundant compare.
- Optional: mirror ICMP/NDP by adding a 0-length trace when diverting to BLACKHOLE/REJECT for better visibility.
- if (nh->type == GR_NH_T_BLACKHOLE) { + if (nh->type == GR_NH_T_BLACKHOLE) { edge = BLACKHOLE; goto next; - } - if (nh->type == GR_NH_T_REJECT) { + } else if (nh->type == GR_NH_T_REJECT) { edge = REJECT; goto next; }To add tracing for drop edges (outside this hunk), just before enqueue:
if (gr_mbuf_is_traced(mbuf) && (edge == BLACKHOLE || edge == REJECT)) { gr_mbuf_trace_add(mbuf, node, 0); }modules/ip6/datapath/ndp_ns_output.c (1)
74-81: Style nit: chain REJECT as else-if.Functionally equivalent but avoids a second compare.
- if (nh->type == GR_NH_T_BLACKHOLE) { + if (nh->type == GR_NH_T_BLACKHOLE) { next = BLACKHOLE; goto next; - } - if (nh->type == GR_NH_T_REJECT) { + } else if (nh->type == GR_NH_T_REJECT) { next = REJECT; goto next; }modules/infra/api/nexthop.c (5)
120-127: Creation path: skip AF-specific ops for BLACKHOLE/REJECT; just increfCalling ops->add on AF_UNSPEC goes through nh_unspec_add, which is effectively just an incref. Make the intent explicit and avoid any future UNSPEC handler side-effects.
Apply:
- ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - ret = ops->add(nh); + if (nh->type == GR_NH_T_BLACKHOLE || nh->type == GR_NH_T_REJECT) { + ret = 0; + nexthop_incref(nh); + } else { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + ret = ops->add(nh); + }
135-139: Update path: do not call ops->del for old BLACKHOLE/REJECTIf the previous nexthop was BLACKHOLE/REJECT (AF_UNSPEC), ops->del would hit the UNSPEC handler which delegates to IP4/IP6 del — unnecessary and potentially noisy.
Apply:
- ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - nexthop_incref(nh); // Prevent ops->del from freeing the nexthop. - ops->del(nh); + if (nh->type != GR_NH_T_BLACKHOLE && nh->type != GR_NH_T_REJECT) { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + nexthop_incref(nh); // Prevent ops->del from freeing the nexthop. + ops->del(nh); + }
147-152: Update path: do not call ops->add for new BLACKHOLE/REJECTFor BLACKHOLE/REJECT after update, skip AF ops; keep refcount balanced without calling UNSPEC add.
Apply:
- ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - ret = ops->add(nh); - nexthop_decref(nh); // ops->add called nexthop_incref if successful. + if (nh->type == GR_NH_T_BLACKHOLE || nh->type == GR_NH_T_REJECT) { + ret = 0; + } else { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + ret = ops->add(nh); + nexthop_decref(nh); // ops->add called nexthop_incref if successful. + }
181-189: Deletion path: avoid UNSPEC del for BLACKHOLE/REJECT; just decrefNice that REJECT is now allowed for deletion. For BLACKHOLE/REJECT, skip AF ops and directly decref; this aligns with the creation/update guards and avoids UNSPEC -> IP4/IP6 del cascade.
Apply:
- ops = nexthop_af_ops_get(nh->af); - assert(ops != NULL); - ops->del(nh); + if (nh->type == GR_NH_T_BLACKHOLE || nh->type == GR_NH_T_REJECT) { + nexthop_decref(nh); + } else { + ops = nexthop_af_ops_get(nh->af); + assert(ops != NULL); + ops->del(nh); + }
42-47: Behavioral guidance: ARP/NDP with BLACKHOLE/REJECTBLACKHOLE nexthops should not trigger ARP/NDP for gateway resolution and should not answer ARP for a blackholed destination. REJECT should signal admin-prohibited (e.g., ICMP Unreachable) rather than resolve L2. The sanitation above plus skipping AF ops ensures no /32-/128 host-route churn or ARP noise.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
frr/rt_grout.c(3 hunks)modules/infra/api/gr_nexthop.h(2 hunks)modules/infra/api/nexthop.c(2 hunks)modules/infra/cli/nexthop.c(3 hunks)modules/infra/control/nexthop.c(2 hunks)modules/ip/cli/route.c(1 hunks)modules/ip/control/route.c(1 hunks)modules/ip/datapath/arp_output_request.c(3 hunks)modules/ip/datapath/icmp_output.c(4 hunks)modules/ip/datapath/ip_input.c(2 hunks)modules/ip6/cli/route.c(1 hunks)modules/ip6/control/route.c(1 hunks)modules/ip6/datapath/icmp6_output.c(4 hunks)modules/ip6/datapath/ip6_input.c(2 hunks)modules/ip6/datapath/ndp_ns_output.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- modules/ip/control/route.c
- modules/ip6/datapath/ip6_input.c
- modules/infra/cli/nexthop.c
- modules/ip/datapath/ip_input.c
- frr/rt_grout.c
- modules/infra/api/gr_nexthop.h
- modules/ip6/datapath/icmp6_output.c
- modules/ip6/control/route.c
🧰 Additional context used
🧬 Code Graph Analysis (4)
modules/ip/datapath/icmp_output.c (2)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(461-488)
modules/ip6/cli/route.c (2)
cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/iface.c (1)
iface_from_id(130-143)
modules/ip/cli/route.c (2)
cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/iface.c (1)
iface_from_id(130-143)
modules/infra/api/nexthop.c (2)
modules/infra/cli/nexthop.c (1)
nh_add(61-107)main/gr_module.h (1)
api_out(16-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
🔇 Additional comments (5)
modules/ip/datapath/icmp_output.c (2)
68-70: Trace-on-enqueue for all edges is good.Adding a trace item for BLACKHOLE/REJECT paths helps diagnostics without payload bloat.
18-23: Verified drop node registration Both ip_blackhole (modules/ip/datapath/ip_input.c) and ip_admin_prohibited (modules/ip/datapath/icmp_output.c) are registered. Resolving.modules/ip/datapath/arp_output_request.c (1)
25-29: New edges and wiring to ip_blackhole/ip_admin_prohibited look good; ensure nodes exist.Matches the IPv4/IPv6 pattern elsewhere. See the verification script in icmp_output.c comment to confirm node registrations.
Also applies to: 137-139
modules/ip6/datapath/ndp_ns_output.c (2)
117-117: Good: adds trace for non-OUTPUT edges.Parity with other datapath nodes; helps troubleshoot diverted packets.
27-31: Wiring BLACKHOLE → ip6_blackhole and REJECT → ip6_admin_prohibited is consistent.This addresses earlier feedback about using a consistent REJECT path in IPv6. Looks good.
Also applies to: 141-143
3 participants
With the following conf:
On ARP Request, grouts writes an error:
The reason is that the /32 route already exists for that specific host.
With a broader blackhole, the NH is created as expected, but is dropped after.
What would be the expected behavior in that case ?
Should we answer to the ARP request ?
Summary by CodeRabbit
New Features
Bug Fixes