Skip to content

zebra: skip NHGs belonging to different VRF - #22686

Open
anlancs wants to merge 2 commits into
FRRouting:masterfrom
anlancs:fix/zebra-vrf
Open

anlancs wants to merge 2 commits into
FRRouting:masterfrom
anlancs:fix/zebra-vrf

Conversation

@anlancs

@anlancs anlancs commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

When an interface migrates between VRFs, zebra_interface_nhg_reinstall() iterates all NHGs in the interface's zif->nhg_dependents tree and reinstalls them. However, the interface's nhg_dependents tree still contains singleton NHGs created while the interface belonged to the previous VRF. zebra_interface_nhg_reinstall() MAYBE blindly re-activate and reinstall these stale NHGs.

Please check the commit log for details.

@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a bug in zebra_interface_nhg_reinstall() where singleton NHGs created while an interface belonged to a previous VRF were incorrectly re-activated and reinstalled when the interface migrated to a new VRF. A one-line VRF-ID guard is added to skip such stale entries, and a topotest validates the expected "not reinstalled" behavior end-to-end.

  • zebra/zebra_nhg.c: Adds a guard in the nhg_dependents walk that skips any NHG whose nexthop vrf_id differs from the interface's current vrf_id, preventing stale old-VRF NHGs from being re-validated and pushed to the kernel.
  • tests/topotests/zebra_rib/test_zebra_rib.py: New topotest test_zebra_stale_nhg_not_reinstalled_after_vrf_change that creates a VRF/interface/static route, removes the route, migrates the interface out of the VRF, then asserts the stale NHE is in keep-around state and neither valid nor installed.

Confidence Score: 5/5

Safe to merge — the change is a narrow, well-placed guard that prevents stale NHGs from being reinstalled during VRF migration, with no impact on the normal reinstall path.

The fix is a single conditional inserted before the activation and kernel-install logic in zebra_interface_nhg_reinstall(). It only skips NHGs whose nexthop vrf_id no longer matches the interface's current VRF, which is exactly the stale-entry scenario described in the PR. The accompanying topotest covers the end-to-end flow. No regressions are introduced on the happy path where vrf_id matches.

No files require special attention.

Important Files Changed

Filename Overview
zebra/zebra_nhg.c Adds a VRF-ID guard in zebra_interface_nhg_reinstall() to skip NHGs whose nexthop belongs to a different VRF than the interface's current one; fix is minimal and correctly placed before the activation/install logic.
tests/topotests/zebra_rib/test_zebra_rib.py New topotest that exercises the exact scenario — VRF creation, static route installation, route removal, interface VRF migration — and asserts the stale NHE is kept-around but not reinstalled; cleanup in finally block is correct.

Sequence Diagram

sequenceDiagram
    participant K as Kernel
    participant Z as Zebra
    participant NHG as NHG Table

    Note over K,NHG: Interface migrates from VRF A → VRF B
    K->>Z: NETLINK: interface nomaster (leaves VRF A)
    Z->>Z: zebra_interface_nhg_reinstall(ifp)
    loop nhg_dependents walk
        Z->>NHG: "Check nhe->nhg.nexthop->vrf_id"
        alt "vrf_id != ifp->vrf->vrf_id (stale old-VRF NHG)"
            Z-->>Z: skip (new guard) ✓
        else "vrf_id == ifp->vrf->vrf_id (current VRF)"
            Z->>Z: SET NEXTHOP_FLAG_ACTIVE
            Z->>Z: zebra_nhg_set_valid_if_active()
            Z->>K: zebra_nhg_install_kernel()
        end
    end
Loading

Reviews (3): Last reviewed commit: "tests: verify stale NHGs stay inactive a..." | Re-trigger Greptile

Comment thread zebra/zebra_nhg.c
@donaldsharp

Copy link
Copy Markdown
Member

can you outline a sequence of events in a topotest that shows this problem to me? I would like to understand it a bit better.

@frrbot frrbot Bot added the tests Topotests, make check, etc label Jul 20, 2026
@github-actions github-actions Bot added size/L and removed size/XS labels Jul 20, 2026
@anlancs

anlancs commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Updated, please help reivew it. Thanks @donaldsharp

@anlancs

anlancs commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@Mergifyio rebase

@mergify

mergify Bot commented Jul 22, 2026

Copy link
Copy Markdown

rebase

❌ Unable to rebase: Mergify can't impersonate anlancs

Details

User anlancs used as bot_account is unknown. Please make sure anlancs exists and has logged into the Mergify dashboard.

@anlancs
anlancs force-pushed the fix/zebra-vrf branch 2 times, most recently from 6c3fa2e to 25a24d3 Compare July 23, 2026 13:57
@anlancs anlancs closed this Jul 23, 2026
@anlancs anlancs reopened this Jul 23, 2026
@anlancs

anlancs commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@donaldsharp
It is updated. Can you please take a look at it? Thanks!

@anlancs

anlancs commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

@donaldsharp can you take a look at it? any comment? thanks :-)

@riw777
riw777 self-requested a review August 31, 2026 11:33

@riw777 riw777 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good

When an interface migrates between VRFs, `zebra_interface_nhg_reinstall()`
iterates all NHGs in the interface's zif->nhg_dependents tree and reinstalls them.
However, the interface's nhg_dependents tree still contains singleton NHGs
created while the interface belonged to the previous VRF.
`zebra_interface_nhg_reinstall()` MAYBE blindly re-activate and reinstall
these stale NHGs.

Zebra keeps an installed or queued nexthop entry alive for a short period
after its last route reference is removed. When the interface moves to another
VRF at this short period, the down path clears the entry's VALID/INSTALLED state.
The following up path calls `zebra_interface_nhg_reinstall()`, which reinstalls
all entries still attached to that interface.

Just skip the NHGs whose nexthop belongs to a different VRF than the interface's
current VRF, which should remain inactive until their keep-around timer expires.

Signed-off-by: anlan_cs <anlan_cs@126.com>
The test records the route's nexthop-group ID. It then withdraws the route and
immediately moves the interface to the default VRF. At this point the old
nexthop entry should not be installed after the interface VRF change.

Signed-off-by: anlan_cs <anlan_cs@126.com>
@anlancs

anlancs commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Rebase it...

@riw777

riw777 commented Sep 19, 2026

Copy link
Copy Markdown
Member

Let's give this a try

@Mergifyio rebase

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

Labels

master size/L tests Topotests, make check, etc zebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants