Skip to content

Sougatab/bfd adminshut periodic tx#22643

Open
sougatahitcs wants to merge 2 commits into
FRRouting:masterfrom
sougatahitcs:sougatab/bfd-adminshut-periodic-tx
Open

Sougatab/bfd adminshut periodic tx#22643
sougatahitcs wants to merge 2 commits into
FRRouting:masterfrom
sougatahitcs:sougatab/bfd-adminshut-periodic-tx

Conversation

@sougatahitcs

@sougatahitcs sougatahitcs commented Jul 13, 2026

Copy link
Copy Markdown
Contributor
bfdd: keep transmitting AdminDown after session/profile shutdown

Problem
-------
When a BFD session (or a whole BFD profile) is administratively shut
down, bfd_set_shutdown() moved the session to AdminDown, deleted the
transmit timer, and sent exactly ONE Control packet carrying
State=AdminDown. After that the session went silent.

If that single AdminDown packet is dropped, or simply loses the race
with the remote's detection timer (detect_mult * negotiated interval),
the peer never learns we went AdminDown. Instead it expires the session
with diag "Control Detection Time Expired", declares the session Down,
and tears down whatever protocol BFD protects. For BGP this means the
peer sends a Cease/BFD Down NOTIFICATION and our session resets.

This is nondeterministic and gets worse at scale and with short timers:
shutting a profile fans out an AdminDown burst across many sessions in a
few milliseconds, so a handful reliably lose the race. Observed on a
304-session box: 6-9 BGP sessions reset per profile shut/no-shut with
50ms/mult-3 (150ms detection) timers.

RFC recommendation
------------------
RFC 5880 Section 6.8.16 (Administrative Control):

  "BFD Control packets SHOULD be transmitted for at least a Detection
   Time after transitioning to AdminDown state in order to ensure that
   the remote system is aware of the state change. BFD Control packets
   MAY be transmitted indefinitely after transitioning to AdminDown
   state in order to maintain session state in each system."

Sending a single packet transmits for ~0 time, violating this SHOULD.

Solution
--------
Do not delete the transmit timer on shutdown. Keep advertising
State=AdminDown, but slow the transmit rate to BFD_DEF_SLOWTX (1s) since
the session is no longer Up (RFC 5880 Section 6.8.3), mirroring the
existing ptm_bfd_sess_dn() DOWN handling. Echo and the receive/detection
timers are still disabled - we only need to keep informing the peer, not
detect it, while administratively down.

The remote now reliably receives AdminDown (diag "Neighbor signaled
session down", RFC 5880 Section 6.8.6) and holds the protocol session up
instead of timing it out. Cost is 1 packet/s per session while shut.

For offloaded (data-plane) sessions the same guarantee must be enforced
in the data-plane program (keep emitting AdminDown); a comment marks
that path.

Signed-off-by: Sougata Barik <sougatab@nvidia.com>

Problem
-------
When a BFD session (or a whole BFD profile) is administratively shut
down, bfd_set_shutdown() moved the session to AdminDown, deleted the
transmit timer, and sent exactly ONE Control packet carrying
State=AdminDown. After that the session went silent.

If that single AdminDown packet is dropped, or simply loses the race
with the remote's detection timer (detect_mult * negotiated interval),
the peer never learns we went AdminDown. Instead it expires the session
with diag "Control Detection Time Expired", declares the session Down,
and tears down whatever protocol BFD protects. For BGP this means the
peer sends a Cease/BFD Down NOTIFICATION and our session resets.

This is nondeterministic and gets worse at scale and with short timers:
shutting a profile fans out an AdminDown burst across many sessions in a
few milliseconds, so a handful reliably lose the race. Observed on a
304-session box: 6-9 BGP sessions reset per profile shut/no-shut with
50ms/mult-3 (150ms detection) timers.

RFC recommendation
------------------
RFC 5880 Section 6.8.16 (Administrative Control):

  "BFD Control packets SHOULD be transmitted for at least a Detection
   Time after transitioning to AdminDown state in order to ensure that
   the remote system is aware of the state change. BFD Control packets
   MAY be transmitted indefinitely after transitioning to AdminDown
   state in order to maintain session state in each system."

Sending a single packet transmits for ~0 time, violating this SHOULD.

Solution
--------
Do not delete the transmit timer on shutdown. Keep advertising
State=AdminDown, but slow the transmit rate to BFD_DEF_SLOWTX (1s) since
the session is no longer Up (RFC 5880 Section 6.8.3), mirroring the
existing ptm_bfd_sess_dn() DOWN handling. Echo and the receive/detection
timers are still disabled - we only need to keep informing the peer, not
detect it, while administratively down.

The remote now reliably receives AdminDown (diag "Neighbor signaled
session down", RFC 5880 Section 6.8.6) and holds the protocol session up
instead of timing it out. Cost is 1 packet/s per session while shut.

For offloaded (data-plane) sessions the same guarantee must be enforced
in the data-plane program (keep emitting AdminDown); a comment marks
that path.

Signed-off-by: Sougata Barik <sougatab@nvidia.com>
Add a topotest that guards the BFD administrative-shutdown transmit
behaviour fixed in the preceding commit.

Problem it reproduces
---------------------
Before the fix, bfd_set_shutdown() deleted the transmit timer and sent a
single Control packet with State=AdminDown, then went silent. A lost
AdminDown (or one that lost the race with the peer's detection timer)
left the peer to time the session out and tear down BGP with a Cease/BFD
Down NOTIFICATION.

RFC 5880 Section 6.8.16 says AdminDown "SHOULD be transmitted for at
least a Detection Time" so the remote reliably learns of the state
change.

What the test checks
--------------------
Topology r1 --- r2 with eBGP + BFD (300ms/mult-3).

 * test_reproduce_send_once_regression: after admin-shutdown on r1,
   capture outbound BFD with tcpdump and require MORE THAN ONE AdminDown
   Control packet. Against the old send-once code exactly one (or zero)
   is seen, so this fails and reproduces the bug.

 * test_fix_periodic_admindown_and_bgp_stable: end-to-end - r2 observes
   the remote AdminDown, r1 keeps re-transmitting AdminDown while shut,
   BGP stays Established on both routers, connectionsDropped is unchanged
   across the shut/no-shut cycle, and the session recovers.

Signed-off-by: Sougata Barik <sougatab@nvidia.com>
@frrbot frrbot Bot added bfd tests Topotests, make check, etc labels Jul 13, 2026
@sougatahitcs
sougatahitcs marked this pull request as draft July 13, 2026 16:14
@greptile-apps

greptile-apps Bot commented Jul 13, 2026

Copy link
Copy Markdown

Greptile Summary

This PR changes BFD administrative shutdown to keep advertising AdminDown. The main changes are:

  • Keeps non-offloaded sessions sending AdminDown at the slow transmit interval.
  • Leaves receive and echo timers disabled while administratively down.
  • Adds transmit timer scheduling trace coverage.
  • Adds topotests for periodic AdminDown transmission and BGP stability.

Confidence Score: 4/5

This is close, but the offloaded shutdown path should be fixed before merging.

  • The non-offloaded timer path now matches the intended AdminDown behavior.
  • The offloaded branch still returns after a single dataplane update.
  • Offloaded sessions can keep the same lost AdminDown packet failure unless the dataplane is explicitly programmed to retransmit.

bfdd/bfd.c

Important Files Changed

Filename Overview
bfdd/bfd.c Adds periodic AdminDown transmission for non-offloaded sessions, while the offloaded branch still returns after one dataplane update.
bfdd/event.c Allows the transmit timer to run during AdminDown while keeping inactive sessions unscheduled.
bfdd/bfd_trace.h Adds an xmttimer_schedule tracepoint for transmit timer scheduling.
tests/topotests/bfd_adminshut_periodic_tx/test_bfd_adminshut_periodic_tx.py Adds tests for periodic AdminDown transmission and BGP stability during shut/no-shut.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
bfdd/bfd.c:1795-1800
**Offload still exits early**

When `bs->bdc` is set, this branch still sends one dataplane update and returns before the new slow AdminDown transmit path is reached. That leaves the offloaded case dependent on an external dataplane behavior that this code does not program or verify. If the dataplane only reacts to `SESSION_SHUTDOWN` once, the peer can still miss the AdminDown packet, expire detection, and reset the protected protocol. Please make this branch explicitly program periodic AdminDown transmission for offloaded sessions, or route it through an equivalent periodic sender.

Reviews (2): Last reviewed commit: "tests: bfdd: topotest for periodic Admin..." | Re-trigger Greptile

Comment thread bfdd/bfd.c
@sougatahitcs
sougatahitcs force-pushed the sougatab/bfd-adminshut-periodic-tx branch from c8741cc to d54fd6b Compare July 13, 2026 16:18
@sougatahitcs
sougatahitcs marked this pull request as ready for review July 13, 2026 16:29
Comment thread bfdd/bfd.c
@sougatahitcs

Copy link
Copy Markdown
Contributor Author

CI:rerun

@riw777

riw777 commented Jul 15, 2026

Copy link
Copy Markdown
Member

Can you take your username out of the title and make it a little more descriptive?

@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.

Should there also be a new state in YANG and show commands to tell the operator the session is shut down but still sending admin shut packets? Or is this state too short to be worried about it?

hostname r1
!
interface lo
ip address 1.1.1.1/32

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.

Please use documentation addresses rather than publicly routeable.

@riw777

riw777 commented Jul 15, 2026

Copy link
Copy Markdown
Member

Will we continue sending the admin down notifications "permanently" with this fix? Should we stop after detection time?

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants