No reserved ifindex for vrf

[maxime-leroy]

Summary by CodeRabbit

• Refactor
  • Simplified VRF (Virtual Routing and Forwarding) handling across multiple routing components for improved efficiency.
  • Removed indirect VRF ID conversions in favor of direct dataplane-provided VRF identifiers.
  • Streamlined interface ID allocation with a unified assignment approach.
  • Enhanced dataplane abstraction integration for improved VRF context operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This change set refactors VRF (Virtual Routing and Forwarding) ID handling across multiple layers of the codebase. The modifications eliminate indirect ifindex-to-vrf_id conversions in Grout interface and route processing, simplify interface ID allocation by removing reserved ranges, and update the zebra dplane integration to obtain VRF IDs directly from dataplane accessors rather than casting ifindex values. Supporting patches separate VRF deletion from update operations.

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c10bc6 and 6c79341.

📒 Files selected for processing (6)

• frr/if_grout.c
• frr/rt_grout.c
• modules/infra/control/iface.c
• subprojects/frr.wrap
• subprojects/packagefiles/frr/0001-zebra-do-not-rely-on-dplane-table_id-for-VRF-delete.patch
• subprojects/packagefiles/frr/0002-zebra-use-dplane-provided-vrf_id-instead-of-casting-.patch

🧰 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 sets x = NULL. There is no risk of double free.
• ec_node_*() functions consume their ec_node arguments. No leaks on error.
• rte_node->ctx is an uint8_t array of size 16, not a pointer.
• Never suggest to replace assert() with graceful error checking.
• We compile with -std=gnu2x. Unnamed parameters in function definitions are valid.

Files:

• frr/if_grout.c
• modules/infra/control/iface.c
• frr/rt_grout.c

🧠 Learnings (13)

📓 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.

📚 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:

• frr/if_grout.c
• modules/infra/control/iface.c
• subprojects/packagefiles/frr/0002-zebra-use-dplane-provided-vrf_id-instead-of-casting-.patch
• frr/rt_grout.c
• subprojects/packagefiles/frr/0001-zebra-do-not-rely-on-dplane-table_id-for-VRF-delete.patch

📚 Learning: 2025-12-17T17:32:11.230Z

Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:11.230Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.

Applied to files:

• frr/if_grout.c
• modules/infra/control/iface.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:

• frr/rt_grout.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:

• frr/rt_grout.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:

• frr/rt_grout.c

📚 Learning: 2025-09-09T07:08:40.398Z

Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:106-111
Timestamp: 2025-09-09T07:08:40.398Z
Learning: For SRv6 header validation, the correct check for last_entry bounds is `srh->last_entry > (srh->hdrlen / 2) - 1` where hdr_len represents the length in 8-byte units excluding the first 8 bytes, and each segment is 16 bytes, so hdr_len/2 gives the maximum number of segments.

Applied to files:

• frr/rt_grout.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:

• frr/rt_grout.c

📚 Learning: 2025-09-09T07:36:34.893Z

Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T07:36:34.893Z
Learning: In the SRv6 routing header structure, the flag and tag fields are used for per-flow metadata and Group-Based Policy use cases. Only 5 bytes are needed to access the basic SRv6 fields: next_hdr, hdr_len, type, segments_left, and last_entry.

Applied to files:

• frr/rt_grout.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: When parsing IPv6 routing headers in SRv6 processing, bounds checking for the full extension header length and validation of the routing header structure is needed before accessing fields like hdr_len, last_entry, segments_left, and type.

Applied to files:

• frr/rt_grout.c

📚 Learning: 2025-10-21T15:42:43.874Z

Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.

Applied to files:

• frr/rt_grout.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:

• frr/rt_grout.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:

• frr/rt_grout.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). (7)

• 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-18, none, debugoptimized, ubuntu-24.04, false)
• GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
• GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
• GitHub Check: deb
• GitHub Check: rpm

🔇 Additional comments (5)

subprojects/packagefiles/frr/0001-zebra-do-not-rely-on-dplane-table_id-for-VRF-delete.patch (1)

1-210: LGTM - Clean refactoring that correctly separates VRF delete and update paths.

The split into interface_vrf_del and interface_vrf_update properly addresses the issue where delete operations were incorrectly depending on dplane_ctx_get_ifp_table_id(), which is only set for RTM_NEWLINK events. The logic is preserved correctly in both paths.

frr/if_grout.c (1)

123-124: LGTM - VRF ID assignment simplified.

The unconditional assignment of both table_id and vrf_id from gr_if->base.vrf_id is correct and consistent with Grout's architecture where table_id always equals vrf_id.

frr/rt_grout.c (1)

195-195: LGTM - Consistent VRF ID refactoring.

All changes correctly use direct VRF ID values instead of ifindex-based conversions. The refactoring is consistent throughout: nexthop VRF assignment, SR6 local out_vrf_id handling, route operations, and kernel nexthop operations all now use the proper vrf_id field directly.

Also applies to: 271-271, 295-295, 317-317, 467-467, 671-671, 707-708, 712-713, 717-717, 721-721, 828-828, 852-852

subprojects/packagefiles/frr/0002-zebra-use-dplane-provided-vrf_id-instead-of-casting-.patch (1)

42-60: LGTM - Proper VRF ID handling in FRR patch.

The patch correctly refactors VRF handling in zebra/interface.c:

• Function signatures properly updated to take vrf_id_t instead of ifindex_t
• Critical preservation of vrf_id before if_delete_update call (line 126) prevents use-after-free
• Uses proper dataplane accessor dplane_ctx_get_ifp_vrf_id(ctx) for updates
• Maintains existing behavior for Linux kernel dataplane while fixing non-kernel dataplanes

Also applies to: 63-113, 118-163

modules/infra/control/iface.c (1)

51-59: Race condition analysis is invalid for this codebase architecture.

The API socket handler (where iface_create is called) uses a single event_base from libevent, implementing a single-threaded, event-driven architecture. API requests are processed sequentially in the event loop callback without concurrent thread access. The only callers of iface_create are the API handler and loopback initialization, both operating in single-threaded contexts. While a TOCTOU race between next_ifid() and ifaces[ifid] = iface is theoretically possible, it cannot occur here because concurrent invocations of iface_create are not possible given the single-threaded event-driven design.

Likely an incorrect or invalid review comment.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}