infra/nexthop: validate VRF before creating nexthop, and drop nh ID if nexthop creation fails #427
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthrough
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 0
🧹 Nitpick comments (1)
modules/infra/control/nexthop.c (1)
348-352: Early iface/VRF validation is good; please confirm semantics and consider minor tweaksThe new checks match the PR goal (fail fast when iface/VRF don’t exist) and look correct functionally. A couple of details are worth a quick sanity check:
- For the iface check,
iface_from_id()used to causenexthop_new()to fail later witherrno = ENODEV(vianexthop_update), whereas now an invalid or missing iface returnsEINVAL. If any callers or tests distinguish ENODEV vs EINVAL, this is an observable change; if not, this is fine but should be intentional.- VRF is now validated unconditionally with
get_vrf_iface(base->vrf_id). For nexthops where VRF is effectively derived fromiface_id(andbase->vrf_idmight have been ignored or left at some default/“unset” value before), this could start rejecting configurations that used to be accepted. Please confirm that all call sites always populatebase->vrf_idwith a valid, existing VRF ID, even wheniface_idis set and drives the final VRF.- Minor/optional: we now call
iface_from_id()twice on the create path (once here, once innexthop_update). If this becomes hot and you care about cycles, you could factor out a small helper that both validates and returns the iface so the second lookup can be avoided, but this is purely an optimization.If you want, I can sketch a small grep/rg script to scan all
nexthop_newcall sites and check howgr_nexthop_base.vrf_idis populated for each nexthop type.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/infra/control/nexthop.c(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/nexthop.c
🧠 Learnings (7)
📓 Common learnings
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/nexthop.c
🧬 Code graph analysis (1)
modules/infra/control/nexthop.c (3)
modules/infra/control/iface.c (1)
iface_from_id(252-259)api/gr_errno.h (1)
errno_set_null(14-17)modules/infra/control/vrf.c (1)
get_vrf_iface(26-28)
⏰ 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). (8)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: rpm
- GitHub Check: deb
maxime-leroy
force-pushed
the
nexthop_iface_validation
branch
from
cbde764 to
0723b4b
Compare
maxime-leroy
commented
Dec 1, 2025
rjarry
requested changes
Dec 1, 2025
maxime-leroy
force-pushed
the
nexthop_iface_validation
branch
3 times, most recently
from
29d23a9 to
5d5954c
Compare
christophefontaine
approved these changes
Dec 2, 2025
grout-bot
force-pushed
the
nexthop_iface_validation
branch
from
5d5954c to
9d77e89
Compare
rjarry
reviewed
Dec 2, 2025
maxime-leroy
force-pushed
the
nexthop_iface_validation
branch
from
9d77e89 to
87ae079
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/infra/control/vrf.c (1)
27-28: Consider ENODEV instead of EOVERFLOW.Past review feedback suggested ENODEV for this case. While EOVERFLOW is semantically valid for a bounds check, ENODEV may be more conventional for invalid device/VRF identifiers in this codebase.
if (vrf_id >= GR_MAX_VRFS) - return errno_set_null(EOVERFLOW); + return errno_set_null(ENODEV);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/infra/control/nexthop.c(1 hunks)modules/infra/control/vrf.c(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/vrf.cmodules/infra/control/nexthop.c
🧠 Learnings (9)
📓 Common learnings
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/vrf.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/infra/control/nexthop.c
🧬 Code graph analysis (2)
modules/infra/control/vrf.c (1)
api/gr_errno.h (1)
errno_set_null(14-17)
modules/infra/control/nexthop.c (1)
modules/infra/control/vrf.c (1)
get_vrf_iface(26-33)
⏰ 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). (8)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (3)
modules/infra/control/vrf.c (1)
29-30: LGTM - appropriate error code for missing VRF interface.The ENONET error code correctly indicates that the VRF network/interface doesn't exist.
modules/infra/control/nexthop.c (2)
386-396: LGTM - proper validation of interface and VRF.The validation logic correctly ensures:
- When an interface is specified, it exists and the nexthop inherits its VRF
- When no interface is specified, the VRF must exist
This addresses the crash risk from referencing invalid interfaces/VRFs mentioned in the PR objectives.
409-411: LGTM - proper resource cleanup on error paths.The
errlabel correctly releases the nexthop ID allocated at line 383 before returning the error. This prevents ID leaks when validation fails after allocation.
maxime-leroy
changed the title
rjarry
approved these changes
Dec 2, 2025
If iface lookup or import_info fails in nexthop_update(), the nexthop id stays registered and the invalid nexthop can still be retrieved and reused (e.g. to add routes). Make sure we release the id on failure. Signed-off-by: Maxime Leroy <[email protected]> Reviewed-by: Robin Jarry <[email protected]>
Validate vrf_id in get_vrf_iface() and make nexthop_update() fail when the referenced VRF does not exist. This prevents creating nexthops that are bound to missing VRFs. Signed-off-by: Maxime Leroy <[email protected]> Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Robin Jarry <[email protected]>
grout-bot
force-pushed
the
nexthop_iface_validation
branch
from
87ae079 to
dbaeb11
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
nexthop_new() didn’t verify whether the provided VRF or interface IDs were valid.
This could result in creating a nexthop with an invalid interface, causing Grout to crash when routing packets through it.
This patch adds checks to ensure the VRF and interface exist before allocating the nexthop.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.