zebra: Send enough NHG information to dplane to support RIB/FIB - #21415
zebra: Send enough NHG information to dplane to support RIB/FIB#21415GaladrielZhao wants to merge 2 commits into
Conversation
Greptile SummaryThis PR extends zebra's dataplane with richer nexthop-group context for FPM/SONiC consumers: a full recursive dependency tree ( Key issues found:
Confidence Score: 1/5Not safe to merge — the unconditional NULL dereference in Two correctness bugs on the hot path: a guaranteed crash (NULL dereference for every route without
Important Files Changed
Sequence DiagramsequenceDiagram
participant Proto as Protocol Client
participant RIB as zebra_rib
participant NHG as zebra_nhg
participant DPlane as zebra_dplane
participant NetLink as rt_netlink
participant FPM as FPM/SONiC
Proto->>RIB: route add (with NHE)
RIB->>NHG: route_entry_update_original_nhe(nhe)
NHG-->>NHG: zebra_nhg_mark_received_flag(nhe) sets NEXTHOP_GROUP_RECEIVED + VALID
RIB->>NHG: route_entry_update_nhe(nhe)
RIB->>NHG: rib_install_kernel(re)
NHG->>NHG: zebra_nhg_install_kernel(re->nhe)
NHG->>NHG: zebra_nhg_install_kernel(re->nhe_received)
NHG->>DPlane: dplane_nexthop_add(nhe)
DPlane->>DPlane: dplane_ctx_nexthop_init() builds nh_grp_full tree
DPlane->>DPlane: sets skip_kernel if RECEIVED or nhg_fib+RECURSIVE
DPlane->>NetLink: netlink_put_nexthop_update_msg()
alt skip_kernel set
NetLink-->>DPlane: FRR_NETLINK_SUCCESS (no kernel write)
else normal NHG
NetLink->>NetLink: netlink_batch_add_msg() to kernel
end
DPlane->>FPM: ctx with full NHG tree (nh_grp_full, nhg_flags, nhe_received_id)
FPM->>FPM: reconstruct forwarding topology for SONiC FIB
Prompt To Fix All With AIThis is a comment left during a code review.
Path: zebra/zebra_rib.c
Line: 655
Comment:
**Unconditional NULL dereference on `re->nhe_received`**
`re->nhe_received` is only set when `route_entry_update_original_nhe()` is called (i.e. routes submitted with a received NHE). For the vast majority of routes it remains `NULL` (its default zero-initialised value in `route_entry`). `zebra_nhg_install_kernel()` immediately dereferences the pointer via `CHECK_FLAG(nhe->flags, ...)` at the very start of the function — no NULL guard exists there.
Every call to `rib_install_kernel()` for a normal route will crash the zebra process. A guard is required:
```suggestion
zebra_nhg_install_kernel(re->nhe, re->type);
if (re->nhe_received)
zebra_nhg_install_kernel(re->nhe_received, re->type);
```
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_nhg.c
Line: 3592-3595
Comment:
**Missing `i++` for leaf nodes — array slots overwritten**
In the `else` branch (leaf NHG, i.e. `zebra_nhg_depends_is_empty(curr_node)` is true), after writing to `grp_full[i]` the index `i` is never incremented. Every subsequent leaf-level dependency will overwrite the same slot. For any NHG with two or more leaf siblings only the last one survives in the array, and `nh_grp_full_count` (the return value) will also be wrong.
Compare the non-leaf branch on line 3556 which correctly does `i++`. The fix:
```suggestion
grp_full[i].id = curr_node->id;
grp_full[i].weight = found ? nexthop->weight : 0;
grp_full[i].num_direct = 0;
i++;
```
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_nhg.h
Line: 28-32
Comment:
**`weight` field truncated from 16-bit to 8-bit**
`nh_grp` (the struct this mirrors) declares `weight` as `uint16_t`, and the existing `--nexthop-weight-16-bit` option allows weights up to 65535. Using `uint8_t` here silently truncates any weight above 255 when the FPM consumer reads `nh_grp_full`. Consider aligning with `nh_grp`:
```suggestion
struct nh_grp_full {
uint32_t id;
uint16_t weight;
uint32_t num_direct;
};
```
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_nhg.c
Line: 3540-3550
Comment:
**`num_direct` undercounts queued sub-dependencies**
When computing `sub_depend_count` for `grp_full[i].num_direct`, the filter only accepts `NEXTHOP_GROUP_INSTALLED` nodes (line 3546). However, the outer loop (lines 3513-3515) deliberately includes `NEXTHOP_GROUP_QUEUED` nodes too. Any sub-dependency that is queued-but-not-yet-installed will be excluded from `num_direct` but present in the flat array, causing the count to mismatch the actual number of children written. The QUEUED check should be added for consistency:
```suggestion
frr_each(nhg_connected_tree, &curr_node->nhg_depends, sub_rb_node) {
/* Apply same filters as main loop */
if (!is_srv6_nhg(sub_rb_node->nhe)
&& !CHECK_FLAG(sub_rb_node->nhe->flags, NEXTHOP_GROUP_VALID))
continue;
if (!is_srv6_nhg(sub_rb_node->nhe)
&& !(CHECK_FLAG(sub_rb_node->nhe->flags, NEXTHOP_GROUP_INSTALLED) ||
CHECK_FLAG(sub_rb_node->nhe->flags, NEXTHOP_GROUP_QUEUED)))
continue;
sub_depend_count++;
}
```
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_nhg.c
Line: 3481-3483
Comment:
**`dep_scan_count` is computed but never used**
The variable `dep_scan_count` is populated by iterating through `nhe->nhg_depends` but is never referenced after that. This is dead code that adds an unnecessary full traversal of the tree on every call. It should be removed.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "zebra: Send enough NHG information to dp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
56f9be2 to
feb57f3
Compare
495a37f to
f090395
Compare
d1269f9 to
4dc1310
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
20e9673 to
79535b7
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Tick the box to add this pull request to the merge queue (same as
|
34cf5da to
146b10f
Compare
|
Hi @donaldsharp , @mjstapp , The mainline logic stays untouched — RECEIVED/RECURSIVE NHGs in nhg-fib mode get the new "INSTALLED_FPM_ONLY" flag instead of "INSTALLED", so Could you please review this PR? Many thanks. |
|
Hi @donaldsharp and @mjstapp, gentle ping on this PR. Could you please help review when you have a chance? I'd really appreciate it. |
738df1b to
d3f5031
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
39dba3c to
9552b4e
Compare
To support RIB/FIB use cases (e.g. SONiC FIB construction via FPM), zebra needs to pass richer nexthop group information through the dataplane and send additional NHG types that were previously skipped. This commit makes the following changes: 1 Extend dplane NHG context with full dependency information 2 Introduce NEXTHOP_GROUP_RECEIVED flag and zebra_nhg_mark_received_flag() 3 Add --nhg-fib command-line option to enable RIB/FIB mode Signed-off-by: Yuqing Zhao <galadriel.zyq@alibaba-inc.com>
Signed-off-by: Yuqing Zhao <galadriel.zyq@alibaba-inc.com>
| if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) || | ||
| CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED_FPM_ONLY)) | ||
| dplane_ctx_set_skip_kernel(ctx); | ||
| UNSET_FLAG(nhe->flags, NEXTHOP_GROUP_REINSTALL_FPM_ONLY); |
There was a problem hiding this comment.
We clear REINSTALL_FPM_ONLY before calling dplane_update_enqueue(). If the enqueue fails, or dependents change again while the NHG is still queued, we can lose that reinstall. Would it be safer to clear the flag only after a successful enqueue, or from the dplane result path?
| * RECURSIVE: intermediate hop, FPM needs recursive address for backwalk. | ||
| * Both are marked skip_kernel in dplane_ctx_nexthop_init(). | ||
| */ | ||
| if (!(zebra_nhg_fib_enabled && (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_RECEIVED) || |
There was a problem hiding this comment.
Skipping zebra_nhg_resolve() for recursive NHGs when nhg-fib is on is a fairly big behavior change. Just want to make sure we're solid that the kernel still never sees those unresolved recursive groups (skip_kernel covers that), and that the dependents backwalk still does the right thing when the recursive NHG stays unresolved in zebra.
|
|
||
| /* We are not supporting backup things temporarily, so we just record */ | ||
| if (IS_ZEBRA_DEBUG_NHG_DETAIL) | ||
| zlog_debug("%s: skipping backup nhe", __func__); |
There was a problem hiding this comment.
nhe2grp_full currently skips backup_info altogether. That's fine for phase 1 if we're not supporting it yet, but it would help to call that out somewhere so we don't quietly lose backup paths later.
| * expanded ECMP recursion. This may be increased if deeper recursion | ||
| * is needed in the future. | ||
| */ | ||
| #define NH_GRP_FULL_NUM (MULTIPATH_NUM * 2) |
There was a problem hiding this comment.
NH_GRP_FULL_NUM = MULTIPATH_NUM * 2 basically assumes we won't go much deeper than a couple of recursion levels. On a deeper ECMP / recursive setup we'll hit the overflow path and abort the update. Is that the failure mode we want, or should we size this differently?
| case OPTION_V6_WITH_V4_NEXTHOP: | ||
| v6_with_v4_nexthop = true; | ||
| break; | ||
| case OPTION_NHG_FIB: |
There was a problem hiding this comment.
Very minor — --nhg-fib is in longopts unconditionally, but the case that sets the flag sits under #ifdef HAVE_NETLINK (see the #endif just below). Not a problem for SONiC, just a little inconsistent.
3 participants
This change extends the zebra dataplane with richer nexthop group information and introduces a new --nhg-fib mode to enable RIB/FIB separation for SONiC FIBconstruction via FPM.
This commit makes the following changes:
1 Richer NHG context in dplane
The dplane NHG context is extended with a nh_grp_full array that captures the complete dependency tree including intermediate recursive nodes. Each entry carries a num_direct count so consumers can reconstruct the recursive hierarchy from the flat array. depends/dependents ID arrays and the NHG flag bitmask are also added so FPM can reconstruct the full forwarding topology.
2 Forwarding received NHGs to dplane
A new NEXTHOP_GROUP_RECEIVED flag is introduced to mark protocol-owned NHGs that should not have their addresses overwritten by zebra resolution. zebra_nhg_mark_received_flag() recursively sets this flag together with NEXTHOP_GROUP_VALID on the NHE and all its depends. The nhe_received field is now installed into the dplane alongside the resolved NHE so FPM providers can access both representations.
3 --nhg-fib command-line option
When --nhg-fib is set, recursive and received NHGs are sent to the dplane with skip_kernel, allowing FPM to perform backwalk and recursive nexthop resolution while keeping them out of the kernel. zebra_nhg_install_kernel() skips zebra_nhg_resolve() for these NHGs so they enter dplane in their original form.
4 FPM-only reinstall on dependent changes
A new NEXTHOP_GROUP_REINSTALL_FPM_ONLY flag is introduced to handle the case where an NHG's dependents list changes (add/remove) while the NHG itself is already installed in the kernel. When triggered, the NHG is re-enqueued to dplane with skip_kernel so FPM receives the updated dependents without redundant kernel programming.If the change occurs while an install is already in-flight (QUEUED), the flag is deferred and picked up in zebra_nhg_dplane_result() upon completion.
Topotests covering the above scenarios have been added.