iface: introduce domain relationship for interface attachment - #489
Conversation
📝 WalkthroughWalkthroughReplaces GR_IFACE_MODE_L3 and GR_IFACE_MODE_L1_XC with GR_IFACE_MODE_VRF, GR_IFACE_MODE_XC, and GR_IFACE_MODE_BOND. Interface domain membership moved from bond/member lists at creation to post-creation domain attach/detach; iface_type gains attach_domain/detach_domain hooks and port/bond types implement them. Bond handling refactored (attach/detach, MAC propagation, primary tracking). CLI and smoke tests updated to use domain binding instead of inline members. Datapath/control code switched to mode/domain_id-based logic; l1_xconnect renamed to xconnect. devtools/complexity.json metrics adjusted. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/datapath/l1_xconnect.c (1)
30-46: Potential NULL pointer dereference onpeer.
iface_from_id()can return NULL (per iface.c lines 330-337), but line 36 dereferencespeer->typewithout a NULL check. Ifdomain_idrefers to a removed or invalid interface, this will crash the datapath.Proposed fix
peer = iface_from_id(iface->domain_id); + if (peer == NULL) { + edge = NO_PORT; + goto enqueue; + } struct iface_stats *rx_stats = iface_get_stats(rte_lcore_id(), iface->id);And add a label before enqueue:
+enqueue: if (gr_mbuf_is_traced(mbuf)) {
🤖 Fix all issues with AI agents
In `@modules/infra/control/iface.c`:
- Around line 269-290: The code detaches the existing domain/VRF unconditionally
before trying type->attach_domain(domain, iface), so on attach failure the iface
remains detached and VRF refcounts can be corrupted; in iface_reconfig capture
the previous attachment state (previous domain pointer and previous vrf_id /
mode) before calling detach_domain(iface), then either (a) defer calling
detach_domain until after type->attach_domain returns success, or (b) if you
must detach first, on attach failure restore the previous attachment by
re-attaching the saved domain or restoring vrf_id/mode and re-incrementing VRF
refs (e.g., call type->attach_domain(previous_domain, iface) or the appropriate
VRF ref increment routine) so the interface and refcounts are returned to their
prior state; update error paths in iface_reconfig accordingly (referencing
iface_reconfig, detach_domain, attach_domain, iface_from_id, iface->vrf_id,
iface->mode).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/ipip/control.c (1)
69-76: Critical: Missing rollback on hash add failure.If
rte_hash_add_key_datafails on line 71, the old key has already been deleted on line 69, leaving the interface orphaned from the hash. Subsequent calls toipip_get_iface()will fail, andiface_ipip_fini()will attempt to delete with stalelocal/remotevalues.Proposed fix: re-add old key on failure
rte_hash_del_key(ipip_hash, &cur_key); - if ((ret = rte_hash_add_key_data(ipip_hash, &next_key, iface)) < 0) + if ((ret = rte_hash_add_key_data(ipip_hash, &next_key, iface)) < 0) { + // Rollback: restore the old key + rte_hash_add_key_data(ipip_hash, &cur_key, iface); return errno_log(-ret, "rte_hash_add_key_data"); + } cur->local = next->local; cur->remote = next->remote;
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/datapath/xconnect.c (1)
30-36: Null pointer dereference:peeris not checked before use.
iface_from_id()returns NULL when the interface ID is undefined or invalid. Line 36 dereferencespeer->typewithout a NULL check, which will crash if the peer interface was removed ordomain_idis invalid.🐛 Proposed fix
iface = mbuf_data(mbuf)->iface; peer = iface_from_id(iface->domain_id); + if (peer == NULL) { + edge = NO_PORT; + goto next; + } struct iface_stats *rx_stats = iface_get_stats(rte_lcore_id(), iface->id);
🤖 Fix all issues with AI agents
In `@modules/infra/control/bond.c`:
- Around line 198-236: The code in bond_detach_member can underflow
bond->primary_member when bond->n_members becomes 0; update the logic after
decrementing bond->n_members so you do not blindly decrement an unsigned
primary_member. Specifically, in function bond_detach_member, replace the
existing check that does "if (bond->primary_member >= bond->n_members)
bond->primary_member--" with a guarded sequence: if bond->n_members == 0 set
bond->primary_member to a safe sentinel (e.g. GR_IFACE_ID_UNDEF or 0 as used
elsewhere), otherwise if bond->primary_member >= bond->n_members decrement it;
this avoids unsigned underflow while keeping existing behavior used by bond_fini
and other callers.
🧹 Nitpick comments (1)
modules/infra/control/iface.c (1)
29-42: Hardcoded type whitelist requires maintenance.
iface_type_validuses a hardcoded switch on known types. When new interface types are added, this function must be updated or registration will abort. Consider deriving validity from the enum range or documenting this maintenance requirement prominently.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/infra/datapath/xconnect.c (1)
30-46: Potential NULL pointer dereference onpeer.
iface_from_id(iface->domain_id)can returnNULLif the domain interface was deleted ordomain_idis invalid. Line 36 dereferencespeer->typewithout a NULL check, which would cause a datapath crash.🐛 Proposed fix
for (uint16_t i = 0; i < nb_objs; i++) { mbuf = objs[i]; iface = mbuf_data(mbuf)->iface; peer = iface_from_id(iface->domain_id); struct iface_stats *rx_stats = iface_get_stats(rte_lcore_id(), iface->id); rx_stats->rx_packets++; rx_stats->rx_bytes += rte_pktmbuf_pkt_len(mbuf); - if (peer->type == GR_IFACE_TYPE_PORT) { + if (peer == NULL) { + edge = NO_PORT; + } else if (peer->type == GR_IFACE_TYPE_PORT) { port = iface_info_port(peer); mbuf->port = port->port_id; edge = OUTPUT;modules/ipip/control.c (1)
60-76: Avoid losing the existing hash entry on failed reconfig.Deleting the current key before successfully inserting the new key can orphan the iface in the hash if the add fails (e.g., ENOSPC), leaving lookups broken even though reconfig failed. Add the new key first (or rollback on failure), then delete the old key.
🐛 Proposed fix
- rte_hash_del_key(ipip_hash, &cur_key); - - if ((ret = rte_hash_add_key_data(ipip_hash, &next_key, iface)) < 0) - return errno_log(-ret, "rte_hash_add_key_data"); + if ((ret = rte_hash_add_key_data(ipip_hash, &next_key, iface)) < 0) + return errno_log(-ret, "rte_hash_add_key_data"); + + rte_hash_del_key(ipip_hash, &cur_key);
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/datapath/xconnect.c (1)
29-44: Guard against NULLpeerfromiface_from_id.If
iface->domain_idis unset/invalid,iface_from_idcan return NULL andpeer->typewill crash the datapath. Treat NULL as NO_PORT (or drop) before dereferencing.🐛 Proposed fix
- peer = iface_from_id(iface->domain_id); + peer = iface_from_id(iface->domain_id); - if (peer->type == GR_IFACE_TYPE_PORT) { + if (peer != NULL && peer->type == GR_IFACE_TYPE_PORT) { port = iface_info_port(peer); mbuf->port = port->port_id; edge = OUTPUT;
da94e64 to
f2aa442
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/datapath/xconnect.c (1)
30-46: NULL pointer dereference:peernot checked before use.
iface_from_id()can return NULL if the domain interface was deleted. Line 36 dereferencespeer->typewithout a NULL check, which will crash the datapath.🐛 Proposed fix
peer = iface_from_id(iface->domain_id); + if (peer == NULL) { + edge = NO_PORT; + rte_node_enqueue_x1(graph, node, edge, mbuf); + continue; + } struct iface_stats *rx_stats = iface_get_stats(rte_lcore_id(), iface->id);
🤖 Fix all issues with AI agents
In `@modules/infra/control/bond.c`:
- Around line 170-173: The code currently calls iface_get_eth_addr(member,
&bond->mac) when rte_is_zero_ether_addr(&bond->mac) but ignores its return
value; change this to check the return code and abort on failure (use
errno_log(...) and return) so we don't continue with a zero/invalid bond MAC.
Specifically, update the branch that uses rte_is_zero_ether_addr(&bond->mac) to
capture iface_get_eth_addr's return, call errno_log with a helpful message and
return on error, and keep the existing iface_add_eth_addr(member, &bond->mac)
error handling for the non-zero-MAC path.
In `@modules/ipip/control.c`:
- Around line 69-72: The code deletes the old hash entry
(rte_hash_del_key(ipip_hash, &cur_key)) before ensuring the new entry was
successfully added, which can orphan iface if rte_hash_add_key_data(ipip_hash,
&next_key, iface) fails; change the flow in the routine that updates entries so
you attempt rte_hash_add_key_data first and only call rte_hash_del_key on
success (or, alternatively, add the new key with a temporary pointer/check and
only remove cur_key after confirming addition), using the existing symbols
rte_hash_add_key_data, rte_hash_del_key, ipip_hash, cur_key, next_key and
ensuring ipip_get_iface can still find iface on failure so callers can recover
or retry.
| if (rte_is_zero_ether_addr(&bond->mac)) | ||
| iface_get_eth_addr(member, &bond->mac); | ||
| else if (iface_add_eth_addr(member, &bond->mac) < 0) | ||
| return errno_log(errno, "iface_add_eth_addr(member)"); |
There was a problem hiding this comment.
Check iface_get_eth_addr errors when adopting the first member’s MAC.
If iface_get_eth_addr() fails (e.g., EOPNOTSUPP), attachment continues with an invalid/zero bond MAC, which breaks later MAC propagation. Bail out on failure.
🔧 Proposed fix
- if (rte_is_zero_ether_addr(&bond->mac))
- iface_get_eth_addr(member, &bond->mac);
+ if (rte_is_zero_ether_addr(&bond->mac)) {
+ if (iface_get_eth_addr(member, &bond->mac) < 0)
+ return errno_log(errno, "iface_get_eth_addr(member)");
+ }
else if (iface_add_eth_addr(member, &bond->mac) < 0)
return errno_log(errno, "iface_add_eth_addr(member)");📝 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.
| if (rte_is_zero_ether_addr(&bond->mac)) | |
| iface_get_eth_addr(member, &bond->mac); | |
| else if (iface_add_eth_addr(member, &bond->mac) < 0) | |
| return errno_log(errno, "iface_add_eth_addr(member)"); | |
| if (rte_is_zero_ether_addr(&bond->mac)) { | |
| if (iface_get_eth_addr(member, &bond->mac) < 0) | |
| return errno_log(errno, "iface_get_eth_addr(member)"); | |
| } | |
| else if (iface_add_eth_addr(member, &bond->mac) < 0) | |
| return errno_log(errno, "iface_add_eth_addr(member)"); |
🤖 Prompt for AI Agents
In `@modules/infra/control/bond.c` around lines 170 - 173, The code currently
calls iface_get_eth_addr(member, &bond->mac) when
rte_is_zero_ether_addr(&bond->mac) but ignores its return value; change this to
check the return code and abort on failure (use errno_log(...) and return) so we
don't continue with a zero/invalid bond MAC. Specifically, update the branch
that uses rte_is_zero_ether_addr(&bond->mac) to capture iface_get_eth_addr's
return, call errno_log with a helpful message and return on error, and keep the
existing iface_add_eth_addr(member, &bond->mac) error handling for the
non-zero-MAC path.
| rte_hash_del_key(ipip_hash, &cur_key); | ||
|
|
||
| if ((ret = rte_hash_add_key_data(ipip_hash, &next_key, iface)) < 0) | ||
| return errno_log(-ret, "rte_hash_add_key_data"); |
There was a problem hiding this comment.
Hash entry deleted before confirming new entry can be added — interface becomes orphaned on failure.
If rte_hash_add_key_data fails at line 71, the old key was already deleted at line 69. The interface is now unreachable via ipip_get_iface() and cannot be recovered without recreating it.
🐛 Proposed fix: defer deletion until add succeeds
if (memcmp(&cur_key, &next_key, sizeof(cur_key)) != 0) {
if (rte_hash_lookup(ipip_hash, &next_key) >= 0)
return errno_set(EADDRINUSE);
if (fib4_lookup(conf->vrf_id, next->local) == NULL)
return -errno;
if (fib4_lookup(conf->vrf_id, next->remote) == NULL)
return -errno;
- rte_hash_del_key(ipip_hash, &cur_key);
-
if ((ret = rte_hash_add_key_data(ipip_hash, &next_key, iface)) < 0)
return errno_log(-ret, "rte_hash_add_key_data");
+ rte_hash_del_key(ipip_hash, &cur_key);
+
cur->local = next->local;
cur->remote = next->remote;
}🤖 Prompt for AI Agents
In `@modules/ipip/control.c` around lines 69 - 72, The code deletes the old hash
entry (rte_hash_del_key(ipip_hash, &cur_key)) before ensuring the new entry was
successfully added, which can orphan iface if rte_hash_add_key_data(ipip_hash,
&next_key, iface) fails; change the flow in the routine that updates entries so
you attempt rte_hash_add_key_data first and only call rte_hash_del_key on
success (or, alternatively, add the new key with a temporary pointer/check and
only remove cur_key after confirming addition), using the existing symbols
rte_hash_add_key_data, rte_hash_del_key, ipip_hash, cur_key, next_key and
ensuring ipip_get_iface can still find iface on failure so callers can recover
or retry.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/datapath/xconnect.c (1)
30-46: NULL pointer dereference:peeris used without a NULL check.
iface_from_id()returnsNULLifdomain_idis invalid. Dereferencingpeerat line 36 will crash if the peer interface was deleted or never existed.🐛 Proposed fix
for (uint16_t i = 0; i < nb_objs; i++) { mbuf = objs[i]; iface = mbuf_data(mbuf)->iface; peer = iface_from_id(iface->domain_id); struct iface_stats *rx_stats = iface_get_stats(rte_lcore_id(), iface->id); rx_stats->rx_packets++; rx_stats->rx_bytes += rte_pktmbuf_pkt_len(mbuf); + if (peer == NULL) { + edge = NO_PORT; + } else if (peer->type == GR_IFACE_TYPE_PORT) { - if (peer->type == GR_IFACE_TYPE_PORT) { port = iface_info_port(peer); mbuf->port = port->port_id; edge = OUTPUT; struct iface_stats *tx_stats = iface_get_stats(rte_lcore_id(), peer->id); tx_stats->tx_packets++; tx_stats->tx_bytes += rte_pktmbuf_pkt_len(mbuf); } else { edge = NO_PORT; }
Replace the simple range check against GR_IFACE_TYPE_COUNT with an explicit allowlist of valid interface types. This ensures that only intentionally defined types can be registered, catching programming errors where an invalid or sentinel value like GR_IFACE_TYPE_UNDEF might be used. Use errno_set_null() in iface_type_get() for consistency with other lookup functions. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
Rename GR_IFACE_MODE_L3 to GR_IFACE_MODE_VRF since an interface in this mode is associated with a VRF rather than a bridge domain. Rename GR_IFACE_MODE_L1_XC to GR_IFACE_MODE_XC since the L1 prefix is unnecessary; cross-connect operates at the packet level regardless of L2/L3 semantics. Also rename the datapath node and its source file from l1_xconnect to xconnect. Change the mode display strings to uppercase for consistency with how they appear in CLI output. Use gr_iface_mode_name() in iface_list() instead of duplicating the switch statement, and add mode display to iface_show() for completeness. Delete control/xconnect.c which was committed by mistake and never used. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
The old union of vrf_id and domain_id was confusing and made it unclear which field applied to which mode. Split these into distinct fields: vrf_id for interfaces in VRF mode and domain_id for interfaces attached to another interface. Introduce attach_domain and detach_domain callbacks in struct iface_type so that domain interface types can manage their members. When an interface is created or reconfigured with a domain, the domain's attach callback is invoked. The mode is now set by the domain type rather than explicitly by the caller. Add GR_IFACE_SET_DOMAIN as a distinct attribute from GR_IFACE_SET_VRF. Update the CLI to accept "domain DOMAIN" as an alternative to "vrf VRF" for interface configuration. Port xconnect configuration now uses this mechanism: when a port is cross-connected to a peer, the peer becomes the domain and sets the mode to XC. This removes the port-specific mode handling code and makes the pattern reusable for bridge and bond interfaces. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
Instead of specifying members during bond creation and tracking them via a bond_iface_id field in port info, use the generic domain mechanism introduced earlier. Ports are now attached to bonds using the domain attribute, and bond membership is tracked through the interface mode. Add GR_IFACE_MODE_BOND to indicate that an interface is part of a bond. Implement attach_domain and detach_domain callbacks for the bond interface type to handle member addition and removal. Remove the bond_iface_id field from struct gr_iface_info_port and the GR_BOND_SET_MEMBERS attribute since members are now managed through the domain interface mechanism. The CLI syntax changes accordingly: bonds are created without member specification, and ports specify their domain when created or reconfigured. The event handler for bond member status updates now checks interface mode instead of bond_iface_id. Member detachment on interface removal is handled automatically by the detach_domain callback. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
63ae8f4 to
4ee6186
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
smoke/config_test.sh (1)
7-41: Bond members used as L3 interfaces after domain attachAfter attaching
p0andp1tobond0’s domain, the script still creates nexthops, addresses, and routes onp0/p1. Bond members should no longer behave as standalone L3 interfaces, so these commands are likely to fail or validate the wrong behavior. Consider moving the L3 objects tobond0orv42/v43, or avoid attaching the ports to the bond domain if per-port L3 is intended.
3 participants
Bridge interface support will require attaching ports to a bridge, similar to how bonds work today. The current bond-specific approach with a bond_iface_id field in port info does not generalize well.
This series introduces a generic domain mechanism for interface attachment. The attach_domain and detach_domain callbacks in struct iface_type allow domain interface types to manage their members and set the appropriate mode.
The interface modes are renamed from L3/L1_XC to VRF/XC to better reflect their meaning. Bond interfaces are migrated to use the new mechanism as a proof of concept: ports are now attached to bonds using the generic domain attribute. Bridge interfaces will follow the same pattern.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / CLI
Tests
✏️ Tip: You can customize this high-level summary in your review settings.