bgpd: don't advertise LLGR stale routes to non-LLGR peers

inder-nexthop · inder-nexthop · commit 9cd7d5962270 · 2026-06-09T20:02:09.000Z
When a BGP peer goes down, an LLGR helper keeps the routes it learned from
that peer, tags them with the LLGR_STALE community, and holds them as a
last resort. RFC 9494 (4.3) says a stale route like that shouldn't be
advertised to any neighbor that hasn't advertised the LLGR capability to
us - a speaker that doesn't implement LLGR has no notion of staleness and
would treat the route as a normal, live path.

The advertise check only withheld a stale route when LLGR had been
negotiated in neither direction (!RCV &amp;&amp; !ADV), but since we advertise the
LLGR capability to every peer, ADV is effectively always set and the route
went out anyway. The capability that matters here is the neighbor's, so key
the decision off whether they advertised it to us (RCV) - which is what the
comment sitting right above that check already says we do.

That alone isn't enough: the advertise decision is made once per
update-subgroup, and an LLGR peer and a non-LLGR peer with the same outbound
policy land in the same subgroup, so there's no way to send the route to one
but not the other. Add PEER_CAP_LLGR_RCV to PEER_UPDGRP_CAP_FLAGS so the
received capability feeds both the update-group hash key and the cmp
function - keying off the same axis the advertise check uses - so a non-LLGR
peer gets its own update-group and is never merged back in on a hash
collision.

The new bgp_llgr_no_capability topotest covers it: kill the peer that
advertised the route, then confirm r2 keeps feeding the stale route to its
LLGR-capable peer r3 while withdrawing it from the non-LLGR peer r4. Worth
noting in the config - FRR advertises LLGR whenever it advertises graceful
restart, and the default helper mode counts too, so r4 has to set
"bgp graceful-restart-disable" to actually be a non-LLGR peer.

Signed-off-by: Inder Pooni &lt;inder@nexthop.ai&gt;
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
@@ -2957,8 +2957,7 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi,
 	if (bgp_attr_get_community(attr) &&
 	    community_include(bgp_attr_get_community(attr),
 			      COMMUNITY_LLGR_STALE) &&
-	    !CHECK_FLAG(peer->cap, PEER_CAP_LLGR_RCV) &&
-	    !CHECK_FLAG(peer->cap, PEER_CAP_LLGR_ADV))
+	    !CHECK_FLAG(peer->cap, PEER_CAP_LLGR_RCV))
 		return false;
 
 	/* Perform operations that involve memory allocation.
diff --git a/bgpd/bgp_updgrp.h b/bgpd/bgp_updgrp.h
@@ -52,7 +52,14 @@
 	 PEER_FLAG_CONFIG_ENCAPSULATION_SRV6 | PEER_FLAG_CONFIG_ENCAPSULATION_SRV6_RELAX |        \
 	 PEER_FLAG_CONFIG_ENCAPSULATION_MPLS)
 
-#define PEER_UPDGRP_CAP_FLAGS (PEER_CAP_AS4_RCV)
+/*
+ * PEER_CAP_LLGR_RCV is included so peers that differ in the received Long-Lived
+ * Graceful Restart capability land in separate update groups: an LLGR_STALE-tagged
+ * route is advertised only to peers from which LLGR was received (RFC 9494 Section
+ * 4.3), and that per-subgroup decision must not merge an LLGR and a non-LLGR peer.
+ * Folding it into this mask covers both the hash key and the cmp function.
+ */
+#define PEER_UPDGRP_CAP_FLAGS (PEER_CAP_AS4_RCV | PEER_CAP_LLGR_RCV)
 
 #define PEER_UPDGRP_AF_CAP_FLAGS                                               \
 	(PEER_CAP_ORF_PREFIX_SM_RCV | PEER_CAP_ADDPATH_AF_TX_ADV |             \
diff --git a/tests/topotests/bgp_llgr_no_capability/__init__.py b/tests/topotests/bgp_llgr_no_capability/__init__.py
diff --git a/tests/topotests/bgp_llgr_no_capability/r1/bgpd.conf b/tests/topotests/bgp_llgr_no_capability/r1/bgpd.conf
@@ -0,0 +1,12 @@
+router bgp 65001
+ no bgp ebgp-requires-policy
+ bgp graceful-restart
+ bgp graceful-restart restart-time 0
+ bgp long-lived-graceful-restart stale-time 600
+ neighbor 192.168.1.2 remote-as external
+ neighbor 192.168.1.2 timers 3 10
+ neighbor 192.168.1.2 timers connect 1
+ address-family ipv4 unicast
+  redistribute connected
+ exit-address-family
+!
diff --git a/tests/topotests/bgp_llgr_no_capability/r1/zebra.conf b/tests/topotests/bgp_llgr_no_capability/r1/zebra.conf
@@ -0,0 +1,7 @@
+!
+int lo
+ ip address 172.16.1.1/32
+!
+int r1-eth0
+ ip address 192.168.1.1/24
+!
diff --git a/tests/topotests/bgp_llgr_no_capability/r2/bgpd.conf b/tests/topotests/bgp_llgr_no_capability/r2/bgpd.conf
@@ -0,0 +1,14 @@
+router bgp 65002
+ no bgp ebgp-requires-policy
+ bgp graceful-restart
+ bgp long-lived-graceful-restart stale-time 600
+ neighbor 192.168.1.1 remote-as external
+ neighbor 192.168.1.1 timers 3 10
+ neighbor 192.168.1.1 timers connect 1
+ neighbor 192.168.2.2 remote-as external
+ neighbor 192.168.2.2 timers 3 10
+ neighbor 192.168.2.2 timers connect 1
+ neighbor 192.168.3.2 remote-as external
+ neighbor 192.168.3.2 timers 3 10
+ neighbor 192.168.3.2 timers connect 1
+!
diff --git a/tests/topotests/bgp_llgr_no_capability/r2/zebra.conf b/tests/topotests/bgp_llgr_no_capability/r2/zebra.conf
@@ -0,0 +1,10 @@
+!
+int r2-eth0
+ ip address 192.168.1.2/24
+!
+int r2-eth1
+ ip address 192.168.2.1/24
+!
+int r2-eth2
+ ip address 192.168.3.1/24
+!
diff --git a/tests/topotests/bgp_llgr_no_capability/r3/bgpd.conf b/tests/topotests/bgp_llgr_no_capability/r3/bgpd.conf
@@ -0,0 +1,8 @@
+router bgp 65003
+ no bgp ebgp-requires-policy
+ bgp graceful-restart
+ bgp long-lived-graceful-restart stale-time 600
+ neighbor 192.168.2.1 remote-as external
+ neighbor 192.168.2.1 timers 3 10
+ neighbor 192.168.2.1 timers connect 1
+!
diff --git a/tests/topotests/bgp_llgr_no_capability/r3/zebra.conf b/tests/topotests/bgp_llgr_no_capability/r3/zebra.conf
@@ -0,0 +1,4 @@
+!
+int r3-eth0
+ ip address 192.168.2.2/24
+!
diff --git a/tests/topotests/bgp_llgr_no_capability/r4/bgpd.conf b/tests/topotests/bgp_llgr_no_capability/r4/bgpd.conf
@@ -0,0 +1,10 @@
+router bgp 65004
+ no bgp ebgp-requires-policy
+! FRR advertises Graceful Restart (and therefore the LLGR capability) by
+! default in helper mode. Disable GR entirely so r4 is a genuine non-LLGR
+! peer: it advertises neither the GR nor the LLGR capability to r2.
+ bgp graceful-restart-disable
+ neighbor 192.168.3.1 remote-as external
+ neighbor 192.168.3.1 timers 3 10
+ neighbor 192.168.3.1 timers connect 1
+!
diff --git a/tests/topotests/bgp_llgr_no_capability/r4/zebra.conf b/tests/topotests/bgp_llgr_no_capability/r4/zebra.conf
@@ -0,0 +1,4 @@
+!
+int r4-eth0
+ ip address 192.168.3.2/24
+!
diff --git a/tests/topotests/bgp_llgr_no_capability/test_bgp_llgr_no_capability.py b/tests/topotests/bgp_llgr_no_capability/test_bgp_llgr_no_capability.py
@@ -0,0 +1,192 @@
+#!/usr/bin/env python
+# SPDX-License-Identifier: ISC
+
+"""
+Test LLGR helper behavior toward peers that did not advertise the
+LLGR capability (RFC 9494 sec. 4.3 / sec. 6):
+
+A receiver of an LLGR_STALE-tagged route SHOULD NOT re-advertise it to a
+BGP peer that has not advertised the LLGR capability for the same AFI/SAFI.
+
+Topology:
+
+    r1 ----- r2 ----- r3   r3: LLGR-capable
+              |
+              +------ r4   r4: NO LLGR capability (no graceful-restart;
+                               in FRR, GR advertises LLGR too)
+
+  - r1 originates 172.16.1.1/32 and gets killed mid-test.
+  - r2 is the LLGR helper for r1.
+  - After r1 dies, r2 must:
+      * mark 172.16.1.1/32 as stale with the llgr-stale community,
+      * keep advertising it to r3 (LLGR-capable peer),
+      * withdraw it from r4 (non-LLGR peer).
+"""
+
+import os
+import sys
+import json
+import functools
+import pytest
+
+pytestmark = [pytest.mark.bgpd]
+
+CWD = os.path.dirname(os.path.realpath(__file__))
+sys.path.append(os.path.join(CWD, "../"))
+
+# pylint: disable=C0413
+from lib import topotest
+from lib.topogen import Topogen, TopoRouter, get_topogen
+from lib.common_config import kill_router_daemons, step
+
+
+PREFIX = "172.16.1.1/32"
+
+
+def build_topo(tgen):
+    for router_id in range(1, 5):
+        tgen.add_router(f"r{router_id}")
+
+    s1 = tgen.add_switch("s1")
+    s1.add_link(tgen.gears["r1"])
+    s1.add_link(tgen.gears["r2"])
+
+    s2 = tgen.add_switch("s2")
+    s2.add_link(tgen.gears["r2"])
+    s2.add_link(tgen.gears["r3"])
+
+    s3 = tgen.add_switch("s3")
+    s3.add_link(tgen.gears["r2"])
+    s3.add_link(tgen.gears["r4"])
+
+
+def setup_module(mod):
+    tgen = Topogen(build_topo, mod.__name__)
+    tgen.start_topology()
+
+    for rname, router in tgen.routers().items():
+        router.load_config(
+            TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname))
+        )
+        router.load_config(
+            TopoRouter.RD_BGP, os.path.join(CWD, "{}/bgpd.conf".format(rname))
+        )
+
+    tgen.start_router()
+
+
+def teardown_module(mod):
+    get_topogen().stop_topology()
+
+
+def _route_present(router, prefix):
+    output = json.loads(router.vtysh_cmd("show ip bgp json"))
+    if prefix not in output.get("routes", {}):
+        return "{} missing on {}".format(prefix, router.name)
+    return None
+
+
+def _route_absent(router, prefix):
+    output = json.loads(router.vtysh_cmd("show ip bgp json"))
+    if prefix in output.get("routes", {}):
+        return "{} still present on {}".format(prefix, router.name)
+    return None
+
+
+def _route_stale_with_llgr_stale(router, prefix):
+    output = json.loads(router.vtysh_cmd("show ip bgp {} json".format(prefix)))
+    # FRR sets BGP_PATH_STALE on LLGR-stale paths (surfacing as "stale": true), so
+    # checking both the llgr-stale community and the stale flag is intentional: if a
+    # future FRR split BGP_PATH_LLGR_STALE from BGP_PATH_STALE, this would catch it
+    # rather than silently passing on the community alone.
+    expected = {"paths": [{"community": {"string": "llgr-stale"}, "stale": True}]}
+    return topotest.json_cmp(output, expected)
+
+
+def _route_with_llgr_stale_community(router, prefix):
+    output = json.loads(router.vtysh_cmd("show ip bgp {} json".format(prefix)))
+    paths = output.get("paths") or []
+    if not paths:
+        return "{} absent on {}".format(prefix, router.name)
+    for p in paths:
+        community_string = (p.get("community") or {}).get("string", "")
+        if "llgr-stale" in community_string:
+            return None
+    return "{} present on {} but llgr-stale community missing".format(
+        prefix, router.name
+    )
+
+
+def _not_advertised_to(helper, peer_ip, prefix):
+    output = json.loads(
+        helper.vtysh_cmd(
+            "show ip bgp neighbor {} advertised-routes json".format(peer_ip)
+        )
+    )
+    advertised = output.get("advertisedRoutes") or {}
+    if prefix in advertised:
+        return "{} still advertised by {} to {}".format(prefix, helper.name, peer_ip)
+    return None
+
+
+def _advertised_to(helper, peer_ip, prefix):
+    output = json.loads(
+        helper.vtysh_cmd(
+            "show ip bgp neighbor {} advertised-routes json".format(peer_ip)
+        )
+    )
+    advertised = output.get("advertisedRoutes") or {}
+    if prefix not in advertised:
+        return "{} no longer advertised by {} to {}".format(
+            prefix, helper.name, peer_ip
+        )
+    return None
+
+
+def test_bgp_llgr_no_capability_withdraw():
+    tgen = get_topogen()
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    r2 = tgen.gears["r2"]
+    r3 = tgen.gears["r3"]
+    r4 = tgen.gears["r4"]
+
+    step("Initial convergence: r3 and r4 both learn {}".format(PREFIX))
+    for r in (r3, r4):
+        test_func = functools.partial(_route_present, r, PREFIX)
+        _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+        assert result is None, result
+
+    step("Kill bgpd on r1 to put r2 into LLGR helper mode")
+    kill_router_daemons(tgen, "r1", ["bgpd"])
+
+    step("r2 marks {} stale with llgr-stale community".format(PREFIX))
+    test_func = functools.partial(_route_stale_with_llgr_stale, r2, PREFIX)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    assert result is None, "r2 did not mark {} as stale/llgr-stale".format(PREFIX)
+
+    step("r3 (LLGR-capable peer) still holds {} with llgr-stale community".format(PREFIX))
+    test_func = functools.partial(_route_with_llgr_stale_community, r3, PREFIX)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    assert result is None, result
+
+    step("r2 keeps advertising {} to r3 (LLGR-capable peer)".format(PREFIX))
+    test_func = functools.partial(_advertised_to, r2, "192.168.2.2", PREFIX)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    assert result is None, result
+
+    step("r4 (non-LLGR peer) no longer has {} (must withdraw fast, not wait for stale-time)".format(PREFIX))
+    test_func = functools.partial(_route_absent, r4, PREFIX)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    assert result is None, result
+
+    step("r2 stops advertising {} to r4 (non-LLGR peer)".format(PREFIX))
+    test_func = functools.partial(_not_advertised_to, r2, "192.168.3.2", PREFIX)
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    assert result is None, result
+
+
+if __name__ == "__main__":
+    args = ["-s"] + sys.argv[1:]
+    sys.exit(pytest.main(args))

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +!
 +int r3-eth0
 + ip address 192.168.2.2/24
 +!