bgpd: Retain the routes if we do a clear with N-bit set for Graceful-Restart - #18498
Merged
riw777 merged 2 commits intoMar 26, 2025
Merged
Conversation
…Restart On receiving side we already did the job correctly, but the peer which initiates the clear does not retain the other's routes. This commit fixes that. Fixes: 2017077 ("bgpd: Activate Graceful-Restart when receiving CEASE/HOLDTIME notifications") Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
…r GR Related-to: b7c657d ("bgpd: Retain the routes if we do a clear with N-bit set for Graceful-Restart") Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Member
Author
|
@Mergifyio backport stable/10.3 stable/10.2 stable/10.1 stable/10.0 |
✅ Backports have been createdDetails
|
Contributor
|
This indeed fixes the issue #18238. Manual testing with this fix included proved to work as expected and routes are retained on both peers (the one we clear the session on, and the one that receives the Cease/Administrative Reset message. Thanks! |
This was referenced Mar 26, 2025
riw777
added a commit
that referenced
this pull request
Apr 1, 2025
bgpd: Retain the routes if we do a clear with N-bit set for Graceful-Restart (backport #18498)
selva-nexthop
added a commit
to selva-nexthop/frr-comm
that referenced
this pull request
May 1, 2026
Problem: When a BGP peer sends a Hard Reset (Cease/subcode 9, per RFC 8538), the GR helper side must flush peer routes immediately rather than retaining them as stale. Four code gaps allow the NSF_WAIT flag to be set (or the hard_reset flag to be missed) after a Hard Reset, causing stale routes to remain in the RIB: 1. bgp_packet.c (bgp_notify_receive): peer->notify.hard_reset is only set when the received Hard Reset has a non-empty encapsulated payload (outer.length > 0). A valid bare Hard Reset (Cease/subcode 9 with no inner notification) leaves peer->notify.hard_reset = false. This bypasses the existing !hard_reset guard for the TCP error handler and doppelganger handler paths below. 2. bgpd.c (bgp_process_conn_error / TCP error handler): When the TCP socket reports an error or close, the handler unconditionally sets NSF_WAIT if NSF_MODE is set. If the peer sent a Hard Reset and the TCP stack delivered the RST before the NOTIFICATION was read from the socket, peer->notify.hard_reset is already true but is ignored here, causing GR helper mode to be entered and routes kept stale. 3. bgp_network.c (bgp_accept / doppelganger handler): Same issue -- when an incoming TCP collision tears down the existing established session, NSF_WAIT is set without checking peer->notify.hard_reset. A peer that sends Hard Reset and immediately reconnects can trigger this path before the NOTIFICATION event is processed by the FSM. 4. bgp_fsm.c (bgp_establish): peer->notify.hard_reset is NOT cleared when a new BGP session is established. After a Hard Reset the peer reconnects and establishes a new session. If that new session later drops normally (TCP drop, no notification), the TCP error handler sees the stale hard_reset=true from the previous session and skips NSF_WAIT, permanently defeating GR for that neighbor until a restart. Note: bgp_packet.c already has correct !hard_reset guards for the NOTIFICATION send path (~line 1058) and receive path (~line 2704). The bgp_packet.c fix here closes the gap that lets those guards be bypassed. FRR upstream master and stable/10.4 have the same gaps as of April 2026. The bgp_packet.c send/recv path fix corresponds to upstream PR FRRouting#18498 (merged to master 2026-03-26, not backported to stable/10.4). Solution: - bgp_packet.c: Set peer->notify.hard_reset = true whenever hard_reset is true, independently of whether outer.length > 0. - bgpd.c: Add !peer->notify.hard_reset check in the TCP error handler before setting NSF_WAIT. - bgp_network.c: Add !peer->notify.hard_reset check in the doppelganger handler before setting NSF_WAIT. - bgp_fsm.c: Clear peer->notify.hard_reset in bgp_establish() alongside PEER_STATUS_NSF_WAIT to prevent the flag from leaking across sessions. Topotest: - TC3: Add test_bgp_hard_reset_gr() -- Hard Reset with delayopen=60 (both sides). Exercises bgp_process_conn_error (bgpd.c fix). Verifies R1 does not retain stale routes after receiving a Hard Reset. - TC4: Add test_bgp_hard_reset_gr_doppelganger() -- Hard Reset with delayopen=5 on R1 only, no delayopen on R2. R2 reconnects immediately after Hard Reset, maximising the probability that R2's new TCP SYN reaches R1's listen socket while R1 is still in Established state, triggering the bgp_accept() doppelganger handler (bgp_network.c fix). R1's delayopen=5 holds the new session in OpenWait for ~5 s providing a stable observation window. Verifies routes are flushed in either code path (TCP error handler or doppelganger). Resolves: FRRouting#21822 Signed-off-by: selva <selva@nexthop.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #18238