Skip to content

zebra: Copy the NHG ID in copy_state for NHT events#21892

Open
GaladrielZhao wants to merge 1 commit into
FRRouting:masterfrom
GaladrielZhao:fix/copy_nhg_id
Open

zebra: Copy the NHG ID in copy_state for NHT events#21892
GaladrielZhao wants to merge 1 commit into
FRRouting:masterfrom
GaladrielZhao:fix/copy_nhg_id

Conversation

@GaladrielZhao
Copy link
Copy Markdown
Contributor

The copy_state function creates a temporary copy of the route entry's nexthop group for use in nexthop tracking (NHT) notifications to protocol clients. When zebra forwards an NHT event to fpmsyncd, the nexthop group ID stored in the copied nhe should be preserved so that fpmsyncd can identify the NHG and build the FIB entry correctly.

Change the ID argument of zebra_nhe_copy from 0 to re->nhe->id so that the copied NHE carries the correct identifier.

The copy_state function creates a temporary copy of the route entry's
nexthop group for use in nexthop tracking (NHT) notifications to protocol
clients. When zebra forwards an NHT event to fpmsyncd, the nexthop group
ID stored in the copied nhe should be preserved so that fpmsyncd can
identify the NHG and build the FIB entry correctly.

Change the ID argument of zebra_nhe_copy from 0 to re->nhe->id so that
the copied NHE carries the correct identifier.

Signed-off-by: Yuqing Zhao <galadriel.zyq@alibaba-inc.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 8, 2026

Greptile Summary

This PR fixes a one-line bug in copy_state() in zebra/zebra_rnh.c where the nexthop group ID was being zeroed out when the temporary route_entry snapshot was created for NHT notifications, preventing fpmsyncd from identifying the NHG.

  • Core fix: zebra_nhe_copy(re->nhe, 0)zebra_nhe_copy(re->nhe, re->nhe->id), preserving the NHG ID in the snapshot so fpmsyncd can look up and build FIB entries for NHT events.
  • Safety: free_state() frees the copy via zebra_nhg_free(), which only releases memory and never touches the NHG hash table, so carrying a real ID in the copy poses no risk of corrupting the live NHE.

Confidence Score: 5/5

Safe to merge; the one-line change is narrowly scoped and well-supported by existing patterns in the codebase.

The fix is a single-line correction that passes the real NHG ID rather than 0 to zebra_nhe_copy. The free path (free_state → zebra_nhg_free) only releases memory and never performs hash-table removal, so a non-zero ID in the copy cannot corrupt the live NHE. The same pattern of copying an NHE with its real ID already exists in zebra_nhg.c at multiple call sites, confirming the approach is sound.

No files require special attention; the change is confined to a single call site in zebra_rnh.c.

Important Files Changed

Filename Overview
zebra/zebra_rnh.c Single-line fix preserving the NHG ID in the NHT state snapshot; free path via zebra_nhg_free() is safe with a non-zero ID

Sequence Diagram

sequenceDiagram
    participant ZebraRIB as Zebra RIB
    participant copy_state
    participant rnh_state as rnh->state (snapshot)
    participant notify as zebra_rnh_notify_protocol_clients
    participant fpmsyncd

    ZebraRIB->>copy_state: "re (route_entry with nhe->id=X)"
    copy_state->>copy_state: "zebra_nhe_copy(re->nhe, re->nhe->id)"
    Note over copy_state: Before fix: id=0 in copy<br/>After fix: id=X preserved in copy
    copy_state->>rnh_state: "state->nhe->id = X (preserved)"
    copy_state->>notify: "rnh->state passed to clients"
    notify->>fpmsyncd: "NHT event with NHG id=X"
    Note over fpmsyncd: Can now identify NHG<br/>and build FIB entry correctly
Loading

Reviews (1): Last reviewed commit: "zebra: Copy the NHG ID in copy_state for..." | Re-trigger Greptile

@GaladrielZhao
Copy link
Copy Markdown
Contributor Author

Hi @mjstapp , I noticed the zebra_nhe_copy call in copy_state passes 0 for the ID argument. Is there a specific reason for that? We're looking at preserving the NHE ID in NHT events for fpmsyncd and wanted to understand the original design intent. Thanks.

@mjstapp
Copy link
Copy Markdown
Contributor

mjstapp commented May 12, 2026

I think, if you look at the zebra_nhg_free path you'll see that there may be side effects if the object being freed has a meaningful id - that path is used within the normal zebra nhg processing. At least, that seems like it might be the reason we didn't try to copy the id.

Hi @mjstapp , I noticed the zebra_nhe_copy call in copy_state passes 0 for the ID argument. Is there a specific reason for that? We're looking at preserving the NHE ID in NHT events for fpmsyncd and wanted to understand the original design intent. Thanks.

Comment thread zebra/zebra_rnh.c
state->status = re->status;

state->nhe = zebra_nhe_copy(re->nhe, 0);
state->nhe = zebra_nhe_copy(re->nhe, re->nhe->id);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be safer to use a separate dedicated cell for the id that's being referenced. I'm a little concerned about possible side-effects if there's a copy with a valid id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants