Skip to content

ospfd,tests: fix OSPF connected overlapping prefix bug#21510

Open
rzalamena wants to merge 2 commits intoFRRouting:masterfrom
opensourcerouting:ospf-overlap-connected
Open

ospfd,tests: fix OSPF connected overlapping prefix bug#21510
rzalamena wants to merge 2 commits intoFRRouting:masterfrom
opensourcerouting:ospf-overlap-connected

Conversation

@rzalamena
Copy link
Copy Markdown
Member

When two distinct interfaces have overlapping prefixes (example: 10.0.0.1/24 and 10.0.0.129/30) the more specific prefix gets omitted due to the wrong comparison in ospf_distribute_check_connected: if (prefix_match(oi->address, (struct prefix *)&ei->p)).

Fix the ospf_distribute_check_connected to use prefix_same() in order to rightfully generate the external route for this case.

@frrbot frrbot bot added bugfix ospf tests Topotests, make check, etc labels Apr 13, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 13, 2026

Greptile Summary

Fixes the OSPF connected-route suppression logic in ospf_distribute_check_connected so that a more-specific prefix (e.g. 10.0.0.128/30) on a non-OSPF interface is no longer wrongly suppressed when a broader OSPF-enabled interface (10.0.0.1/24) is present. The old prefix_match performed a containment check (is /30 inside the /24?), which returned true and suppressed redistribution; the new code masks the interface address with apply_mask before calling prefix_same, which requires an exact network and prefix-length match. A new topotest reproduces the bug with r1 advertising both a /24 OSPF intra-area route and a /30 redistributed external route, and verifies the correct behavior on r2.

Confidence Score: 5/5

Safe to merge; the fix is correct and the test suite covers both the regression and the suppression guard

The apply_mask + prefix_same pattern correctly replaces prefix_match by zeroing host bits before the exact-match comparison, fixing the root cause without introducing new regressions. The topotest validates the positive case (external /30) and the suppression case (intra-area /24). No P0 or P1 issues remain.

No files require special attention

Important Files Changed

Filename Overview
ospfd/ospf_zebra.c Replaces prefix_match with apply_mask + prefix_same to fix incorrect suppression of more-specific overlapping connected prefixes; the approach is correct and safe
tests/topotests/ospf_connected_overlapping_prefix/test_ospf_connected_overlapping_prefix.py New topotest covering the bug scenario; tests neighbor convergence, external advertisement of the /30, and intra-area presence of the /24; topology dict "s2": "r1" is handled correctly by Topogen's is_string guard
tests/topotests/ospf_connected_overlapping_prefix/r1/frr.conf r1 config: eth0 in OSPF area 0 (/24), eth1 not in OSPF (/30 overlapping), redistribute connected — correctly models the bug scenario
tests/topotests/ospf_connected_overlapping_prefix/r2/frr.conf r2 config: eth0 in OSPF area 0 (/24), acts as the observer verifying that the /30 external LSA and /24 intra-area route are received correctly

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["ospf_distribute_check_connected(ospf, ei)"] --> B["for each oi in ospf->oiflist"]
    B --> C["prefix_copy(&address, oi->address)\napply_mask(&address)\n(zeros host bits: 10.0.0.1/24 → 10.0.0.0/24)"]
    C --> D{"prefix_same(address, ei->p)?"}
    D -- "Yes\n(exact network match)\ne.g. 10.0.0.0/24 == 10.0.0.0/24" --> E["return 0\n(suppress redistribution)"]
    D -- "No\n(different network or prefix len)\ne.g. 10.0.0.0/24 ≠ 10.0.0.128/30" --> F["next oi"]
    F --> B
    B -- "exhausted" --> G["return 1\n(allow redistribution)"]

    style E fill:#f99,color:#000
    style G fill:#9f9,color:#000
Loading

Reviews (3): Last reviewed commit: "ospfd: fix redistribution for overlappin..." | Re-trigger Greptile

@rzalamena rzalamena force-pushed the ospf-overlap-connected branch from 19186b6 to 8f7b6c8 Compare April 13, 2026 21:04
@rzalamena
Copy link
Copy Markdown
Member Author

@greptileai

@rzalamena rzalamena force-pushed the ospf-overlap-connected branch from 8f7b6c8 to 56a9ead Compare April 13, 2026 23:38
@rzalamena
Copy link
Copy Markdown
Member Author

@greptileai

Add test for the OSPF connected overlapping prefix bug where an external
route is ommited because a connected route overlaps the prefix.

Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
OSPF should not originate AS-external LSAs for networks that are
already advertised internally (i.e. via OSPF-enabled interfaces).

The redistribution check for connected routes used `prefix_match()`,
which incorrectly suppressed routes whose prefixes only overlap with
an OSPF-enabled interface.

Use `prefix_same()` instead, so only identical prefixes are skipped
and distinct connected networks are correctly redistributed.

Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
@rzalamena rzalamena force-pushed the ospf-overlap-connected branch from 56a9ead to 57b1005 Compare April 14, 2026 14:54
@riw777 riw777 self-requested a review April 14, 2026 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants