zebra: Use zebra dplane for RTM route message#230
Open
mwinter-osr wants to merge 2 commits intoopensourcerouting:masterfrom
Open
zebra: Use zebra dplane for RTM route message#230mwinter-osr wants to merge 2 commits intoopensourcerouting:masterfrom
mwinter-osr wants to merge 2 commits intoopensourcerouting:masterfrom
Conversation
Introduce getter and setter functions for route-related attributes in the zebra dplane context, including startup flag, nexthop ifindex, multipath status, nexthop group, and single nexthop. Signed-off-by: Canzovo <1037306097@qq.com>
a) Move the processing of netlink RTM_NEWROUTE and RTM_DELROUTE messages into the dataplane. b) Introduced `rib_route_dplane_update()` to process the queued dplane context and perform the actual RIB update using the stored data. c) Defer VRF lookup and assignment of vrf_id for each nexthop to the main thread, as `Map to VRF` should be done here. Signed-off-by: Canzovo <1037306097@qq.com>
Greptile SummaryThis PR offloads RTM_NEWROUTE/RTM_DELROUTE kernel netlink message parsing to the dplane pthread, following the same pattern as FRRouting#19296 for other message types. The core change adds
|
| Filename | Overview |
|---|---|
| zebra/rt_netlink.c | Adds netlink_route_change_read_unicast_internal and netlink_route_read_unicast_ctx to parse RTM_NEWROUTE/DELROUTE into a dplane ctx; contains a NULL-deref crash (ng passed as NULL to nexthop_group_copy) and a missing prefsrc recovery after ctx parsing. |
| zebra/zebra_rib.c | Adds rib_route_dplane_update to process kernel route notifications from the dplane queue; contains a double-free bug (ctx freed both inside the function's early exit and unconditionally by the caller) and an unguarded nexthop dereference in rib_update_nexthop_vrf_id. |
| zebra/zebra_dplane.c | Adds accessor functions for new rinfo fields (bhtype, prefsrc, gw, startup, rtnexthop_ifindex, is_multipath, route_nhg, nexthop); memory cleanup for route_nhg in dplane_ctx_free is correct. |
| zebra/zebra_dplane.h | Declares new dplane ctx accessor APIs for route-level fields; interface is clean and consistent with existing patterns. |
| zebra/kernel_netlink.c | Routes RTM_NEWROUTE/DELROUTE to netlink_route_change inside dplane_netlink_information_fetch; change is minimal and correct. |
| zebra/rib.h | Adds declaration for rib_route_dplane_update; straightforward header change. |
| zebra/rt_netlink.h | Adds declaration for netlink_route_notify_read_ctx; straightforward header change. |
Sequence Diagram
sequenceDiagram
participant K as Linux Kernel
participant DP as dplane pthread<br/>(kernel_netlink.c)
participant NL as rt_netlink.c
participant Q as dplane ctx queue
participant MT as zebra main pthread<br/>(zebra_rib.c)
K->>DP: RTM_NEWROUTE / RTM_DELROUTE
DP->>NL: netlink_route_change()
NL->>NL: netlink_route_change_read_unicast_internal()
NL->>NL: netlink_parse_rtattr(tb)
NL->>NL: dplane_ctx_alloc()
NL->>NL: netlink_route_read_unicast_ctx(h, ns_id, tb, ctx)
Note over NL: sets prefix, afi, table, flags,<br/>metric, distance, gw, prefsrc in ctx
NL->>NL: parse_nexthop_unicast / parse_multipath_nexthops_unicast
Note over NL: ⚠ prefsrc always NULL (bug)<br/>⚠ NULL ng when nhe_id!=0 (crash)
NL->>NL: dplane_ctx_set_notif_provider(ctx, NOTIF_PROVIDER_KERNEL)
NL->>Q: dplane_provider_enqueue_to_zebra(ctx)
Q-->>MT: event wakeup
MT->>MT: rib_process_dplane_results()
MT->>MT: zebra_rib_translate_ctx_from_dplane() [VRF resolution]
alt "notif_provider != 0"
MT->>MT: rib_route_dplane_update(ctx)
Note over MT: rib_add_multipath / rib_delete
else "notif_provider == 0"
MT->>MT: rib_process_result(ctx)
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: zebra/rt_netlink.c
Line: 1204
Comment:
**NULL dereference crash when kernel NHG routes are received**
When `nhe_id != 0` (the kernel sends `RTA_NH_ID`), `ng` is never allocated and remains `NULL`. Passing `NULL` to `dplane_ctx_route_set_nexthop_group` calls `nexthop_group_copy(to, NULL)`, which immediately dereferences `from->nhgr` → SIGSEGV. This crash fires on any modern Linux kernel (≥ 4.19) with nexthop object support enabled, i.e., whenever the kernel uses kernel NHG IDs for installed routes.
```suggestion
dplane_ctx_route_set_is_multipath(ctx, is_multipath);
if (ng)
dplane_ctx_route_set_nexthop_group(ctx, ng);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: zebra/zebra_rib.c
Line: 1740-1742
Comment:
**Double-free / use-after-free of `ctx`**
`rib_route_dplane_update` calls `dplane_ctx_fini(&ctx)` here (setting its *local* copy to NULL), then returns. The caller, `rib_process_dplane_results`, still holds the original pointer and unconditionally calls `dplane_ctx_fini(&ctx)` at line 5459 — freeing already-freed memory. The fix is to not free `ctx` here and let the caller own the lifetime (consistent with every other dispatch branch in `rib_process_dplane_results`).
```suggestion
if (IS_ZEBRA_DEBUG_KERNEL)
zlog_debug("Route opcode: (%s) unable to parse table received %u, ignoring",
dplane_op2str(op), table);
return;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: zebra/rt_netlink.c
Line: 1150-1171
Comment:
**`prefsrc` is never recovered from ctx — always NULL when building nexthops**
`gate` is correctly recovered from the context after `netlink_route_read_unicast_ctx` (lines 1150-1152), but `prefsrc` is left as `NULL` (initialised at line 1049 and never updated). Both `parse_nexthop_unicast` (lines 1170, 1216) and `parse_multipath_nexthops_unicast` (line 1191) receive `prefsrc = NULL`, so `nh.src` / the IPv4 preferred-source address is silently dropped for every route that carries `RTA_PREFSRC`. The fix mirrors how `gate_addr` is retrieved:
```c
const struct ipaddr *prefsrc_addr = dplane_ctx_get_route_prefsrc(ctx);
if (prefsrc_addr && !IS_IPADDR_NONE(prefsrc_addr))
prefsrc = (void *)&(prefsrc_addr->ip.addr);
```
This block should be added alongside the `gate_addr` retrieval (around line 1150).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: zebra/rt_netlink.c
Line: 119-124
Comment:
**Misleading enum name `NOTIF_PROVIDER_DPLANE = 0`**
`NOTIF_PROVIDER_DPLANE` has value `0`, which is also the zero-initialised default of `zd_notif_provider` in every normal (non-notification) dplane ctx. The name implies "this notification came from the dplane", but the actual semantics of the check in `rib_process_dplane_results` (`== 0` → normal result, `!= 0` → kernel/FPM notification) mean this slot is really "no notification provider". Consider renaming to `NOTIF_PROVIDER_NONE` (or just using `0` without the enum value) to avoid confusion with future readers.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: zebra/zebra_rib.c
Line: 1783-1788
Comment:
**`rib_update_nexthop_vrf_id` is called unconditionally before the `nhe_id` guard**
`rib_update_nexthop_vrf_id` accesses `ng->nexthop->vrf_id` in the non-multipath branch without first checking that `ng->nexthop != NULL`. When `nhe_id != 0`, `route_nhg.nexthop` may be NULL (no inline nexthops were stored), leading to a NULL dereference. The call should be guarded:
```c
if (!nhe_id)
rib_update_nexthop_vrf_id(is_multipath, ns_id, index, rtnh_ifindex, ng, vrf_id);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "zebra: Use zebra dplane for RTM route me..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| __func__, &p); | ||
| zebra_rib_route_entry_free(re); | ||
| dplane_ctx_route_set_is_multipath(ctx, is_multipath); | ||
| dplane_ctx_route_set_nexthop_group(ctx, ng); |
There was a problem hiding this comment.
When nhe_id != 0 (the kernel sends RTA_NH_ID), ng is never allocated and remains NULL. Passing NULL to dplane_ctx_route_set_nexthop_group calls nexthop_group_copy(to, NULL), which immediately dereferences from->nhgr → SIGSEGV. This crash fires on any modern Linux kernel (≥ 4.19) with nexthop object support enabled, i.e., whenever the kernel uses kernel NHG IDs for installed routes.
Suggested change
| dplane_ctx_route_set_nexthop_group(ctx, ng); | |
| dplane_ctx_route_set_is_multipath(ctx, is_multipath); | |
| if (ng) | |
| dplane_ctx_route_set_nexthop_group(ctx, ng); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/rt_netlink.c
Line: 1204
Comment:
**NULL dereference crash when kernel NHG routes are received**
When `nhe_id != 0` (the kernel sends `RTA_NH_ID`), `ng` is never allocated and remains `NULL`. Passing `NULL` to `dplane_ctx_route_set_nexthop_group` calls `nexthop_group_copy(to, NULL)`, which immediately dereferences `from->nhgr` → SIGSEGV. This crash fires on any modern Linux kernel (≥ 4.19) with nexthop object support enabled, i.e., whenever the kernel uses kernel NHG IDs for installed routes.
```suggestion
dplane_ctx_route_set_is_multipath(ctx, is_multipath);
if (ng)
dplane_ctx_route_set_nexthop_group(ctx, ng);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1740
to
+1742
| if (ctx) | ||
| dplane_ctx_fini(&ctx); | ||
| return; |
There was a problem hiding this comment.
ctx
rib_route_dplane_update calls dplane_ctx_fini(&ctx) here (setting its local copy to NULL), then returns. The caller, rib_process_dplane_results, still holds the original pointer and unconditionally calls dplane_ctx_fini(&ctx) at line 5459 — freeing already-freed memory. The fix is to not free ctx here and let the caller own the lifetime (consistent with every other dispatch branch in rib_process_dplane_results).
Suggested change
| if (ctx) | |
| dplane_ctx_fini(&ctx); | |
| return; | |
| if (IS_ZEBRA_DEBUG_KERNEL) | |
| zlog_debug("Route opcode: (%s) unable to parse table received %u, ignoring", | |
| dplane_op2str(op), table); | |
| return; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/zebra_rib.c
Line: 1740-1742
Comment:
**Double-free / use-after-free of `ctx`**
`rib_route_dplane_update` calls `dplane_ctx_fini(&ctx)` here (setting its *local* copy to NULL), then returns. The caller, `rib_process_dplane_results`, still holds the original pointer and unconditionally calls `dplane_ctx_fini(&ctx)` at line 5459 — freeing already-freed memory. The fix is to not free `ctx` here and let the caller own the lifetime (consistent with every other dispatch branch in `rib_process_dplane_results`).
```suggestion
if (IS_ZEBRA_DEBUG_KERNEL)
zlog_debug("Route opcode: (%s) unable to parse table received %u, ignoring",
dplane_op2str(op), table);
return;
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
1150
to
+1171
| gate_addr = dplane_ctx_get_route_gw(ctx); | ||
| if (!IS_IPADDR_NONE(gate_addr)) | ||
| gate = (void *)&(gate_addr->ip.addr); | ||
|
|
||
| nhe_id = dplane_ctx_get_nhe_id(ctx); | ||
|
|
||
| metric = dplane_ctx_get_metric(ctx); | ||
| distance = dplane_ctx_get_distance(ctx); | ||
| tag = dplane_ctx_get_tag(ctx); | ||
| mtu = dplane_ctx_get_mtu(ctx); | ||
|
|
||
| afi = dplane_ctx_get_afi(ctx); | ||
|
|
||
| bh_type = dplane_ctx_get_route_bhtype(ctx); | ||
|
|
||
| if (IS_ZEBRA_DEBUG_KERNEL) { | ||
| char buf2[PREFIX_STRLEN]; | ||
|
|
||
| zlog_debug( | ||
| "%s %pFX%s%s vrf %s(%u) table_id: %u metric: %d Admin Distance: %d", | ||
| nl_msg_type_to_str(h->nlmsg_type), &p, | ||
| src_p.prefixlen ? " from " : "", | ||
| src_p.prefixlen ? prefix2str(&src_p, buf2, sizeof(buf2)) | ||
| : "", | ||
| vrf_id_to_name(vrf_id), vrf_id, table, metric, | ||
| distance); | ||
| } | ||
|
|
||
| if (h->nlmsg_type == RTM_NEWROUTE) { | ||
| struct route_entry *re; | ||
| dplane_ctx_set_op(ctx, DPLANE_OP_ROUTE_INSTALL); | ||
|
|
||
| struct nexthop_group *ng = NULL; | ||
|
|
||
| re = zebra_rib_route_entry_new(vrf_id, proto, 0, flags, nhe_id, | ||
| table, metric, mtu, distance, | ||
| tag); | ||
| if (!nhe_id) | ||
| ng = nexthop_group_new(); | ||
|
|
||
| if (!tb[RTA_MULTIPATH]) { | ||
| struct nexthop *nexthop, nh; | ||
|
|
||
| if (!nhe_id) { | ||
| nh = parse_nexthop_unicast( | ||
| ns_id, rtm, tb, bh_type, index, prefsrc, | ||
| gate, afi, vrf_id); | ||
| nh = parse_nexthop_unicast(ns_id, rtm, tb, bh_type, index, prefsrc, | ||
| gate, afi); |
There was a problem hiding this comment.
prefsrc is never recovered from ctx — always NULL when building nexthops
gate is correctly recovered from the context after netlink_route_read_unicast_ctx (lines 1150-1152), but prefsrc is left as NULL (initialised at line 1049 and never updated). Both parse_nexthop_unicast (lines 1170, 1216) and parse_multipath_nexthops_unicast (line 1191) receive prefsrc = NULL, so nh.src / the IPv4 preferred-source address is silently dropped for every route that carries RTA_PREFSRC. The fix mirrors how gate_addr is retrieved:
const struct ipaddr *prefsrc_addr = dplane_ctx_get_route_prefsrc(ctx);
if (prefsrc_addr && !IS_IPADDR_NONE(prefsrc_addr))
prefsrc = (void *)&(prefsrc_addr->ip.addr);This block should be added alongside the gate_addr retrieval (around line 1150).
Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/rt_netlink.c
Line: 1150-1171
Comment:
**`prefsrc` is never recovered from ctx — always NULL when building nexthops**
`gate` is correctly recovered from the context after `netlink_route_read_unicast_ctx` (lines 1150-1152), but `prefsrc` is left as `NULL` (initialised at line 1049 and never updated). Both `parse_nexthop_unicast` (lines 1170, 1216) and `parse_multipath_nexthops_unicast` (line 1191) receive `prefsrc = NULL`, so `nh.src` / the IPv4 preferred-source address is silently dropped for every route that carries `RTA_PREFSRC`. The fix mirrors how `gate_addr` is retrieved:
```c
const struct ipaddr *prefsrc_addr = dplane_ctx_get_route_prefsrc(ctx);
if (prefsrc_addr && !IS_IPADDR_NONE(prefsrc_addr))
prefsrc = (void *)&(prefsrc_addr->ip.addr);
```
This block should be added alongside the `gate_addr` retrieval (around line 1150).
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+119
to
+124
| enum notif_provider { | ||
| NOTIF_PROVIDER_DPLANE, | ||
| NOTIF_PROVIDER_KERNEL, | ||
| }; | ||
|
|
||
| static enum notif_provider notif_src = NOTIF_PROVIDER_KERNEL; |
There was a problem hiding this comment.
NOTIF_PROVIDER_DPLANE = 0
NOTIF_PROVIDER_DPLANE has value 0, which is also the zero-initialised default of zd_notif_provider in every normal (non-notification) dplane ctx. The name implies "this notification came from the dplane", but the actual semantics of the check in rib_process_dplane_results (== 0 → normal result, != 0 → kernel/FPM notification) mean this slot is really "no notification provider". Consider renaming to NOTIF_PROVIDER_NONE (or just using 0 without the enum value) to avoid confusion with future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/rt_netlink.c
Line: 119-124
Comment:
**Misleading enum name `NOTIF_PROVIDER_DPLANE = 0`**
`NOTIF_PROVIDER_DPLANE` has value `0`, which is also the zero-initialised default of `zd_notif_provider` in every normal (non-notification) dplane ctx. The name implies "this notification came from the dplane", but the actual semantics of the check in `rib_process_dplane_results` (`== 0` → normal result, `!= 0` → kernel/FPM notification) mean this slot is really "no notification provider". Consider renaming to `NOTIF_PROVIDER_NONE` (or just using `0` without the enum value) to avoid confusion with future readers.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1783
to
+1788
|
|
||
| ng = dplane_ctx_route_get_nexthop_group(ctx); | ||
| rtnh_ifindex = dplane_ctx_route_get_rtnexthop_ifindex(ctx); | ||
|
|
||
| rib_update_nexthop_vrf_id(is_multipath, ns_id, index, rtnh_ifindex, ng, vrf_id); | ||
|
|
There was a problem hiding this comment.
rib_update_nexthop_vrf_id is called unconditionally before the nhe_id guard
rib_update_nexthop_vrf_id accesses ng->nexthop->vrf_id in the non-multipath branch without first checking that ng->nexthop != NULL. When nhe_id != 0, route_nhg.nexthop may be NULL (no inline nexthops were stored), leading to a NULL dereference. The call should be guarded:
if (!nhe_id)
rib_update_nexthop_vrf_id(is_multipath, ns_id, index, rtnh_ifindex, ng, vrf_id);Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/zebra_rib.c
Line: 1783-1788
Comment:
**`rib_update_nexthop_vrf_id` is called unconditionally before the `nhe_id` guard**
`rib_update_nexthop_vrf_id` accesses `ng->nexthop->vrf_id` in the non-multipath branch without first checking that `ng->nexthop != NULL`. When `nhe_id != 0`, `route_nhg.nexthop` may be NULL (no inline nexthops were stored), leading to a NULL dereference. The call should be guarded:
```c
if (!nhe_id)
rib_update_nexthop_vrf_id(is_multipath, ns_id, index, rtnh_ifindex, ng, vrf_id);
```
How can I resolve this? If you propose a fix, please make it concise.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Similar to FRRouting#19296, this PR offloads the parsing and handling of RTM route messages to the dplane.
Original PR19536 by Canzovo