bgpd: initialise nh_flag attribute by fdumontet6WIND · Pull Request #21498 · FRRouting/frr

fdumontet6WIND · 2026-04-10T15:34:20Z

in "leak_update" function
nh_flag is not initialized when nexthop is valid at route creation.

greptile-apps · 2026-04-10T15:38:54Z

Greptile Summary

This PR fixes a bug in leak_update() where BGP_ATTR_NH_VALID was never set in new->attr->nh_flags for newly-created leaked routes when the nexthop is valid. As part of the fix, the function is refactored to accept a non-interned static_attr instead of a pre-interned new_attr, moving all bgp_attr_intern() calls into leak_update() itself. The refactoring is internally consistent: the early-return "no change" path no longer calls bgp_attr_unintern, the update path interns at line 1354, and the new-route path interns at line 1447 — after setting the flag.

The code correctly sets SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID) while new->attr still points to the uninterned static_attr, then immediately replaces new->attr with the interned copy via bgp_attr_intern(static_attr). Since bgp_attr_intern hashes on all fields including nh_flags, the interned attr captures the flag correctly.

Confidence Score: 5/5

Safe to merge — the fix is correct and the refactoring is internally consistent with no logic regressions.

The bug fix correctly initialises BGP_ATTR_NH_VALID in nh_flags before the attr is interned. The accompanying refactoring (moving bgp_attr_intern into leak_update) is sound: the "no change" early return no longer needs bgp_attr_unintern, the update path interns before setting bpi->attr, the new-route path interns after setting the flag, and bgp_attr_flush in callers is correctly placed after leak_update returns. No P0 or P1 findings.

No files require special attention.

Important Files Changed

Filename	Overview
bgpd/bgp_mplsvpn.c	Refactors `leak_update()` to accept a non-interned attr, moves `bgp_attr_intern` calls inside the function, and adds the missing `SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID)` for the new-route creation path; logic is correct.

Sequence Diagram

sequenceDiagram
    participant Caller as vpn_leak_to_vrf_update_onevrf
    participant LU as leak_update()
    participant AI as bgp_attr_intern()
    participant NHV as leak_update_nexthop_valid()

    Caller->>LU: leak_update(..., &static_attr [uninterned])
    Note over LU: New-route path
    LU->>LU: info_make(..., static_attr, bn) / new->attr = static_attr (uninterned)
    LU->>NHV: leak_update_nexthop_valid(..., new, ...)
    NHV-->>LU: nh_valid = true
    LU->>LU: bgp_path_info_set_flag(BGP_PATH_VALID)
    LU->>LU: SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID)
    LU->>AI: bgp_attr_intern(static_attr)
    AI-->>LU: interned attr (nh_flags set)
    LU->>LU: new->attr = interned attr
    LU-->>Caller: new route
    Caller->>Caller: bgp_attr_flush(&static_attr)

_{Reviews (2): Last reviewed commit: "bgpd: initialise nh_flag attribute" | Re-trigger Greptile}

ton31337 · 2026-04-10T16:57:55Z

+				      source_bpi, new, bgp_orig, p, debug)) {
 		bgp_path_info_set_flag(bn, new, BGP_PATH_VALID);
-	else
+		SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID);


Is this BGP_ATTR_NH_VALID flag used anywhere at all in the code?

@greptileai is this nh_flags (BGP_ATTR_NH_VALID) used anywhere despite SET/UNSET?

Confirmed — BGP_ATTR_NH_VALID is not checked anywhere in the codebase. Searching the entire repo, it only appears in:

bgpd/bgp_attr.h — the #define

bgpd/bgp_mplsvpn.c — this PR's SET_FLAG call (and the corresponding UNSET_FLAG in the else branch)

There are zero CHECK_FLAG(attr->nh_flags, BGP_ATTR_NH_VALID) calls anywhere. The flag is defined, set, and unset — but never consumed. That makes this change a no-op from a behavioral standpoint, which raises the question of whether the fix is addressing the real bug, or whether BGP_ATTR_NH_VALID needs to actually be wired into nexthop resolution/route selection logic somewhere to have any meaningful effect.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules and .greptile/config.

This patch modifies `leak_update()` to accept a non-interned static attribute rather than requiring an already-interned one. Previously, callers such as `vpn_leak_to_vrf_update_onevrf()` and `_vpn_leak_from_vrf_update_leak_attr()` would intern the attribute prior to calling `leak_update()`. If `leak_update()` determined that the route was unchanged, it would simply unintern the attribute and continue, resulting in unnecessary memory allocation and hash table overhead. With this change, the caller passes a pointer to the static attribute, and `leak_update()` now defers calling `bgp_attr_intern()` until it actually needs to update an existing path or allocate a new one. The callers handle flushing the static attribute when done. Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>

this commit fixes an issue during new path creation: if the new leaked path is determined to have a valid nexthop, the `BGP_ATTR_NH_VALID` flag is now correctly set on the `nh_flags`. Fixes: 879bfc0 (bgpd: fix VRF leaking with 'network import-check') Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>

fdumontet6WIND · 2026-04-15T13:59:13Z

failure not linked with commit
ci:rerun

mjstapp · 2026-04-15T14:03:15Z

so you've now added a completely unrelated second commit - why?

pguibert6WIND · 2026-04-15T17:14:54Z

so you've now added a completely unrelated second commit - why?

preparatory commit.
changing attr requires uninterning and re-interning it.
this rework avoids doing this, and will do an intern a single time.

donaldsharp · 2026-04-16T20:09:46Z

@greptile review

ton31337

Seems BGP_ATTR_NH_VALID flag is not used at all (despite SET/UNSET);
90c0b89 is not aimed for this PR?

fdumontet6WIND · 2026-04-20T08:52:33Z

in fact BGP_ATTR_NH_VALID flag is tested in "attrhash_cmp" function.
old value is tested against the new one to detect change

fdumontet6WIND · 2026-04-20T09:26:23Z

90c0b89
is to allowing change of nh_flags (with BGP_ATTR_NH_VALID) ,that can't be changed when it is unterned.

frrbot bot added the bgp label Apr 10, 2026

github-actions bot added master size/XS rebase PR needs rebase labels Apr 10, 2026

greptile-apps bot reviewed Apr 10, 2026

View reviewed changes

Comment thread bgpd/bgp_mplsvpn.c Outdated

fdumontet6WIND force-pushed the init_attr branch 2 times, most recently from c2cf998 to dd5baf1 Compare April 10, 2026 15:48

ton31337 reviewed Apr 10, 2026

View reviewed changes

mwinter-osr mentioned this pull request Apr 14, 2026

greptile: Add Greptile configuration for automated code review on PRs. #21482

Open

github-actions bot added size/M and removed size/XS labels Apr 14, 2026

fdumontet6WIND force-pushed the init_attr branch 2 times, most recently from b55ece2 to ab122b2 Compare April 15, 2026 09:00

frrbot bot added the bugfix label Apr 15, 2026

fdumontet6WIND added 2 commits April 15, 2026 14:06

fdumontet6WIND force-pushed the init_attr branch from ab122b2 to fd8e0ca Compare April 15, 2026 12:07

fdumontet6WIND requested a review from ton31337 April 17, 2026 08:30

ton31337 reviewed Apr 20, 2026

View reviewed changes

fdumontet6WIND requested a review from ton31337 April 20, 2026 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bgpd: initialise nh_flag attribute#21498

bgpd: initialise nh_flag attribute#21498
fdumontet6WIND wants to merge 2 commits intoFRRouting:masterfrom
fdumontet6WIND:init_attr

fdumontet6WIND commented Apr 10, 2026

Uh oh!

greptile-apps bot commented Apr 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

ton31337 Apr 10, 2026

Uh oh!

ton31337 Apr 20, 2026

Uh oh!

greptile-apps bot Apr 20, 2026

Uh oh!

fdumontet6WIND commented Apr 15, 2026

Uh oh!

mjstapp commented Apr 15, 2026

Uh oh!

pguibert6WIND commented Apr 15, 2026

Uh oh!

donaldsharp commented Apr 16, 2026

Uh oh!

ton31337 left a comment

Uh oh!

fdumontet6WIND commented Apr 20, 2026

Uh oh!

fdumontet6WIND commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

fdumontet6WIND commented Apr 10, 2026

Uh oh!

greptile-apps bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

ton31337 Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

ton31337 Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

fdumontet6WIND commented Apr 15, 2026

Uh oh!

mjstapp commented Apr 15, 2026

Uh oh!

pguibert6WIND commented Apr 15, 2026

Uh oh!

donaldsharp commented Apr 16, 2026

Uh oh!

ton31337 left a comment

Choose a reason for hiding this comment

Uh oh!

fdumontet6WIND commented Apr 20, 2026

Uh oh!

fdumontet6WIND commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

greptile-apps bot commented Apr 10, 2026 •

edited

Loading