addr events #324
addr events #324
Conversation
📝 WalkthroughWalkthroughReworks interface-address event handling and dataplane notifications to use per-family, per-address payloads. The public API grout_interface_addr_dplane(struct gr_nexthop *) was removed and replaced with a static grout_interface_addr_change helper plus two public wrappers grout_interface_addr4_change and grout_interface_addr6_change. Call sites in zebra_dplane_grout.c now send structured gr_ip4_ifaddr / gr_ip6_ifaddr payloads and invoke the new per-family handlers directly. CLI printers and event serializers in modules/ip and modules/ip6 switched payload types from gr_nexthop to gr_ip4_ifaddr / gr_ip6_ifaddr. IPv6 control logic was reorganized: solicited‑node joins moved into add path, iface6_addr_del helper introduced, multicast removal reordered, and serializer sizes updated to reflect the new payload types. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
Fix leak in mcast6_addr_del. Make sure to always remove the address from the array. Signed-off-by: Robin Jarry <[email protected]>
rjarry
force-pushed
the
addr-events
branch
2 times, most recently
from
e5af55e to
4a1696f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modules/ip/control/address.c (1)
83-105: Fix inconsistent nexthop origin sent to the RIB.
base.originis nowGR_NH_ORIGIN_INTERNAL, yet we still insert the entry into the IPv4 RIB withGR_NH_ORIGIN_LINKon Line 100. That mismatch means the nexthop handed to FRR is tagged as “internal” while the route we store locally is still marked “link”, which breaks the origin-based filtering we rely on during sync. Please passGR_NH_ORIGIN_INTERNALtorib4_insertso the RIB’s bookkeeping stays coherent with the nexthop we advertise. Patch:- if ((ret = rib4_insert(iface->vrf_id, nh->ipv4, nh->prefixlen, - GR_NH_ORIGIN_LINK, nh)) < 0) + if ((ret = rib4_insert(iface->vrf_id, nh->ipv4, nh->prefixlen, + GR_NH_ORIGIN_INTERNAL, nh)) < 0)frr/if_grout.c (1)
147-179: Validate address family before enqueuing.With the new signature we can now be called with any
af. If someone slips in AF_UNSPEC (or worse), we memcpy garbage intop.uand still enqueue, leaking junk to zebra. Add a default branch that logs and aborts early whenafis unsupported.modules/ip6/cli/address.c (1)
84-84: Fix IP6_F argument: pass the IPv6 address fieldIP6_F expects a pointer to the IPv6 address, not the wrapper struct. Passing &addr->addr is incorrect; use &addr->addr.ip.
Apply this diff:
- scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr, addr->addr.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr.ip, addr->addr.prefixlen);
🧹 Nitpick comments (3)
frr/zebra_dplane_grout.c (2)
184-211: Don’t be silent on unsupported address families.These sync helpers invoke
grout_interface_addr_dplaneassuming AF_INET/AF_INET6. If we ever add another family upstream, we’ll walk right past the switch in grout_interface_addr_dplane and enqueue garbage. Guard the call with anelsethat logs and skips to keep zebra clean.
464-493: Reject unknown AF values before forwarding.Same story here: a rogue event with an unexpected AF will feed rubbish into the dplane context. Bail out loudly instead of calling
grout_interface_addr_dplaneblindly.frr/if_grout.h (1)
15-21: Avoid C++ keyword in public API and prefer sa_family_t for AF
- new is a C++ keyword; avoid in public headers to prevent accidental C++ build issues. Suggest is_add.
- int af is less precise; sa_family_t is the conventional type.
Apply this diff to the signature:
-void grout_interface_addr_dplane( - uint16_t iface_id, - int af, - const void *addr, - uint8_t prefixlen, - bool new -); +void grout_interface_addr_dplane( + uint16_t iface_id, + sa_family_t af, + const void *addr, + uint8_t prefixlen, + bool is_add +);Add the necessary include at the top of this header:
#include <sys/socket.h> // for sa_family_t
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frr/if_grout.c(1 hunks)frr/if_grout.h(1 hunks)frr/zebra_dplane_grout.c(4 hunks)modules/ip/cli/address.c(2 hunks)modules/ip/control/address.c(4 hunks)modules/ip6/cli/address.c(2 hunks)modules/ip6/control/address.c(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
PR: DPDK/grout#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/ip/control/address.cmodules/ip6/control/address.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
PR: DPDK/grout#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/ip/control/address.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
PR: DPDK/grout#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/ip/control/address.c
🧬 Code graph analysis (4)
frr/if_grout.h (1)
frr/if_grout.c (1)
grout_interface_addr_dplane(147-179)
modules/ip/control/address.c (2)
modules/infra/control/worker_test.c (1)
gr_event_push(36-36)main/event.c (1)
gr_event_push(18-29)
modules/ip6/control/address.c (6)
modules/infra/control/iface.c (2)
iface_get_eth_addr(224-237)iface_from_id(215-222)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/nexthop.c (2)
nexthop_new(329-375)nexthop_decref(520-555)modules/ip6/control/route.c (2)
rib6_insert(215-224)rib6_cleanup(467-469)main/event.c (1)
gr_event_push(18-29)main/gr_vec.h (1)
gr_vec_len(47-51)
frr/zebra_dplane_grout.c (1)
frr/if_grout.c (1)
grout_interface_addr_dplane(147-179)
⏰ 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-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 (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: deb
- GitHub Check: rpm
🔇 Additional comments (2)
modules/ip/cli/address.c (1)
131-131: Event payload switch to gr_ip4_ifaddr looks correctCasting obj to gr_ip4_ifaddr and printing via IP4_F with ifa->addr.ip/prefixlen is consistent with the new event payloads. LGTM.
Also applies to: 145-149
modules/ip6/cli/address.c (1)
136-136: IPv6 event printer payload/type update is correctCasting to gr_ip6_ifaddr and using IP6_F with ifa->addr.ip/prefixlen matches the new per-address payload struct. LGTM.
Also applies to: 150-154
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 (3)
modules/ip6/cli/address.c (1)
84-85: Print the IPv6 address field, not the whole net struct.Pass the IP member to IP6_F.
Apply:
- scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr, addr->addr.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr.ip, addr->addr.prefixlen);modules/ip6/control/address.c (2)
121-123: Fix errno propagation on nexthop_new failure.Passing
-errnoto errno_set flips the sign and returns success. Use positive errno.Apply:
- if ((nh = nexthop_new(&base)) == NULL) - return errno_set(-errno); + if ((nh = nexthop_new(&base)) == NULL) + return errno_set(errno);
147-156: Don’t remove vector entry on non‑last multicast leave.Unconditionally deleting
maddrs->nh[i]breaks reference‑counted membership. Only remove when this is the last reference.Apply:
- // shift remaining addresses - gr_vec_del(maddrs->nh, i); - - if (nh->ref_count == 1) { + bool last = (nh->ref_count == 1); + if (last) { + // shift remaining addresses + gr_vec_del(maddrs->nh, i); + } + + if (last) { LOG(INFO, "%s: leaving multicast group " IP6_F, iface->name, ip); // remove ethernet filter ret = iface_del_eth_addr(iface->id, &nh->mac); } nexthop_decref(nh);Also applies to: 158-158
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frr/if_grout.c(2 hunks)frr/if_grout.h(2 hunks)frr/zebra_dplane_grout.c(4 hunks)modules/ip/cli/address.c(2 hunks)modules/ip/control/address.c(4 hunks)modules/ip6/cli/address.c(2 hunks)modules/ip6/control/address.c(7 hunks)modules/srv6/control/route.c(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
PR: DPDK/grout#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/ip/control/address.cmodules/ip6/control/address.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
PR: DPDK/grout#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/ip/control/address.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
PR: DPDK/grout#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/ip/control/address.cmodules/srv6/control/route.c
🧬 Code graph analysis (4)
modules/ip/control/address.c (2)
modules/infra/control/worker_test.c (1)
gr_event_push(36-36)main/event.c (1)
gr_event_push(18-29)
frr/zebra_dplane_grout.c (1)
frr/if_grout.c (2)
grout_interface_addr4_change(181-185)grout_interface_addr6_change(187-191)
modules/ip6/control/address.c (6)
modules/infra/control/iface.c (2)
iface_get_eth_addr(224-237)iface_from_id(215-222)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/nexthop.c (2)
nexthop_new(329-375)nexthop_decref(520-555)modules/ip6/control/route.c (2)
rib6_insert(215-224)rib6_cleanup(467-469)main/event.c (1)
gr_event_push(18-29)main/gr_vec.h (1)
gr_vec_len(47-51)
frr/if_grout.h (1)
frr/if_grout.c (2)
grout_interface_addr4_change(181-185)grout_interface_addr6_change(187-191)
⏰ 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 (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-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (17)
modules/srv6/control/route.c (1)
271-281: Origin flip keeps the tunsrc nexthop aligned. UsingGR_NH_ORIGIN_INTERNALmatches the new address-event plumbing and prevents the synthetic tunsrc entry from surfacing as a link-learned address. Looks good.modules/ip/control/address.c (3)
92-92: Origin change to INTERNAL looks correct.Prevents spurious NEXTHOP_NEW/DELETE events for local ifaddrs.
220-221: Serializer size update is consistent with payload type.Switch to sizeof(gr_ip4_ifaddr) matches the new event payload.
104-105: Do not push transient request memory to events.
&req->addrbecomes invalid once the handler returns; subscribers/serializers can read freed memory. Allocate a stable gr_ip4_ifaddr or restructure to pass a lifetime‑safe object beforegr_event_push.Apply this minimal improvement (still stack‑bound but avoids dependency on the API buffer); for a robust fix, ensure a stable allocation/lifetime:
- gr_event_push(GR_EVENT_IP_ADDR_ADD, &req->addr); + struct gr_ip4_ifaddr ev = req->addr; + gr_event_push(GR_EVENT_IP_ADDR_ADD, &ev);If the serializer ever defers access, introduce a by‑value push or a copier in the event layer.
modules/ip/cli/address.c (1)
131-150: LGTM: payload switch to gr_ip4_ifaddr.Type, field accesses, and formatting with IP4_F are correct.
modules/ip6/cli/address.c (1)
136-155: LGTM: event printer uses gr_ip6_ifaddr.Correct cast and field printing with IP6_F.
frr/if_grout.h (1)
7-9: API surface aligns with per‑family address events.New wrappers and includes look good; old nexthop‑based API removed.
Also applies to: 17-18
frr/zebra_dplane_grout.c (2)
187-189: Sync path calls the new per‑family wrappers.Per‑address logging and direct calls are correct.
Also applies to: 201-203
423-481: LGTM: dplane notifications handle per‑address payloads.Casting to gr_ip{4,6}_ifaddr, logging, and delegating via grout_interface_addr{4,6}_change are correct;
gr_eis freed after use.frr/if_grout.c (2)
147-179: Helper constructs correct prefix and enqueues to zebra.AF switch with memcpy into prefix union and ifindex set from iface_id are correct; op selection handles add/del.
181-191: Wrappers correctly forward to the helper.Field mapping matches gr_ip{4,6}_ifaddr layout.
modules/ip6/control/address.c (6)
354-363: LGTM: cleanup now uses iface6_addr_del and mcast6_addr_del.Order and loops are correct; resources freed per‑entry.
419-420: Serializer size matches new payload.Switch to sizeof(gr_ip6_ifaddr) is correct.
282-293: Fix missing_ok handling (inverted logic).Return success when ENOENT and missing_ok is true.
Apply:
- if (iface6_addr_del(iface, &req->addr.addr.ip, req->addr.addr.prefixlen) < 0) - if (errno != ENOENT || req->missing_ok) - return api_out(errno, 0, NULL); + if (iface6_addr_del(iface, &req->addr.addr.ip, req->addr.addr.prefixlen) < 0) + if (errno != ENOENT || !req->missing_ok) + return api_out(errno, 0, NULL);
273-277: Fix errno propagation on mcast leave failure.Use positive errno when bubbling up errors.
Apply:
- if (mcast6_addr_del(iface, &solicited_node) < 0) { - if (errno != EOPNOTSUPP && errno != ENOENT) - return errno_set(-errno); - } + if (mcast6_addr_del(iface, &solicited_node) < 0) { + if (errno != EOPNOTSUPP && errno != ENOENT) + return errno_set(errno); + }
211-217: Ignore compound-literal UB warning Compound literals at block scope live until the end of the enclosing function, andgr_event_pushinvokes subscribers synchronously, so passing&(struct gr_ip6_ifaddr){…}is safe.Likely an incorrect or invalid review comment.
259-265: No change needed: compound literal has automatic storage until function scope exit and gr_event_push is synchronousLikely an incorrect or invalid review comment.
In both places where iface6_addr_add() is called, we also compute a solicited node multicast address and add it to the interface. Include this directly in iface6_addr_add(). Signed-off-by: Robin Jarry <[email protected]>
| return; | ||
| } | ||
| dplane_ctx_set_intf_addr(ctx, &p); | ||
| dplane_ctx_set_intf_metric(ctx, METRIC_MAX); |
There was a problem hiding this comment.
(not related to that patch)
metric and distance are valuable for the control plane, so it makes sense for grout to not have these info.
Yet, for the sync grout->frr, which metric should we declare to frr ?
Move all address deletion logic in a new iface6_addr_del() function (similar to iface6_addr_add()), including the suppression of the associated solicited node multicast address. Call that function when an API message asks to delete an address and also before deleting an interface, when all addresses must be removed. Instead of manual Ethernet filter removal when deleting an interface, call mcast6_addr_del() directly. Signed-off-by: Robin Jarry <[email protected]>
errno_set expects a positive errno value. Fixes: 0fc1cd2 ("modules: add static ipv6 forwarding support") Signed-off-by: Robin Jarry <[email protected]>
When the creation of the nexthop fails, we return a negative errno value which is then encoded in an uint32_t value. The client will receive a garbage error code. Fixes: d9a915c ("srv6: add tunsrc api") Signed-off-by: Robin Jarry <[email protected]>
Refactor interface address event handling to use dedicated address objects instead of generic nexthop structures. Change GR_EVENT_IP*ADDR* events to pass gr_ip4_ifaddr and gr_ip6_ifaddr objects directly rather than gr_nexthop structures. Update grout_interface_addr_dplane in the frr plugin to accept individual parameters instead of a nexthop structure, eliminating the need for temporary dummy objects during address synchronization. Modify event handlers and serializers to work with the new address object types. Change the addresses origin from GR_NH_ORIGIN_LINK to GR_NH_ORIGIN_INTERNAL so that these nexthops are not exposed in the API nor events. Signed-off-by: Robin Jarry <[email protected]>
This nexthop should not be exposed in the API notifications. Signed-off-by: Robin Jarry <[email protected]>
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/ip6/cli/address.c (1)
84-85: Fix wrong argument to IP6_F in list output.You pass &addr->addr (struct gr_ip6_net), but IP6_F expects a pointer to the IPv6 address field.
Apply:
- scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr, addr->addr.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr.ip, addr->addr.prefixlen);
🧹 Nitpick comments (2)
frr/zebra_dplane_grout.c (1)
187-189: Include prefix length in sync logs for clarity.Helps troubleshooting and matches route logs.
Apply:
- gr_log_debug("sync addr %pI4", &ip_addr->addr.ip); + gr_log_debug("sync addr %pI4/%u", &ip_addr->addr.ip, ip_addr->addr.prefixlen);- gr_log_debug("sync addr %pI6", &ip6_addr->addr.ip); + gr_log_debug("sync addr %pI6/%u", &ip6_addr->addr.ip, ip6_addr->addr.prefixlen);Also applies to: 201-203
modules/ip6/control/address.c (1)
147-149: Vector delete before ETH filter removal: verify failure semantics.Removing from maddrs->nh before iface_del_eth_addr may leave an orphaned ETH filter if iface_del_eth_addr fails (ret < 0). Consider deferring gr_vec_del until after successful filter removal or re-adding on failure to keep state consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frr/if_grout.c(2 hunks)frr/if_grout.h(2 hunks)frr/zebra_dplane_grout.c(4 hunks)modules/ip/cli/address.c(2 hunks)modules/ip/control/address.c(4 hunks)modules/ip6/cli/address.c(2 hunks)modules/ip6/control/address.c(8 hunks)modules/srv6/control/route.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frr/if_grout.h
- modules/ip/cli/address.c
- frr/if_grout.c
- modules/srv6/control/route.c
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
PR: DPDK/grout#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/ip/control/address.cmodules/ip6/control/address.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
PR: DPDK/grout#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/ip/control/address.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
PR: DPDK/grout#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/ip/control/address.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
PR: DPDK/grout#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/ip6/control/address.c
🧬 Code graph analysis (3)
modules/ip/control/address.c (2)
modules/infra/control/worker_test.c (1)
gr_event_push(36-36)main/event.c (1)
gr_event_push(18-29)
frr/zebra_dplane_grout.c (1)
frr/if_grout.c (2)
grout_interface_addr4_change(181-185)grout_interface_addr6_change(187-191)
modules/ip6/control/address.c (6)
api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/iface.c (2)
iface_get_eth_addr(224-237)iface_from_id(215-222)modules/infra/control/nexthop.c (2)
nexthop_new(329-375)nexthop_decref(520-555)modules/ip6/control/route.c (2)
rib6_insert(215-224)rib6_cleanup(467-469)main/event.c (1)
gr_event_push(18-29)main/gr_vec.h (1)
gr_vec_len(47-51)
⏰ 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 (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: deb
- 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: rpm
🔇 Additional comments (11)
frr/zebra_dplane_grout.c (2)
460-468: Good switch to per-IPv4 address payloads and handlers.Directly consuming gr_ip4_ifaddr and calling grout_interface_addr4_change is correct and consistent with the new API.
470-481: Good switch to per-IPv6 address payloads and handlers.Directly consuming gr_ip6_ifaddr and calling grout_interface_addr6_change is correct and consistent with the new API.
modules/ip/control/address.c (2)
92-92: Origin INTERNAL for local iface NH is correct.Prevents spurious NEXTHOP_NEW/DELETE events for local addresses.
104-105: Event payload lifetime: avoid passing transient request buffer.You pass &req->addr to gr_event_push. If any subscriber or serializer copies after the request buffer is freed, this can turn into UAF.
Please confirm event dispatch is strictly synchronous and all consumers copy immediately. If not, switch to a stable payload (e.g., heap-allocate a gr_ip4_ifaddr and ensure a cleanup path in the consumer/serializer) or change the event framework to copy by value.
Run this to locate serializer/consumer code paths and check whether they copy immediately:
#!/bin/bash # Find serializers and their callbacks, and where they handle IP addr events rg -nP -C3 'gr_event_register_serializer|struct\s+gr_event_serializer' rg -nP -C3 '\bGR_EVENT_IP_ADDR_(ADD|DEL)\b' rg -nP -C3 '\bgr_event_push\s*\('Also applies to: 131-132
modules/ip6/cli/address.c (1)
136-136: CLI event printer update looks good.Switch to const gr_ip6_ifaddr* with correct field access.
Also applies to: 150-155
modules/ip6/control/address.c (6)
121-123: errno propagation fix is correct.Returning errno_set(errno) on nexthop_new failure fixes the prior negative-errno bug.
189-190: Origin INTERNAL for local iface NH is correct.Aligns with the intent to suppress nexthop lifecycle events for internal objects.
197-205: Solicited-node join handling is robust.Joining before route insert and tolerating EOPNOTSUPP/EEXIST with proper errno propagation looks good.
354-357: Cleanup loops on IFACE_PRE_REMOVE look correct.Iterative del using iface6_addr_del/mcast6_addr_del until vectors empty ensures consistent teardown order.
Also applies to: 360-363
290-293: missing_ok handling fixed.Correctly returns success when address is absent and missing_ok is true.
211-217: Drop ephemeral-lifetime concern: synchronous dispatch + deep-copy serializer prevents UAF. Compound literals persist through the call and are memcpy’ed by the registered serializer. No action needed.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Other