Srv6 l2 dx2 #466
Open
Srv6 l2 dx2 #466
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
christophefontaine
force-pushed
the
srv6_l2_dx2
branch
8 times, most recently
from
0736fa8 to
5fb7619
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds SRv6 DX2 support (SR_BEHAVIOR_END_DX2) across CLI, control, and datapath code, including local behavior handling and nexthop mapping. Introduces an iface-mode API (mode enum, mode_data/mode_info, register/get, init/reconfig callbacks) with L3, L1 xconnect, and SRv6 L2-encap modes and CLI integration. Implements an SRv6 L2 encapsulation datapath node (srv6_l2_encap) and Ethernet handling in srv6 output, plus nexthop↔interface wiring and cleanup on nexthop deletion. Adds smoke tests and a Scapy helper; CI/README package lists now include scapy. Removes ICMP queue draining in fini paths. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/cli/iface.c (1)
295-330: NULL pointer dereference:mode_from_idcan return NULL.If an interface has an unregistered mode,
mode_from_idreturns NULL and setserrno = ENODEV. Line 313 then dereferencesmode->str, causing a crash. Unliketypewhich has anassert(type != NULL)at line 322,modehas no such check.const struct cli_iface_type *type = type_from_id(iface->type); const struct cli_iface_mode *mode = mode_from_id(iface->mode); + if (mode == NULL) { + ret = -1; + continue; + } struct libscols_line *line = scols_table_new_line(table, NULL);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
.github/workflows/check.yml(1 hunks)README.md(1 hunks)frr/rt_grout.c(1 hunks)modules/infra/api/gr_infra.h(3 hunks)modules/infra/cli/gr_cli_iface.h(4 hunks)modules/infra/cli/iface.c(12 hunks)modules/infra/cli/port.c(3 hunks)modules/infra/control/gr_iface.h(1 hunks)modules/infra/control/iface.c(6 hunks)modules/infra/control/xconnect.c(1 hunks)modules/srv6/api/gr_srv6.h(3 hunks)modules/srv6/cli/iface_nh.c(1 hunks)modules/srv6/cli/localsid.c(5 hunks)modules/srv6/cli/meson.build(1 hunks)modules/srv6/control/iface_nh.c(1 hunks)modules/srv6/control/meson.build(1 hunks)modules/srv6/datapath/l2_encap.c(1 hunks)modules/srv6/datapath/meson.build(1 hunks)modules/srv6/datapath/srv6.h(1 hunks)modules/srv6/datapath/srv6_local.c(6 hunks)modules/srv6/datapath/srv6_output.c(2 hunks)smoke/srv6_l2_encap_scapy.py(1 hunks)smoke/srv6_l2_encap_test.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/srv6_l2_encap_test.sh
**/*.{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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/srv6/datapath/srv6.hmodules/infra/api/gr_infra.hmodules/infra/control/gr_iface.hmodules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.cfrr/rt_grout.cmodules/srv6/api/gr_srv6.hmodules/infra/cli/iface.cmodules/infra/control/iface.cmodules/infra/cli/gr_cli_iface.hmodules/infra/cli/port.cmodules/srv6/cli/localsid.cmodules/infra/control/xconnect.cmodules/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/srv6_local.c
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: - Never comment about setext vs ATX style headings.
Files:
README.md
🧠 Learnings (16)
📓 Common learnings
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.
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
modules/srv6/control/meson.buildmodules/srv6/cli/meson.build
📚 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:
smoke/srv6_l2_encap_test.shmodules/infra/api/gr_infra.hmodules/srv6/datapath/l2_encap.cmodules/infra/control/iface.cmodules/infra/cli/gr_cli_iface.hmodules/srv6/cli/localsid.cmodules/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/srv6_local.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
modules/infra/api/gr_infra.h
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/api/gr_infra.hmodules/srv6/datapath/srv6_local.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:
modules/infra/api/gr_infra.hmodules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.cmodules/srv6/api/gr_srv6.h
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_local.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/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.cfrr/rt_grout.cmodules/srv6/cli/localsid.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/srv6_local.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:
modules/srv6/datapath/srv6_output.cmodules/srv6/api/gr_srv6.hmodules/srv6/control/iface_nh.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:
modules/srv6/datapath/srv6_output.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/srv6/datapath/srv6_output.cmodules/srv6/datapath/srv6_local.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
modules/srv6/cli/meson.buildmodules/srv6/cli/localsid.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
modules/srv6/cli/localsid.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:
modules/srv6/cli/localsid.cmodules/srv6/datapath/srv6_local.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/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.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/srv6/control/iface_nh.c
🧬 Code graph analysis (5)
smoke/srv6_l2_encap_test.sh (1)
smoke/_init.sh (2)
netns_add(107-115)move_to_netns(117-131)
modules/infra/cli/gr_cli_iface.h (1)
modules/infra/cli/iface.c (6)
register_iface_type(23-25)register_iface_mode(27-29)type_from_name(55-69)type_from_id(90-99)mode_from_id(101-110)with_iface_set_callback(31-40)
modules/infra/cli/port.c (1)
modules/infra/cli/iface.c (5)
iface_from_name(134-147)complete_iface_names(112-132)iface_from_id(149-157)constructor(665-669)register_iface_mode(27-29)
modules/infra/control/xconnect.c (1)
modules/infra/control/iface.c (1)
iface_mode_register(58-64)
modules/srv6/control/iface_nh.c (4)
modules/infra/control/gr_iface.h (1)
iface(16-25)modules/infra/control/nexthop.c (1)
nexthop_lookup_by_id(491-498)modules/infra/control/iface.c (1)
iface_mode_register(58-64)main/event.c (1)
gr_event_subscribe(16-18)
🪛 Cppcheck (2.18.0)
modules/srv6/datapath/l2_encap.c
[error] 9-9: There is an unknown macro here somewhere. Configuration is required. If GR_MBUF_PRIV_DATA_TYPE is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.8)
smoke/srv6_l2_encap_scapy.py
5-5: from scapy.all import * used; unable to detect undefined names
(F403)
17-17: conf may be undefined, or defined from star imports
(F405)
34-34: sendp may be undefined, or defined from star imports
(F405)
34-34: Ether may be undefined, or defined from star imports
(F405)
39-39: sniff may be undefined, or defined from star imports
(F405)
🪛 Shellcheck (0.11.0)
smoke/srv6_l2_encap_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (8)
.github/workflows/check.yml (1)
76-76: LGTM!Adding
scapyto CI dependencies aligns with the new SRv6 L2 encapsulation test requirements.modules/srv6/control/meson.build (1)
4-8: LGTM!Build file correctly adds the new
iface_nh.csource to the SRv6 control module.modules/infra/control/gr_iface.h (1)
60-75: LGTM!The new
iface_modeAPI follows the established pattern ofiface_typeand provides a clean extension point for mode-specific init/reconfig hooks.smoke/srv6_l2_encap_scapy.py (1)
17-17: Unclear purpose of route lookup.
conf.route6.route()returns routing information but doesn't modify anything. If this is meant to verify the route exists, consider adding an assertion or removing it if unnecessary.modules/srv6/datapath/srv6.h (1)
1-9: LGTM!The private mbuf data type definition is clean and properly structured for carrying nexthop state through the SRv6 DX2 datapath.
modules/infra/control/xconnect.c (1)
38-44: LGTM!The L1 XConnect mode registration follows the established pattern.
modules/srv6/datapath/srv6_output.c (1)
90-93: LGTM!The Ethernet L2 encapsulation path correctly retrieves the nexthop from mbuf private data and uses appropriate payload length and protocol values.
modules/srv6/control/iface_nh.c (1)
1-83: LGTM!The SRv6 interface-nexthop lifecycle management is well-structured. The cleanup handler properly clears stale mode_data references when nexthops are deleted, and the init/reconfig hooks correctly resolve nexthop IDs to pointers.
christophefontaine
force-pushed
the
srv6_l2_dx2
branch
3 times, most recently
from
837e8f5 to
f5c98d0
Compare
christophefontaine
force-pushed
the
srv6_l2_dx2
branch
from
f5c98d0 to
f15039d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (25)
.github/workflows/check.yml(1 hunks)README.md(1 hunks)frr/rt_grout.c(1 hunks)modules/infra/api/gr_infra.h(3 hunks)modules/infra/cli/gr_cli_iface.h(4 hunks)modules/infra/cli/iface.c(12 hunks)modules/infra/cli/port.c(3 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/iface.c(6 hunks)modules/infra/control/xconnect.c(1 hunks)modules/ip/control/icmp.c(0 hunks)modules/ip6/control/icmp6.c(2 hunks)modules/srv6/api/gr_srv6.h(3 hunks)modules/srv6/cli/iface_nh.c(1 hunks)modules/srv6/cli/localsid.c(5 hunks)modules/srv6/cli/meson.build(1 hunks)modules/srv6/control/iface_nh.c(1 hunks)modules/srv6/control/meson.build(1 hunks)modules/srv6/datapath/l2_encap.c(1 hunks)modules/srv6/datapath/meson.build(1 hunks)modules/srv6/datapath/srv6.h(1 hunks)modules/srv6/datapath/srv6_local.c(6 hunks)modules/srv6/datapath/srv6_output.c(2 hunks)smoke/srv6_l2_encap_scapy.py(1 hunks)smoke/srv6_l2_encap_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/ip/control/icmp.c
🚧 Files skipped from review as they are similar to previous changes (11)
- modules/infra/api/gr_infra.h
- modules/srv6/control/meson.build
- .github/workflows/check.yml
- modules/srv6/control/iface_nh.c
- README.md
- modules/srv6/datapath/srv6_output.c
- modules/srv6/datapath/srv6.h
- modules/srv6/datapath/meson.build
- frr/rt_grout.c
- modules/infra/cli/gr_cli_iface.h
- modules/srv6/cli/localsid.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/iface.cmodules/srv6/datapath/srv6_local.cmodules/ip6/control/icmp6.cmodules/infra/cli/iface.cmodules/srv6/api/gr_srv6.hmodules/infra/cli/port.cmodules/infra/control/xconnect.cmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/srv6_l2_encap_test.sh
🧠 Learnings (14)
📚 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:
modules/infra/control/iface.cmodules/srv6/datapath/srv6_local.cmodules/infra/cli/iface.cmodules/srv6/api/gr_srv6.hmodules/infra/control/xconnect.cmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.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/iface.csmoke/srv6_l2_encap_test.shmodules/srv6/datapath/srv6_local.cmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
modules/srv6/cli/meson.build
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
modules/srv6/cli/meson.build
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/srv6/datapath/srv6_local.cmodules/srv6/datapath/l2_encap.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/srv6/datapath/srv6_local.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/datapath/srv6_local.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/datapath/srv6_local.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/srv6/datapath/srv6_local.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/srv6/datapath/srv6_local.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/api/gr_srv6.hmodules/srv6/datapath/l2_encap.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/srv6/cli/iface_nh.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/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/datapath/l2_encap.c
🧬 Code graph analysis (6)
modules/infra/control/iface.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-26)
smoke/srv6_l2_encap_test.sh (1)
smoke/_init.sh (2)
netns_add(107-115)move_to_netns(117-131)
modules/srv6/datapath/srv6_local.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/control/iface.c (1)
iface_from_id(284-291)
modules/infra/control/xconnect.c (1)
modules/infra/control/iface.c (1)
iface_mode_register(58-64)
modules/infra/control/gr_iface.h (1)
modules/infra/control/iface.c (4)
iface_type_register(39-45)iface_mode_register(58-64)iface_type_get(30-37)iface_mode_get(49-56)
modules/srv6/datapath/l2_encap.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(592-619)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(23-26)modules/infra/datapath/port_rx.c (1)
register_interface_mode(33-37)
🪛 Cppcheck (2.18.0)
modules/srv6/cli/iface_nh.c
[warning] 23-23: Uninitialized variable
(uninitvar)
modules/srv6/datapath/l2_encap.c
[error] 9-9: There is an unknown macro here somewhere. Configuration is required. If GR_MBUF_PRIV_DATA_TYPE is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.8)
smoke/srv6_l2_encap_scapy.py
5-5: from scapy.all import * used; unable to detect undefined names
(F403)
17-17: conf may be undefined, or defined from star imports
(F405)
34-34: sendp may be undefined, or defined from star imports
(F405)
34-34: Ether may be undefined, or defined from star imports
(F405)
39-39: sniff may be undefined, or defined from star imports
(F405)
🪛 Shellcheck (0.11.0)
smoke/srv6_l2_encap_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ 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). (6)
- 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 (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-cross-aarch64
🔇 Additional comments (7)
modules/srv6/cli/meson.build (1)
4-8: LGTM!Build file correctly adds the new SRv6 CLI source file following the project's modular build pattern.
modules/srv6/datapath/l2_encap.c (1)
16-44: LGTM!The L2 encapsulation datapath node correctly retrieves the nexthop from
iface->mode_info, handles tracing, and routes mbufs appropriately based on nexthop availability.modules/infra/cli/port.c (1)
192-268: LGTM!The L1 XC mode implementation properly manages resources and follows the project's interface mode registration pattern.
modules/srv6/cli/iface_nh.c (1)
17-27: LGTM!The
nh_from_idhelper correctly streams through nexthops and safely returns NULL if no matching nexthop is found, avoiding the out-of-bounds access issue from the prior approach.modules/srv6/api/gr_srv6.h (1)
100-114: LGTM!The new interface nexthop mapping API follows the existing request type pattern and provides a clean interface for SRv6 L2 encapsulation configuration.
modules/infra/control/iface.c (2)
49-64: LGTM!The
iface_mode_getandiface_mode_registerfunctions properly mirror the existingiface_type_*pattern with appropriate validation and error handling.
96-104: LGTM!The NULL check for mode has been properly added, addressing the previously flagged issue about potential NULL pointer dereference.
Comment on lines
54
to
56
| ip netns exec n1 python3 smoke/srv6_l2_encap_scapy.py & | ||
| sleep 3 |
There was a problem hiding this comment.
Verify the background Python script started successfully.
Starting the Python script in the background without checking if it launched successfully can lead to confusing test failures. If the script fails to start (e.g., missing scapy dependency, wrong path, permission issues), the ping test will fail with an error message that doesn't indicate the root cause.
🔎 Add verification after starting the background process:
ip netns exec n1 python3 smoke/srv6_l2_encap_scapy.py &
+SCAPY_PID=$!
sleep 3
+
+# Verify the scapy script is still running
+if ! kill -0 $SCAPY_PID 2>/dev/null; then
+ echo "ERROR: SRv6 encapsulation script failed to start"
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ip netns exec n1 python3 smoke/srv6_l2_encap_scapy.py & | |
| sleep 3 | |
| ip netns exec n1 python3 smoke/srv6_l2_encap_scapy.py & | |
| SCAPY_PID=$! | |
| sleep 3 | |
| # Verify the scapy script is still running | |
| if ! kill -0 $SCAPY_PID 2>/dev/null; then | |
| echo "ERROR: SRv6 encapsulation script failed to start" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In smoke/srv6_l2_encap_test.sh around lines 54-55, after launching the Python
script in the background you must verify it actually started: start the process
with its stdout/stderr redirected to a logfile, capture its PID ($!)
immediately, wait a short period, then check the PID is still alive (e.g., kill
-0 $PID or ps -p $PID) and if not, print the logfile and exit the test with a
non-zero status so failures surface immediately.
rjarry
reviewed
Dec 18, 2025
Comment on lines
-189
to
-191
| struct icmp_queue_item *i, *tmp; | ||
| STAILQ_FOREACH_SAFE (i, &icmp_queue, next, tmp) | ||
| icmp_queue_pop(i, true); |
Collaborator
There was a problem hiding this comment.
Could you note in the commit message that it happens because:
- Since icmp{,6}_init do not depend on any other init callbacks, they are executed first. And their _fini counterparts are executed last.
- Other _fini routines are executed before them (such as iface_fini or port_fini). These callbacks actually free the mbuf memory pools used by datapath.
- Packets present in the icmp{,6} queues contain references to the mbufs which were freed by iface_fini. When icmp{,6}_fini try to free them again, it causes a double free (reported as use-after-free).
Comment on lines
+550
to
+559
| while ((iface_mode = ec_node_find_next(root, iface_mode, "IFACE_MODE"))) { | ||
| STAILQ_FOREACH (mode, &modes, next) { | ||
| if ((ret = mode->init(iface_mode)) < 0) | ||
| return ret; | ||
| } |
Collaborator
There was a problem hiding this comment.
Can you add a comment explaining that there are as many IFACE_MODE nodes as number of interfaces and that we need to call mode->init() on all of them?
Comment on lines
215
to
235
| with_help( | ||
| "l1 cross connect", | ||
| EC_NODE_SEQ( | ||
| "", | ||
| ec_node_str("", "xconnect"), | ||
| with_iface_set_callback( | ||
| l1_xc_set_peer, | ||
| with_help( | ||
| "Peer interface for xconnect", | ||
| ec_node_dyn( | ||
| "PEER", | ||
| complete_iface_names, | ||
| INT2PTR(GR_IFACE_TYPE_PORT) |
Collaborator
There was a problem hiding this comment.
This will insert the l1 mode in all interface types (vlan, bond, ipip). It would be interesting to only have this mode available on ports.
modules/srv6/cli/iface_nh.c
Outdated
Comment on lines
22
to
25
| gr_api_client_stream_foreach (nh, ret, c, GR_NH_LIST, sizeof(req), &req) { | ||
| if (nh->nh_id == nh_id) | ||
| return nh; | ||
| } |
Collaborator
There was a problem hiding this comment.
Returning before the end of the loop causes a memory leak. The allocated nh will not be freed. Also, all messages in the stream response must be consumed.
| uint16_t vrf_id; // L3 addressing and routing domain | ||
| uint16_t domain_id; // L2 xconnect peer interface id | ||
| }; | ||
| uint32_t mode_data; // Abstract mode data |
Collaborator
There was a problem hiding this comment.
This feels a bit hacky. Could you explain the reasoning behind this opaque parameter?
Collaborator
Author
There was a problem hiding this comment.
Similar to the type specific info, we need a mode specific info.
Yet, we can't have 2 VLA at then end of the structure. And yes, that's a bit hacky :(
modules/srv6/datapath/srv6_local.c
Outdated
| m->hash.usr = rte_be_to_cpu_32(ip6_info->ip6_hdr->vtc_flow) & RTE_IPV6_HDR_FL_MASK; | ||
| rte_pktmbuf_adj(m, ip6_info->ext_offset); | ||
| return edge; | ||
| break; |
Collaborator
There was a problem hiding this comment.
I don't really like how it's coded here.
I would prefer have 2 functions:
process_behav_ip_decap and process_behave_decap_eth
Else you need to factorize more the code in process_behav_decap to manage both case eth & ip.
Collaborator
Author
There was a problem hiding this comment.
That's fair. I'll split that.
| iface = iface_from_name(c, iface_name); | ||
| if (iface == NULL) | ||
| goto out; | ||
| sr6->out_vrf_id = iface->id; |
Collaborator
There was a problem hiding this comment.
Using out_vrf_id for iface->id is really odd.
modules/srv6/cli/localsid.c
Outdated
| EC_NODE_CMD( | ||
| EC_NO_ID, | ||
| "(end.dt4|end.dt6|end.dt46) [table TABLE]", | ||
| "((end.dx2 IFACE)|end.dt4|end.dt6|end.dt46) [table TABLE]", |
Collaborator
There was a problem hiding this comment.
does it means we can specifiy a table with end.dx2 ? it does'nt make any sense.
I think we should rename table to vrf in an other commit;
Collaborator
Author
There was a problem hiding this comment.
indeed, I'll update this to
((end.dx2 IFACE))|((end.dt4|end.dt6|end.dt46) [table TABLE])
maxime-leroy
requested changes
Dec 23, 2025
| dst=SEGMENTS[0] | ||
| ) / srh / pkt | ||
|
|
||
| sendp(Ether() / outer_ip6, iface=SR6IFACE, verbose=False) |
Collaborator
There was a problem hiding this comment.
Why using scapy here ? END.DX2 is supporterd by kernel ?
Collaborator
Author
There was a problem hiding this comment.
I was able to configure the end.dx2, yes.
Yet, I wasn't able to encapsulate the l2 frame: the only way to do this even with a recent kernel would be to attach an eBPF helper on the interface.
Instead of doing half in conf, and the other half in python (or eBPF), I chose to do everything with scapy.
| if (gr_mbuf_is_traced(mbuf)) | ||
| gr_mbuf_trace_add(mbuf, node, 0); | ||
|
|
||
| rte_node_enqueue_x1(graph, node, nh ? ENCAP : NO_DATA, mbuf); |
Collaborator
There was a problem hiding this comment.
This node do really nothing, no ?
Why not put this code in srv6_output.c
And direclty call a sub function srv6_encap ?
In which case nh can be NULL ?
Collaborator
Author
There was a problem hiding this comment.
quite the opposite: this node replaces the eth_input node for a specific interface.
Without it, we can't enter into srv6_output node.
| nh = srv6_dx2_mbuf_data(m)->nh; | ||
| plen = rte_pktmbuf_data_len(m); | ||
| proto = IPPROTO_ETHERNET; | ||
| } else { |
Collaborator
There was a problem hiding this comment.
The code below to do the encap can be easily extracted. Instead of having 2 nodes to just do L2 encap.
Collaborator
Author
There was a problem hiding this comment.
you'd want to create a static inline func to do the encap ?
I'm not against it.
icmp and icmp6 have the same static symbols for init/fini. While this isn't an issue, it is better to have unique function names to have an non ambiguous callstack or in a debug session. Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
Similar to "control_output: fix use-after-free on shutdown",
packets are still in the icmp(6) queue when grout quits, which ends up
into a 'use-after-free':
==51938==ERROR: AddressSanitizer: heap-use-after-free on address 0x7f6e3084f080 at pc 0x560293c3dc87 bp 0x7ffe3e9298b0 sp 0x7ffe3e9298a0
READ of size 8 at 0x7f6e3084f080 thread T0
#0 0x560293c3dc86 in rte_pktmbuf_free ../subprojects/dpdk/lib/mbuf/rte_mbuf.h:1522
DPDK#1 0x560293c42a8a in icmp6_queue_pop ../modules/ip6/control/icmp6.c:30
DPDK#2 0x560293c49cb8 in icmp_fini ../modules/ip6/control/icmp6.c:177
DPDK#3 0x560293ac1068 in modules_fini ../main/module.c:123
DPDK#4 0x560293abfb52 in main ../main/main.c:326
DPDK#5 0x7f6e5ca2a1c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
DPDK#6 0x7f6e5ca2a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
DPDK#7 0x560293ab5974 in _start (/home/runner/work/grout/grout/build/grout+0x312974) (BuildId: 9f73d7b419f769b2197f675b4a3d768d8259e548)
Similar to other modules, don't attempt to free individual elements.
Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
In the control folder, decoupling allows more flexibility when a new interface mode is added, with a new callback when the interface mode is updated. For the CLI, add iface mode callbacks similar to iface type callbacks. This allows additional flexibility in interface mode definition. Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
christophefontaine
force-pushed
the
srv6_l2_dx2
branch
from
f15039d to
ae02ff1
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @modules/infra/cli/iface.c:
- Around line 236-247: When ec_pnode_find(node, "l3") is false and
find_cmd_iface_mode_callback(node) returns NULL (with errno=EOPNOTSUPP), the
code currently ignores this and proceeds as if successful; update the branch
handling around find_cmd_iface_mode_callback so that if cb is NULL you jump to
the existing err: cleanup path (same behavior as other parse errors) instead of
continuing, ensuring the function returns failure rather than reporting success
when an unsupported mode is requested; adjust references around
find_cmd_iface_mode_callback, cb, set_attrs and the err: label to mirror the
error propagation used elsewhere in this function.
In @modules/srv6/control/iface_nh.c:
- Around line 16-32: The cleanup function srv6_iface_nh_cleanup_nexthop
currently stops after clearing the first matching interface, leaving other
interfaces with dangling references; modify the loop in
srv6_iface_nh_cleanup_nexthop (which uses iface_next and checks iface->mode ==
GR_IFACE_MODE_SRV6_XC and iface->mode_data == nh->nh_id) to clear
iface->mode_data = (uintptr_t)0 and iface->mode_info = NULL for every matching
interface instead of returning early—remove the early return so the while loop
continues through all interfaces and clears all references to nh->nh_id.
In @modules/srv6/datapath/srv6_local.c:
- Around line 284-315: process_behav_decap_eth stores a possibly-NULL iface into
the mbuf via mbuf_data(m)->iface after calling iface_from_id(sr_d->out_vrf_id);
change the logic to match the pattern in process_behav_decap_ip: only assign
mbuf_data(m)->iface when iface_from_id(...) returns non-NULL, and if iface is
NULL return DEST_UNREACH without modifying the mbuf; keep the same determination
of edge (PORT_OUTPUT/BOND_OUTPUT) when iface is valid and preserve the existing
m->hash.usr and rte_pktmbuf_adj(...) steps only in the success path before
returning the edge.
In @smoke/srv6_l2_encap_scapy.py:
- Line 17: The call conf.route6.route(SEGMENTS[0]) discards the returned routing
info (unused lookup); either remove it if it was only incidental, or replace it
with a route configuration call conf.route6.add(...) to actually install the
route using the same SEGMENTS[0] value. Locate the invocation of
conf.route6.route in the script (search for conf.route6.route and SEGMENTS) and
either delete the unused lookup or change it to conf.route6.add(...) with the
appropriate parameters so the route is configured rather than ignored.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (25)
.github/workflows/check.ymlREADME.mdfrr/rt_grout.cmodules/infra/api/gr_infra.hmodules/infra/cli/gr_cli_iface.hmodules/infra/cli/iface.cmodules/infra/cli/port.cmodules/infra/control/gr_iface.hmodules/infra/control/iface.cmodules/infra/control/xconnect.cmodules/ip/control/icmp.cmodules/ip6/control/icmp6.cmodules/srv6/api/gr_srv6.hmodules/srv6/cli/iface_nh.cmodules/srv6/cli/localsid.cmodules/srv6/cli/meson.buildmodules/srv6/control/iface_nh.cmodules/srv6/control/meson.buildmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/meson.buildmodules/srv6/datapath/srv6.hmodules/srv6/datapath/srv6_local.cmodules/srv6/datapath/srv6_output.csmoke/srv6_l2_encap_scapy.pysmoke/srv6_l2_encap_test.sh
💤 Files with no reviewable changes (1)
- modules/ip/control/icmp.c
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/check.yml
- modules/infra/control/iface.c
- README.md
- modules/srv6/control/meson.build
- modules/infra/cli/gr_cli_iface.h
- modules/infra/control/gr_iface.h
- frr/rt_grout.c
- modules/srv6/cli/meson.build
- modules/srv6/datapath/srv6.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/api/gr_infra.hmodules/srv6/api/gr_srv6.hmodules/infra/cli/iface.cmodules/infra/cli/port.cmodules/srv6/control/iface_nh.cmodules/infra/control/xconnect.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_local.cmodules/srv6/cli/localsid.cmodules/srv6/datapath/srv6_output.cmodules/ip6/control/icmp6.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/srv6_l2_encap_test.sh
🧠 Learnings (18)
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
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:
modules/infra/api/gr_infra.hmodules/srv6/api/gr_srv6.hmodules/infra/cli/iface.cmodules/srv6/control/iface_nh.cmodules/infra/control/xconnect.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_local.cmodules/srv6/datapath/srv6_output.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/api/gr_infra.hmodules/srv6/control/iface_nh.csmoke/srv6_l2_encap_test.shmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_local.cmodules/srv6/cli/localsid.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
modules/infra/api/gr_infra.h
📚 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:
modules/srv6/api/gr_srv6.hmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_output.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/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.cmodules/ip6/control/icmp6.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/srv6/control/iface_nh.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/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_local.cmodules/srv6/cli/localsid.cmodules/srv6/datapath/srv6_output.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:
modules/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_local.cmodules/srv6/datapath/srv6_output.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/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/cli/iface_nh.cmodules/srv6/datapath/srv6_local.cmodules/srv6/cli/localsid.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_local.cmodules/srv6/datapath/srv6_output.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/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_local.cmodules/srv6/datapath/srv6_output.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:
modules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_output.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/srv6/datapath/srv6_local.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
modules/srv6/cli/localsid.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
modules/srv6/cli/localsid.c
📚 Learning: 2025-12-18T16:04:28.693Z
Learnt from: rjarry
Repo: DPDK/grout PR: 466
File: modules/ip6/control/icmp6.c:173-178
Timestamp: 2025-12-18T16:04:28.693Z
Learning: In modules that manage ICMP packet queues (e.g., icmp.c and icmp6.c), do not manually free mbufs from icmp_queue in fini functions if those mbufs are owned by other pktmbuf pools that are freed elsewhere. Rely on the ownership of those pools and free the icmp_queue_item mempool as the last step, avoiding double frees. When reviewing, verify that icmp_fini/icmp6_fini do not attempt to free mbufs from icmp_queue that are owned by other pools, and ensure no memory being freed twice. Apply this check to similarly structured ICMP handling code in other architectures if present.
Applied to files:
modules/ip6/control/icmp6.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/ip6/control/icmp6.c
🧬 Code graph analysis (6)
modules/infra/cli/iface.c (2)
api/gr_errno.h (1)
errno_set_null(16-19)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/port.c (2)
modules/infra/cli/iface.c (5)
iface_from_name(134-147)complete_iface_names(112-132)iface_from_id(149-157)constructor(670-674)register_iface_mode(27-29)modules/infra/control/iface.c (1)
iface_from_id(284-291)
modules/srv6/control/iface_nh.c (4)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/control/nexthop.c (1)
nexthop_lookup_by_id(491-498)modules/infra/control/iface.c (1)
iface_mode_register(58-64)main/event.c (1)
gr_event_subscribe(16-18)
modules/infra/control/xconnect.c (1)
modules/infra/control/iface.c (1)
iface_mode_register(58-64)
smoke/srv6_l2_encap_test.sh (3)
smoke/_init.sh (2)
netns_add(107-115)move_to_netns(117-131)modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/control/gr_nh_control.h (1)
nexthop(22-30)
modules/srv6/datapath/srv6_local.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/control/iface.c (1)
iface_from_id(284-291)
🪛 Cppcheck (2.19.0)
modules/srv6/cli/iface_nh.c
[warning] 23-23: Uninitialized variable
(uninitvar)
modules/srv6/datapath/l2_encap.c
[error] 9-9: There is an unknown macro here somewhere. Configuration is required. If GR_MBUF_PRIV_DATA_TYPE is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.10)
smoke/srv6_l2_encap_scapy.py
5-5: from scapy.all import * used; unable to detect undefined names
(F403)
17-17: conf may be undefined, or defined from star imports
(F405)
34-34: sendp may be undefined, or defined from star imports
(F405)
34-34: Ether may be undefined, or defined from star imports
(F405)
39-39: sniff may be undefined, or defined from star imports
(F405)
🪛 Shellcheck (0.11.0)
smoke/srv6_l2_encap_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ 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: deb
- 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 (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: rpm
🔇 Additional comments (10)
modules/ip6/control/icmp6.c (1)
173-178: LGTM: Correct implementation per mbuf ownership model.The removal of queue draining is correct. Based on learnings, the mbufs in
icmp_queueare owned by other pktmbuf pools that are freed elsewhere, so manually freeing them here would risk double-frees. The simplified fini path (freeing only the mempool) is the proper approach.modules/srv6/api/gr_srv6.h (2)
59-66: LGTM!The SR_BEHAVIOR_END_DX2 enum value 0x0015 correctly follows the IANA SRv6 Endpoint Behaviors registry, and the string mapping is properly added.
103-114: API endpoints defined for interface-nexthop mapping.These request types and structures define the control plane interface for SRv6 L2 encapsulation. The structures are correctly aligned and sized.
modules/infra/api/gr_infra.h (2)
51-56: LGTM!The new
GR_IFACE_MODE_SRV6_XCmode andmode_datafield cleanly extend the interface infrastructure to support SRv6 L2 encapsulation without breaking existing structures.Also applies to: 78-78
457-458: LGTM!String mapping for the new mode is correctly added.
smoke/srv6_l2_encap_scapy.py (1)
19-39: LGTM!The encapsulation logic correctly builds the SRv6 header with next-header 143 (Ethernet) and properly skips already-encapsulated packets to prevent double encapsulation.
smoke/srv6_l2_encap_test.sh (1)
54-64: LGTM!Good addition of the background process verification - this will surface failures in the scapy script early rather than causing confusing ping failures.
modules/infra/control/xconnect.c (1)
38-44: LGTM!The L1 XC mode is correctly registered with
GR_IFACE_MODE_L1_XCand the constructor properly registers the mode before the API handler.modules/srv6/datapath/meson.build (1)
4-8: LGTM!Build file correctly includes the new l2_encap.c source.
modules/srv6/datapath/srv6_output.c (1)
90-94: LGTM!The L2 Ethernet handling path correctly extracts the nexthop from
srv6_dx2_mbuf_dataand setsproto = IPPROTO_ETHERNET(143) for the SRv6 encapsulation. Theplencalculation usingrte_pktmbuf_data_len(m)is appropriate for raw Ethernet frames.
Comment on lines
+236
to
+247
| if ((node = ec_pnode_find(p, "IFACE_MODE")) != NULL) { | ||
| if (ec_pnode_find(node, "l3")) { | ||
| set_attrs |= GR_IFACE_SET_MODE; | ||
| set_attrs |= GR_IFACE_SET_VRF; | ||
| iface->mode = GR_IFACE_MODE_L3; | ||
| iface->vrf_id = 0; | ||
| } else { | ||
| cmd_iface_set_cb_t cb = find_cmd_iface_mode_callback(node); | ||
| if (cb) | ||
| cb(c, iface, node, &set_attrs); | ||
| } | ||
| } |
There was a problem hiding this comment.
Mode configuration failure is silently ignored.
Lines 243-245: when find_cmd_iface_mode_callback() returns NULL (setting errno = EOPNOTSUPP), the code silently skips the callback without reporting an error. The function continues and may return success even though the requested mode was not configured.
This differs from other parse errors in this function, which jump to the err: label and return 0. A user invoking interface set <name> mode <unsupported> would see the command succeed even though the mode was not applied.
🔧 Propagate the error
} else {
cmd_iface_set_cb_t cb = find_cmd_iface_mode_callback(node);
- if (cb)
- cb(c, iface, node, &set_attrs);
+ if (cb == NULL)
+ goto err;
+ cb(c, iface, node, &set_attrs);
}
}🤖 Prompt for AI Agents
In @modules/infra/cli/iface.c around lines 236 - 247, When ec_pnode_find(node,
"l3") is false and find_cmd_iface_mode_callback(node) returns NULL (with
errno=EOPNOTSUPP), the code currently ignores this and proceeds as if
successful; update the branch handling around find_cmd_iface_mode_callback so
that if cb is NULL you jump to the existing err: cleanup path (same behavior as
other parse errors) instead of continuing, ensuring the function returns failure
rather than reporting success when an unsupported mode is requested; adjust
references around find_cmd_iface_mode_callback, cb, set_attrs and the err: label
to mirror the error propagation used elsewhere in this function.
ae02ff1 to
b4e41b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @modules/srv6/control/iface_nh.c:
- Around line 16-32: The cleanup function srv6_iface_nh_cleanup_nexthop
incorrectly returns after clearing only the first matching iface, leaving other
interfaces with dangling mode_info; remove the early `return` inside the loop so
the while loop continues scanning all interfaces and clears iface->mode_data and
iface->mode_info for every iface whose mode is GR_IFACE_MODE_SRV6_XC and whose
mode_data equals nh->nh_id (use the existing uintptr_t comparison), ensuring all
references to the nexthop are cleared before exiting the function.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
.github/workflows/check.ymlREADME.mdfrr/rt_grout.cmodules/infra/api/gr_infra.hmodules/infra/control/gr_iface.hmodules/srv6/api/gr_srv6.hmodules/srv6/cli/iface_nh.cmodules/srv6/cli/localsid.cmodules/srv6/cli/meson.buildmodules/srv6/control/iface_nh.cmodules/srv6/control/meson.buildmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/meson.buildmodules/srv6/datapath/srv6.hmodules/srv6/datapath/srv6_local.cmodules/srv6/datapath/srv6_output.csmoke/srv6_l2_encap_scapy.pysmoke/srv6_l2_encap_test.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/check.yml
- modules/srv6/control/meson.build
- frr/rt_grout.c
- README.md
- modules/srv6/api/gr_srv6.h
- modules/srv6/datapath/srv6_output.c
- modules/srv6/cli/localsid.c
- modules/infra/api/gr_infra.h
- modules/srv6/datapath/srv6_local.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/srv6_l2_encap_test.sh
**/*.{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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/srv6.hmodules/srv6/datapath/l2_encap.c
🧠 Learnings (18)
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
modules/srv6/cli/meson.build
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
modules/srv6/cli/meson.build
📚 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:
smoke/srv6_l2_encap_test.shmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
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:
modules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/srv6.hmodules/srv6/datapath/l2_encap.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/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.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/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.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/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/cli/iface_nh.c
📚 Learning: 2025-10-03T13:34:31.801Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.801Z
Learning: The `gr_api_client_stream_foreach` macro in `api/gr_api.h` always sets its `ret` parameter during the first outer loop condition evaluation (via `if (__first == 1) ret = __id;`), even when the stream yields zero items. Therefore, the `ret` variable does not need to be initialized at declaration when used with this macro.
Applied to files:
modules/srv6/cli/iface_nh.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/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.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:
smoke/srv6_l2_encap_scapy.pymodules/srv6/control/iface_nh.c
📚 Learning: 2025-12-18T16:04:37.486Z
Learnt from: rjarry
Repo: DPDK/grout PR: 466
File: modules/ip6/control/icmp6.c:173-178
Timestamp: 2025-12-18T16:04:37.486Z
Learning: In modules/ip/control/icmp.c and modules/ip6/control/icmp6.c, the mbufs stored in icmp_queue are owned by other pktmbuf pools that are freed elsewhere. It is not necessary to free these mbufs in icmp_fini/icmp6_fini before calling rte_mempool_free on the icmp_queue_item mempool.
Applied to files:
modules/srv6/control/iface_nh.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/srv6/control/iface_nh.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/srv6/datapath/l2_encap.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/srv6/datapath/l2_encap.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:
modules/srv6/datapath/l2_encap.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:
modules/srv6/datapath/l2_encap.c
🧬 Code graph analysis (4)
smoke/srv6_l2_encap_test.sh (1)
smoke/_init.sh (2)
netns_add(107-115)move_to_netns(117-131)
modules/infra/control/gr_iface.h (1)
modules/infra/control/iface.c (4)
iface_type_register(39-45)iface_mode_register(58-64)iface_type_get(30-37)iface_mode_get(49-56)
modules/srv6/cli/iface_nh.c (4)
modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/gr_cli.h (1)
arg_u32(77-83)cli/ecoli.c (1)
with_help(11-20)modules/infra/cli/iface.c (2)
with_iface_set_callback(31-40)register_iface_mode(27-29)
modules/srv6/control/iface_nh.c (4)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/control/nexthop.c (1)
nexthop_lookup_by_id(491-498)modules/infra/control/iface.c (1)
iface_mode_register(58-64)main/event.c (1)
gr_event_subscribe(16-18)
🪛 Cppcheck (2.19.0)
modules/srv6/cli/iface_nh.c
[warning] 23-23: Uninitialized variable
(uninitvar)
modules/srv6/datapath/l2_encap.c
[error] 9-9: There is an unknown macro here somewhere. Configuration is required. If GR_MBUF_PRIV_DATA_TYPE is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.10)
smoke/srv6_l2_encap_scapy.py
5-5: from scapy.all import * used; unable to detect undefined names
(F403)
33-33: sendp may be undefined, or defined from star imports
(F405)
33-33: Ether may be undefined, or defined from star imports
(F405)
36-36: sniff may be undefined, or defined from star imports
(F405)
🪛 Shellcheck (0.11.0)
smoke/srv6_l2_encap_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ 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-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 (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (2)
smoke/srv6_l2_encap_test.sh (1)
58-62: Verification implementation is adequate.The process liveness check successfully addresses the previous concern about detecting startup failures. While capturing logs would provide better diagnostics on failure, the current implementation is sufficient for a smoke test.
smoke/srv6_l2_encap_scapy.py (1)
22-33: SRv6 L2 encapsulation logic is correct.The SRH next header value (nh=143) correctly indicates Ethernet encapsulation per RFC 8986. The encapsulation structure (Ether/IPv6/SRH/OriginalEther) is appropriate for SRv6 End.DX2 behavior.
christophefontaine
force-pushed
the
srv6_l2_dx2
branch
from
b4e41b2 to
e3f9082
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @modules/srv6/control/iface_nh.c:
- Around line 16-32: In srv6_iface_nh_cleanup_nexthop the function exits early
after clearing only the first interface referencing the nexthop (due to the
return inside the loop), leaving other interfaces with dangling mode_info;
modify srv6_iface_nh_cleanup_nexthop so that instead of returning after clearing
one match it continues scanning all interfaces and clears iface->mode_data and
iface->mode_info for every iface where iface->mode == GR_IFACE_MODE_SRV6_XC and
iface->mode_data == nh->nh_id; remove the early return and ensure the loop
completes to clear all references to nh->nh_id.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
.github/workflows/check.ymlREADME.mdmodules/infra/api/gr_infra.hmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/cli/localsid.cmodules/srv6/cli/meson.buildmodules/srv6/control/iface_nh.cmodules/srv6/control/meson.buildmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/meson.buildmodules/srv6/datapath/srv6.hmodules/srv6/datapath/srv6_output.csmoke/srv6_l2_encap_scapy.pysmoke/srv6_l2_encap_test.sh
✅ Files skipped from review due to trivial changes (1)
- modules/srv6/cli/meson.build
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/check.yml
- modules/srv6/control/meson.build
- modules/srv6/datapath/srv6.h
- README.md
- modules/infra/api/gr_infra.h
- modules/srv6/cli/localsid.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/srv6/cli/iface_nh.cmodules/srv6/datapath/srv6_output.cmodules/infra/control/gr_iface.hmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/srv6_l2_encap_test.sh
🧠 Learnings (16)
📓 Common learnings
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.
📚 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/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
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:
modules/srv6/cli/iface_nh.cmodules/srv6/datapath/srv6_output.cmodules/infra/control/gr_iface.hmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.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/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.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/srv6/cli/iface_nh.cmodules/srv6/datapath/srv6_output.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/cli/iface_nh.cmodules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/cli/iface_nh.c
📚 Learning: 2025-10-03T13:34:31.801Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.801Z
Learning: The `gr_api_client_stream_foreach` macro in `api/gr_api.h` always sets its `ret` parameter during the first outer loop condition evaluation (via `if (__first == 1) ret = __id;`), even when the stream yields zero items. Therefore, the `ret` variable does not need to be initialized at declaration when used with this macro.
Applied to files:
modules/srv6/cli/iface_nh.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/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.c
📚 Learning: 2025-12-18T16:04:37.486Z
Learnt from: rjarry
Repo: DPDK/grout PR: 466
File: modules/ip6/control/icmp6.c:173-178
Timestamp: 2025-12-18T16:04:37.486Z
Learning: In modules/ip/control/icmp.c and modules/ip6/control/icmp6.c, the mbufs stored in icmp_queue are owned by other pktmbuf pools that are freed elsewhere. It is not necessary to free these mbufs in icmp_fini/icmp6_fini before calling rte_mempool_free on the icmp_queue_item mempool.
Applied to files:
modules/srv6/cli/iface_nh.cmodules/srv6/control/iface_nh.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/srv6/cli/iface_nh.cmodules/infra/control/gr_iface.hmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.csmoke/srv6_l2_encap_test.sh
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.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:
modules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.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/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.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/srv6/control/iface_nh.csmoke/srv6_l2_encap_scapy.py
🧬 Code graph analysis (3)
modules/srv6/cli/iface_nh.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/gr_cli.h (1)
arg_u32(77-83)modules/infra/cli/iface.c (1)
with_iface_set_callback(31-40)modules/srv6/cli/localsid.c (1)
constructor(235-238)
modules/infra/control/gr_iface.h (3)
modules/infra/control/iface.c (4)
iface_type_register(39-45)iface_mode_register(58-64)iface_type_get(30-37)iface_mode_get(49-56)modules/infra/control/port_test.c (1)
iface_type_register(23-23)modules/infra/control/worker_test.c (1)
iface_type_register(36-36)
smoke/srv6_l2_encap_test.sh (1)
smoke/_init.sh (2)
netns_add(107-115)move_to_netns(117-131)
🪛 Cppcheck (2.19.0)
modules/srv6/cli/iface_nh.c
[warning] 23-23: Uninitialized variable
(uninitvar)
modules/srv6/datapath/l2_encap.c
[error] 9-9: There is an unknown macro here somewhere. Configuration is required. If GR_MBUF_PRIV_DATA_TYPE is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.10)
smoke/srv6_l2_encap_scapy.py
5-5: from scapy.all import * used; unable to detect undefined names
(F403)
33-33: sendp may be undefined, or defined from star imports
(F405)
33-33: Ether may be undefined, or defined from star imports
(F405)
36-36: sniff may be undefined, or defined from star imports
(F405)
🪛 Shellcheck (0.11.0)
smoke/srv6_l2_encap_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ 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-18, none, debugoptimized, ubuntu-24.04, false)
- 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 (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: rpm
- GitHub Check: deb
modules/srv6/cli/iface_nh.c
Outdated
Comment on lines
17
to
28
| static struct gr_nexthop *nh_from_id(struct gr_api_client *c, uint32_t nh_id) { | ||
| struct gr_nh_list_req req = {.vrf_id = GR_VRF_ID_ALL, .type = GR_NH_T_SR6_OUTPUT}; | ||
| struct gr_nexthop *nh, *found = NULL; | ||
| int ret; | ||
|
|
||
| gr_api_client_stream_foreach (nh, ret, c, GR_NH_LIST, sizeof(req), &req) { | ||
| if (nh->nh_id == nh_id) | ||
| found = nh; | ||
| } | ||
|
|
||
| return found; | ||
| } |
There was a problem hiding this comment.
Critical: Memory leak - non-matching nexthop objects not freed.
The stream loop allocates a new gr_nexthop object on each iteration. Only the matching nexthop is saved in found (line 23), while all non-matching objects are leaked. Every nexthop object retrieved from the stream must be freed except the one being returned.
🔧 Required fix
static struct gr_nexthop *nh_from_id(struct gr_api_client *c, uint32_t nh_id) {
struct gr_nh_list_req req = {.vrf_id = GR_VRF_ID_ALL, .type = GR_NH_T_SR6_OUTPUT};
struct gr_nexthop *nh, *found = NULL;
int ret;
gr_api_client_stream_foreach (nh, ret, c, GR_NH_LIST, sizeof(req), &req) {
- if (nh->nh_id == nh_id)
+ if (nh->nh_id == nh_id && found == NULL)
found = nh;
+ else
+ free(nh);
}
return found;
}🧰 Tools
🪛 Cppcheck (2.19.0)
[warning] 23-23: Uninitialized variable
(uninitvar)
Implement "end.dx2" behavior, which removes the IPv6 header and forward the ethernet frame to a specified interface. Even if FRR doesn't support (yet) the behavior end dx.2, take it into account to build properly the plugin. Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
Add L2 encapsulation from an interface: all packets received on that interface will be encapsulated and sent to the srv6_output node. As it is hard to get l2encap mode with linux, use scapy to encap the packets in the smoke test. Update README and github workflow dependencies. Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
christophefontaine
force-pushed
the
srv6_l2_dx2
branch
from
e3f9082 to
9f9c684
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @frr/rt_grout.c:
- Around line 732-736: Replace the incorrect GR_VRF_ID_ALL usage: for
ZEBRA_SEG6_LOCAL_ACTION_END_DX2 set sr6_local->out_vrf_id by mapping the FRR
interface index taken from the seg6local context on the nexthop (i.e., use
ifindex_frr_to_grout(nh->nh_srv6->seg6local_ctx.ifindex) or the actual
seg6local_ctx field that holds the output interface), so the code mirrors the
pattern used by other behaviors (e.g., END_DT6 uses
nh->nh_srv6->seg6local_ctx.table) and does not pass a VRF constant to
ifindex_frr_to_grout.
In @modules/srv6/datapath/srv6_local.c:
- Around line 284-316: process_behav_decap_eth has an uninitialized local 'edge'
used when iface->type is neither GR_IFACE_TYPE_PORT nor GR_IFACE_TYPE_BOND;
ensure you handle the unknown iface type explicitly (e.g., set edge to a safe
sentinel or return an error like DEST_UNREACH) instead of falling through, by
adding an else branch after the iface->type checks that either assigns edge a
defined value or returns an error; update references to edge accordingly in
process_behav_decap_eth so the function cannot return an uninitialized value.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
.github/workflows/check.ymlREADME.mdfrr/rt_grout.cmodules/infra/api/gr_infra.hmodules/infra/control/gr_iface.hmodules/srv6/api/gr_srv6.hmodules/srv6/cli/iface_nh.cmodules/srv6/cli/localsid.cmodules/srv6/cli/meson.buildmodules/srv6/control/iface_nh.cmodules/srv6/control/meson.buildmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/meson.buildmodules/srv6/datapath/srv6.hmodules/srv6/datapath/srv6_local.cmodules/srv6/datapath/srv6_output.csmoke/srv6_l2_encap_scapy.pysmoke/srv6_l2_encap_test.sh
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/check.yml
- modules/srv6/control/meson.build
- modules/srv6/datapath/srv6.h
- modules/infra/api/gr_infra.h
- README.md
- modules/srv6/datapath/srv6_output.c
- modules/srv6/cli/meson.build
- modules/srv6/cli/localsid.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
frr/rt_grout.cmodules/srv6/api/gr_srv6.hmodules/srv6/datapath/srv6_local.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/srv6_l2_encap_test.sh
🧠 Learnings (18)
📓 Common learnings
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.
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.
📚 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.cmodules/srv6/datapath/srv6_local.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/srv6/cli/iface_nh.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.cmodules/srv6/datapath/srv6_local.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/srv6/cli/iface_nh.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.cmodules/srv6/datapath/srv6_local.cmodules/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.csmoke/srv6_l2_encap_scapy.py
📚 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.cmodules/srv6/datapath/srv6_local.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/srv6/cli/iface_nh.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/srv6/datapath/srv6_local.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
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:
modules/srv6/datapath/srv6_local.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/l2_encap.cmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.c
📚 Learning: 2025-12-18T16:04:37.486Z
Learnt from: rjarry
Repo: DPDK/grout PR: 466
File: modules/ip6/control/icmp6.c:173-178
Timestamp: 2025-12-18T16:04:37.486Z
Learning: In modules/ip/control/icmp.c and modules/ip6/control/icmp6.c, the mbufs stored in icmp_queue are owned by other pktmbuf pools that are freed elsewhere. It is not necessary to free these mbufs in icmp_fini/icmp6_fini before calling rte_mempool_free on the icmp_queue_item mempool.
Applied to files:
modules/srv6/datapath/srv6_local.cmodules/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.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/srv6/datapath/srv6_local.cmodules/srv6/datapath/l2_encap.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/srv6/datapath/srv6_local.cmodules/srv6/control/iface_nh.csmoke/srv6_l2_encap_test.shmodules/srv6/datapath/l2_encap.cmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.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:
modules/srv6/datapath/srv6_local.cmodules/srv6/cli/iface_nh.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/srv6/datapath/srv6_local.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/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.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/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.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:
modules/srv6/datapath/l2_encap.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:
modules/srv6/datapath/l2_encap.c
📚 Learning: 2025-10-03T13:34:31.801Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.801Z
Learning: The `gr_api_client_stream_foreach` macro in `api/gr_api.h` always sets its `ret` parameter during the first outer loop condition evaluation (via `if (__first == 1) ret = __id;`), even when the stream yields zero items. Therefore, the `ret` variable does not need to be initialized at declaration when used with this macro.
Applied to files:
modules/srv6/cli/iface_nh.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/srv6/cli/iface_nh.c
🧬 Code graph analysis (6)
frr/rt_grout.c (1)
frr/if_map.c (1)
ifindex_frr_to_grout(90-94)
modules/srv6/datapath/srv6_local.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/control/iface.c (1)
iface_from_id(284-291)
modules/srv6/control/iface_nh.c (4)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/control/nexthop.c (1)
nexthop_lookup_by_id(491-498)modules/infra/control/iface.c (1)
iface_mode_register(58-64)main/event.c (1)
gr_event_subscribe(16-18)
smoke/srv6_l2_encap_test.sh (1)
smoke/_init.sh (2)
netns_add(107-115)move_to_netns(117-131)
modules/srv6/datapath/l2_encap.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(592-619)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(23-26)modules/infra/datapath/port_rx.c (1)
register_interface_mode(33-37)
modules/infra/control/gr_iface.h (1)
modules/infra/control/iface.c (4)
iface_type_register(39-45)iface_mode_register(58-64)iface_type_get(30-37)iface_mode_get(49-56)
🪛 Cppcheck (2.19.0)
modules/srv6/datapath/l2_encap.c
[error] 9-9: There is an unknown macro here somewhere. Configuration is required. If GR_MBUF_PRIV_DATA_TYPE is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.10)
smoke/srv6_l2_encap_scapy.py
5-5: from scapy.all import * used; unable to detect undefined names
(F403)
33-33: sendp may be undefined, or defined from star imports
(F405)
33-33: Ether may be undefined, or defined from star imports
(F405)
36-36: sniff may be undefined, or defined from star imports
(F405)
🪛 Shellcheck (0.11.0)
smoke/srv6_l2_encap_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ 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 (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: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (6)
modules/srv6/api/gr_srv6.h (1)
65-82: LGTM!The SR_BEHAVIOR_END_DX2 enum value and name mapping are correctly implemented according to IANA specifications.
frr/rt_grout.c (1)
269-271: LGTM!Correct mapping from SR_BEHAVIOR_END_DX2 to ZEBRA_SEG6_LOCAL_ACTION_END_DX2.
smoke/srv6_l2_encap_scapy.py (1)
1-36: LGTM!The Scapy script correctly implements SRv6 L2 encapsulation for testing. Protocol 143 (line 25) is the correct next header value for Ethernet encapsulation per RFC 8986.
Note: Static analysis warnings about undefined names from wildcard import are false positives - this is standard scapy usage.
smoke/srv6_l2_encap_test.sh (1)
1-64: LGTM!The test script properly sets up the SRv6 DX2 test environment with namespace isolation, configures routes and nexthops, and includes verification that the Scapy capture process starts successfully before running connectivity tests.
modules/infra/control/gr_iface.h (2)
21-21: LGTM!Addition of
mode_infofield to store per-interface mode-specific data is appropriate.
61-76: LGTM!The interface mode infrastructure follows the established pattern of the existing
iface_typeregistration system. The API is consistent and well-structured.
Comment on lines
+732
to
+736
| case ZEBRA_SEG6_LOCAL_ACTION_END_DX2: | ||
| sr6_local->behavior = SR_BEHAVIOR_END_DX2; | ||
| // FIXME | ||
| sr6_local->out_vrf_id = ifindex_frr_to_grout(GR_VRF_ID_ALL); | ||
| break; |
There was a problem hiding this comment.
Critical: Incorrect function usage for DX2 interface mapping.
The code passes GR_VRF_ID_ALL to ifindex_frr_to_grout(), which expects an FRR interface index (ifindex_t), not a VRF ID constant. This will fail the mapping lookup and return GR_IFACE_ID_UNDEF.
For END.DX2 behavior, you need to extract the output interface from the seg6local context (similar to how other behaviors use nh->nh_srv6->seg6local_ctx.table). The FIXME acknowledges this is incomplete, but the current implementation will not work correctly.
Expected pattern based on other behaviors
Other local behaviors extract the interface/table from the nexthop context:
case ZEBRA_SEG6_LOCAL_ACTION_END_DT6:
sr6_local->behavior = SR_BEHAVIOR_END_DT6;
sr6_local->out_vrf_id = ifindex_frr_to_grout(
nh->nh_srv6->seg6local_ctx.table
);
break;For DX2, you should extract the output interface from the appropriate field in seg6local_ctx rather than using GR_VRF_ID_ALL.
🤖 Prompt for AI Agents
In @frr/rt_grout.c around lines 732 - 736, Replace the incorrect GR_VRF_ID_ALL
usage: for ZEBRA_SEG6_LOCAL_ACTION_END_DX2 set sr6_local->out_vrf_id by mapping
the FRR interface index taken from the seg6local context on the nexthop (i.e.,
use ifindex_frr_to_grout(nh->nh_srv6->seg6local_ctx.ifindex) or the actual
seg6local_ctx field that holds the output interface), so the code mirrors the
pattern used by other behaviors (e.g., END_DT6 uses
nh->nh_srv6->seg6local_ctx.table) and does not pass a VRF constant to
ifindex_frr_to_grout.
Comment on lines
+284
to
+316
| static int process_behav_decap_eth( | ||
| struct rte_mbuf *m, | ||
| struct nexthop_info_srv6_local *sr_d, | ||
| struct ip6_info *ip6_info | ||
| ) { | ||
| struct rte_ipv6_routing_ext *sr = ip6_info->sr; | ||
| const struct iface *iface; | ||
| rte_edge_t edge; | ||
|
|
||
| // transit is not allowed | ||
| if (sr != NULL && sr->segments_left > 0) | ||
| return NO_TRANSIT; | ||
|
|
||
| switch (ip6_info->proto) { | ||
| case IPPROTO_ETHERNET: | ||
| if (sr_d->behavior != SR_BEHAVIOR_END_DX2) | ||
| return UNEXPECTED_UPPER; | ||
| iface = iface_from_id(sr_d->out_vrf_id); | ||
| if (iface == NULL) | ||
| return DEST_UNREACH; | ||
|
|
||
| if (iface->type == GR_IFACE_TYPE_PORT) | ||
| edge = PORT_OUTPUT; | ||
| else if (iface->type == GR_IFACE_TYPE_BOND) | ||
| edge = BOND_OUTPUT; | ||
| mbuf_data(m)->iface = iface; | ||
| m->hash.usr = rte_be_to_cpu_32(ip6_info->ip6_hdr->vtc_flow) & RTE_IPV6_HDR_FL_MASK; | ||
| rte_pktmbuf_adj(m, ip6_info->ext_offset); | ||
| return edge; | ||
| default: | ||
| return process_upper_layer(m, ip6_info); | ||
| } | ||
| } |
There was a problem hiding this comment.
Uninitialized variable leads to undefined behavior.
In process_behav_decap_eth, the variable edge (line 291) is used uninitialized if iface->type is neither GR_IFACE_TYPE_PORT nor GR_IFACE_TYPE_BOND (lines 305-308). Line 312 returns this uninitialized value, which causes undefined behavior.
🐛 Proposed fix
if (iface->type == GR_IFACE_TYPE_PORT)
edge = PORT_OUTPUT;
else if (iface->type == GR_IFACE_TYPE_BOND)
edge = BOND_OUTPUT;
+ else
+ return INVALID_PACKET;
mbuf_data(m)->iface = iface;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static int process_behav_decap_eth( | |
| struct rte_mbuf *m, | |
| struct nexthop_info_srv6_local *sr_d, | |
| struct ip6_info *ip6_info | |
| ) { | |
| struct rte_ipv6_routing_ext *sr = ip6_info->sr; | |
| const struct iface *iface; | |
| rte_edge_t edge; | |
| // transit is not allowed | |
| if (sr != NULL && sr->segments_left > 0) | |
| return NO_TRANSIT; | |
| switch (ip6_info->proto) { | |
| case IPPROTO_ETHERNET: | |
| if (sr_d->behavior != SR_BEHAVIOR_END_DX2) | |
| return UNEXPECTED_UPPER; | |
| iface = iface_from_id(sr_d->out_vrf_id); | |
| if (iface == NULL) | |
| return DEST_UNREACH; | |
| if (iface->type == GR_IFACE_TYPE_PORT) | |
| edge = PORT_OUTPUT; | |
| else if (iface->type == GR_IFACE_TYPE_BOND) | |
| edge = BOND_OUTPUT; | |
| mbuf_data(m)->iface = iface; | |
| m->hash.usr = rte_be_to_cpu_32(ip6_info->ip6_hdr->vtc_flow) & RTE_IPV6_HDR_FL_MASK; | |
| rte_pktmbuf_adj(m, ip6_info->ext_offset); | |
| return edge; | |
| default: | |
| return process_upper_layer(m, ip6_info); | |
| } | |
| } | |
| static int process_behav_decap_eth( | |
| struct rte_mbuf *m, | |
| struct nexthop_info_srv6_local *sr_d, | |
| struct ip6_info *ip6_info | |
| ) { | |
| struct rte_ipv6_routing_ext *sr = ip6_info->sr; | |
| const struct iface *iface; | |
| rte_edge_t edge; | |
| // transit is not allowed | |
| if (sr != NULL && sr->segments_left > 0) | |
| return NO_TRANSIT; | |
| switch (ip6_info->proto) { | |
| case IPPROTO_ETHERNET: | |
| if (sr_d->behavior != SR_BEHAVIOR_END_DX2) | |
| return UNEXPECTED_UPPER; | |
| iface = iface_from_id(sr_d->out_vrf_id); | |
| if (iface == NULL) | |
| return DEST_UNREACH; | |
| if (iface->type == GR_IFACE_TYPE_PORT) | |
| edge = PORT_OUTPUT; | |
| else if (iface->type == GR_IFACE_TYPE_BOND) | |
| edge = BOND_OUTPUT; | |
| else | |
| return INVALID_PACKET; | |
| mbuf_data(m)->iface = iface; | |
| m->hash.usr = rte_be_to_cpu_32(ip6_info->ip6_hdr->vtc_flow) & RTE_IPV6_HDR_FL_MASK; | |
| rte_pktmbuf_adj(m, ip6_info->ext_offset); | |
| return edge; | |
| default: | |
| return process_upper_layer(m, ip6_info); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @modules/srv6/datapath/srv6_local.c around lines 284 - 316,
process_behav_decap_eth has an uninitialized local 'edge' used when iface->type
is neither GR_IFACE_TYPE_PORT nor GR_IFACE_TYPE_BOND; ensure you handle the
unknown iface type explicitly (e.g., set edge to a safe sentinel or return an
error like DEST_UNREACH) instead of falling through, by adding an else branch
after the iface->type checks that either assigns edge a defined value or returns
an error; update references to edge accordingly in process_behav_decap_eth so
the function cannot return an uninitialized value.
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.
Summary by CodeRabbit
New Features
Tests
Chores
Bug Fixes / Maintenance
✏️ Tip: You can customize this high-level summary in your review settings.