infra: send GARP and NA on MAC address change - #473
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new interface event Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/infra/control/iface.c (1)
296-313: Potential functional gap: MAC change event won’t reach subinterfaces (VLAN, etc.)
iface_set_eth_addr()now emitsGR_EVENT_IFACE_MAC_CHANGEon success, but the infraiface_eventhandler doesn’t subscribe to (or forward) this event toiface->subinterfaces. If a parent’s MAC change affects neighbors’ reachability for IPs hosted on subinterfaces, they may not send GARP/NA, defeating the PR’s goal for those IPs.Proposed fix (subscribe + forward MAC_CHANGE to subinterfaces)
static void iface_event(uint32_t event, const void *obj) { const struct iface *iface = obj; char *str = ""; switch (event) { + case GR_EVENT_IFACE_MAC_CHANGE: + str = "MAC_CHANGE"; + gr_vec_foreach (struct iface *s, iface->subinterfaces) + gr_event_push(event, s); + break; case GR_EVENT_IFACE_ADD: str = "ADD"; break; ... } LOG(DEBUG, "iface event [0x%08x] %s triggered for iface %s.", event, str, iface->name); } static struct gr_event_subscription iface_event_handler = { .callback = iface_event, - .ev_count = 7, + .ev_count = 8, .ev_types = { GR_EVENT_IFACE_ADD, GR_EVENT_IFACE_POST_ADD, GR_EVENT_IFACE_PRE_REMOVE, GR_EVENT_IFACE_REMOVE, GR_EVENT_IFACE_POST_RECONFIG, GR_EVENT_IFACE_STATUS_UP, GR_EVENT_IFACE_STATUS_DOWN, + GR_EVENT_IFACE_MAC_CHANGE, }, };modules/infra/control/vlan.c (1)
34-98: Critical: VLAN MAC reconfig can end up deleting a bond parent’s primary MAC from member portsDuring VLAN reconfig, when
GR_VLAN_SET_MACis set you doiface_del_eth_addr(cur_parent, &cur->mac)(Line 49-53). With the newiface_vlan_set_eth_addr()“inherit from parent” path,cur->maccan be the parent’s primary MAC. If the parent is a bond,bond_mac_del()will remove that MAC from all member ports, potentially breaking the bond.Minimal mitigation: don’t delete the parent’s primary MAC
if (reconfig) { if ((cur_parent = iface_from_id(cur->parent_id)) == NULL) return -errno; if (set_attrs & GR_VLAN_SET_MAC) { // reconfig, *not initial config* - // remove previous mac filter (ignore errors) - iface_del_eth_addr(cur_parent, &cur->mac); + // remove previous mac filter (ignore errors), but never delete parent's primary MAC + struct rte_ether_addr parent_mac; + if (iface_get_eth_addr(cur_parent, &parent_mac) == 0 + && rte_is_same_ether_addr(&parent_mac, &cur->mac)) { + /* inherited/primary MAC: nothing to delete */ + } else { + iface_del_eth_addr(cur_parent, &cur->mac); + } } } else { cur_parent = NULL; }This still leaves an edge case if the VLAN was previously “inherited” but cached an old parent MAC (e.g., parent MAC changed); the robust fix is to track whether the VLAN MAC was explicitly added to the parent (e.g., a boolean in the VLAN private info) and only delete in that case.
Also applies to: 153-168
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
modules/infra/api/gr_infra.hmodules/infra/control/bond.cmodules/infra/control/iface.cmodules/infra/control/port.cmodules/infra/control/vlan.cmodules/ip/control/address.cmodules/ip6/control/address.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- 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/infra/control/iface.cmodules/infra/control/bond.cmodules/infra/control/vlan.cmodules/ip6/control/address.cmodules/ip/control/address.cmodules/infra/control/port.c
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/infra/control/iface.cmodules/ip6/control/address.cmodules/ip/control/address.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/ip6/control/address.c
🧬 Code graph analysis (3)
modules/infra/control/iface.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-25)modules/infra/control/port_test.c (1)
gr_event_push(24-24)modules/infra/control/worker_test.c (1)
gr_event_push(37-37)
modules/infra/control/vlan.c (1)
modules/infra/control/iface.c (2)
iface_set_eth_addr(296-313)iface_from_id(256-263)
modules/infra/control/port.c (2)
modules/infra/control/iface.c (1)
iface_set_eth_addr(296-313)modules/infra/control/gr_iface.h (1)
iface(16-25)
⏰ 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-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-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: rpm
- GitHub Check: deb
🔇 Additional comments (5)
modules/ip6/control/address.c (1)
421-428: No critical issues spotted in the addedGR_EVENT_IFACE_MAC_CHANGEhandling/subscription.Also applies to: 473-481
modules/infra/control/port.c (1)
333-334: No critical issues spotted in the switch toiface_set_eth_addr()for port MAC reconfig.modules/infra/control/bond.c (1)
356-367: No critical issues spotted in the switch toiface_set_eth_addr()for bond MAC reconfig.modules/ip/control/address.c (1)
271-275: No critical issues spotted in the updated IPv4 iface event subscription/serializer counts.Also applies to: 276-280
modules/infra/api/gr_infra.h (1)
205-216: No critical issues spotted in addingGR_EVENT_IFACE_MAC_CHANGE.
cc677f9 to
cceea80
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @modules/infra/control/vlan.c:
- Around line 97-100: The VLAN MAC handling currently copies the parent MAC and
loses the “inherit” semantics, and lacks NULL checks; add a boolean inherit_mac
(or similar) to struct iface_info_vlan, set it instead of copying when the
requested MAC is 00:00:00:00:00:00 in iface_vlan_set_eth_addr(), validate that
parent and mac pointers are non-NULL before using them, update
iface_vlan_get_eth_addr() to return the parent’s current MAC dynamically when
inherit_mac==true (only return stored vlan->mac when inherit_mac==false), and
ensure iface_vlan_fini() and the reconfig delete path only call
iface_del_eth_addr(parent, &vlan->mac) when inherit_mac==false so you never try
to delete an inherited/parent primary address.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
modules/infra/api/gr_infra.hmodules/infra/api/iface.cmodules/infra/cli/iface.cmodules/infra/control/bond.cmodules/infra/control/iface.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/vlan.cmodules/infra/control/worker_test.cmodules/ip/control/address.cmodules/ip6/control/address.c
🚧 Files skipped from review as they are similar to previous changes (4)
- modules/infra/control/port_test.c
- modules/infra/api/gr_infra.h
- modules/infra/control/worker_test.c
- modules/infra/control/port.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/ip6/control/address.cmodules/infra/control/iface.cmodules/infra/cli/iface.cmodules/infra/api/iface.cmodules/ip/control/address.cmodules/infra/control/vlan.cmodules/infra/control/bond.c
🧠 Learnings (3)
📓 Common learnings
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.
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
📚 Learning: 2025-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/ip6/control/address.cmodules/infra/control/iface.cmodules/infra/cli/iface.cmodules/infra/api/iface.cmodules/ip/control/address.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/ip6/control/address.c
🧬 Code graph analysis (3)
modules/infra/control/iface.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-25)
modules/infra/control/vlan.c (3)
modules/infra/control/iface.c (5)
iface_get_eth_addr(265-277)iface_del_eth_addr(329-341)iface_set_eth_addr(296-313)iface_from_id(256-263)iface_add_eth_addr(315-327)modules/infra/control/port_test.c (1)
iface_set_eth_addr(31-33)modules/infra/control/worker_test.c (2)
iface_set_eth_addr(60-62)iface_from_id(45-47)
modules/infra/control/bond.c (3)
modules/infra/control/iface.c (1)
iface_set_eth_addr(296-313)modules/infra/control/port_test.c (1)
iface_set_eth_addr(31-33)modules/infra/control/gr_iface.h (1)
iface(16-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: rpm
- GitHub Check: deb
aharivel
left a comment
aharivel
left a comment
There was a problem hiding this comment.
LGTM - couple of very minor questions / changes...
cceea80 to
4f4be53
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/ip6/control/address.c (1)
539-549:⚠️ Potential issue | 🔴 Critical
ev_countnot updated —GR_EVENT_IFACE_MAC_CHANGEsubscription is silently dropped.
ev_count = 4was not incremented whenGR_EVENT_IFACE_MAC_CHANGEwas added as the 5th entry inev_types. The subscription infrastructure iterates only entries[0..ev_count-1], so this event is never registered andip6_iface_event_handlerwill never fire for it. IPv6 NA won't be sent on MAC change.Fix
static struct gr_event_subscription iface_event_subscription = { .callback = ip6_iface_event_handler, - .ev_count = 4, + .ev_count = 5, .ev_types = { GR_EVENT_IFACE_POST_ADD, GR_EVENT_IFACE_POST_RECONFIG, GR_EVENT_IFACE_PRE_REMOVE, GR_EVENT_IFACE_STATUS_UP, GR_EVENT_IFACE_MAC_CHANGE, }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/ip6/control/address.c` around lines 539 - 549, The subscription struct iface_event_subscription lists five event types but its ev_count is still 4, so GR_EVENT_IFACE_MAC_CHANGE is never registered; update ev_count to match the number of entries (e.g., change ev_count = 4 to ev_count = 5) or replace the hardcoded value with a size expression (compute from the ev_types array) so ip6_iface_event_handler will be subscribed for GR_EVENT_IFACE_MAC_CHANGE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@modules/ip6/control/address.c`:
- Around line 539-549: The subscription struct iface_event_subscription lists
five event types but its ev_count is still 4, so GR_EVENT_IFACE_MAC_CHANGE is
never registered; update ev_count to match the number of entries (e.g., change
ev_count = 4 to ev_count = 5) or replace the hardcoded value with a size
expression (compute from the ev_types array) so ip6_iface_event_handler will be
subscribed for GR_EVENT_IFACE_MAC_CHANGE.
---
Duplicate comments:
In `@modules/infra/control/vlan.c`:
- Around line 161-176: Validate inputs and stop caching a stale parent MAC: in
iface_vlan_set_eth_addr check that mac is not NULL before calling
rte_is_zero_ether_addr and check that parent (from iface_from_id) is not NULL
before calling iface_get_eth_addr/iface_add_eth_addr; when mac is zero (inherit
case) do not copy the parent's address into vlan->mac (which becomes stale if
parent changes) — instead either leave vlan->mac zeroed or set an explicit
inherit flag on the iface_info_vlan struct so callers use the parent's MAC
dynamically (reference iface_vlan_set_eth_addr, iface_info_vlan, iface_from_id,
iface_get_eth_addr, iface_add_eth_addr, and rte_is_zero_ether_addr when making
changes).
When an interface MAC address changes, neighbors must be notified so they can update their caches without waiting for stale entries to expire. Introduce GR_EVENT_IFACE_MAC_CHANGE which is pushed from iface_set_eth_addr() on success. Have bond and port reconfig handlers call the generic iface_set_eth_addr() wrapper instead of their type-specific callbacks. Implement set_eth_addr for VLAN interfaces. Subscribe IPv4 and IPv6 address handlers to this event to trigger gratuitous ARP and unsolicited neighbor advertisements respectively. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
4f4be53 to
d51865d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/control/vlan.c (1)
107-121:⚠️ Potential issue | 🟠 Major
iface_vlan_finireturns a spurious error when destroying a VLAN with an inherited MAC.
iface_vlan_reconfig(lines 50–56) correctly skipsiface_del_eth_addrwhen the current VLAN MAC equals the parent's primary MAC.iface_vlan_finihas no such guard: it unconditionally callsiface_del_eth_addr(parent, &vlan->mac), which will fail whenvlan->macis the parent's primary/default MAC (set via the inherit path iniface_vlan_set_eth_addr). That failure propagates throughiface_destroyback to the API caller as a hard error, even though the hash entry was already removed and the subinterface detached — i.e., the VLAN was cleaned up but the response says it failed.Apply the same guard used in the reconfig path:
🐛 Proposed fix
static int iface_vlan_fini(struct iface *iface) { struct iface_info_vlan *vlan = iface_info_vlan(iface); struct iface *parent = iface_from_id(vlan->parent_id); int ret, status = 0; rte_hash_del_key(vlan_hash, &(struct vlan_key) {vlan->parent_id, vlan->vlan_id}); - if ((ret = iface_del_eth_addr(parent, &vlan->mac)) < 0) - status = status ?: ret; + struct rte_ether_addr parent_mac; + if (parent == NULL + || iface_get_eth_addr(parent, &parent_mac) < 0 + || !rte_is_same_ether_addr(&parent_mac, &vlan->mac)) { + if ((ret = iface_del_eth_addr(parent, &vlan->mac)) < 0) + status = status ?: ret; + } if ((ret = iface_del_subinterface(parent, iface)) < 0) status = status ?: ret; return status; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/infra/control/vlan.c` around lines 107 - 121, The cleanup function iface_vlan_fini currently always calls iface_del_eth_addr(parent, &vlan->mac) which yields a spurious error when the VLAN inherited the parent's primary MAC; update iface_vlan_fini to use the same guard as iface_vlan_reconfig and skip calling iface_del_eth_addr if vlan->mac equals the parent's primary/default MAC (i.e., compare vlan->mac with the parent's primary MAC value used by iface_vlan_set_eth_addr), then proceed to delete the hash entry (rte_hash_del_key) and call iface_del_subinterface as before so a successful cleanup isn't reported as an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@modules/infra/control/vlan.c`:
- Around line 107-121: The cleanup function iface_vlan_fini currently always
calls iface_del_eth_addr(parent, &vlan->mac) which yields a spurious error when
the VLAN inherited the parent's primary MAC; update iface_vlan_fini to use the
same guard as iface_vlan_reconfig and skip calling iface_del_eth_addr if
vlan->mac equals the parent's primary/default MAC (i.e., compare vlan->mac with
the parent's primary MAC value used by iface_vlan_set_eth_addr), then proceed to
delete the hash entry (rte_hash_del_key) and call iface_del_subinterface as
before so a successful cleanup isn't reported as an error.
---
Duplicate comments:
In `@modules/infra/control/vlan.c`:
- Around line 161-176: iface_vlan_set_eth_addr currently snapshots the parent's
MAC into vlan->mac when given a zero (inherit) address, so later parent MAC
changes aren't reflected; fix this by recording that the VLAN is in "inherit"
mode (add a flag like mac_inherit to struct iface_info_vlan and set it in
iface_vlan_set_eth_addr when mac is zero) and then, in the iface event path that
handles GR_EVENT_IFACE_MAC_CHANGE (where iface_event updates subinterfaces),
detect mac_inherit and refresh vlan->mac from the parent (call
iface_get_eth_addr(parent, &vlan->mac) or equivalent) so iface_vlan_get_eth_addr
returns the up-to-date value; ensure iface_vlan_get_eth_addr respects the
mac_inherit flag (or always consult parent when mac_inherit is set).
2 participants
Add a GR_EVENT_IFACE_MAC_CHANGE event that is pushed when an interface MAC address is successfully changed via iface_set_eth_addr().
Refactor bond and port reconfig handlers to use iface_set_eth_addr() instead of calling the type-specific callback directly. Implement set_eth_addr for VLAN interfaces.
Subscribe IPv4 and IPv6 address handlers to the new event so that gratuitous ARP and neighbor advertisements are sent when the MAC changes. This allows neighbors to update their caches without waiting for stale entries to expire.
Summary by CodeRabbit