BFD timer negotiation is non-existent

[taspelund]

While working on adding juniper to falcon-lab, I noticed that the BFD tests were consistently due to the incorrect next-hops in the FIB after stopping FRR and EOS peers (meaning juniper should be the only active next-hop). On closer examination, the failures are inconsistent and show 1, 2 or 3 next-hops depending on the timing in which the dpd query occurs.

The inconsistency in the output is actually a result of the BFD session between mgd and juniper flapping indefinitely, and the contents of the dpd query change depending on what point of convergence we happen to get a result back during (when all next-hops for a route are bfd-shutdown, they all become valid again).

This looks to be happening due to mgd not negotiating BFD timers with the peer.
While Juniper cRPD is configured with matching timers (300ms tx/rx * 3 detect multiplier), there seems to be a bug where the config is ignored and default transmit/receive values of 2s are applied:

root@crpd1> show configuration routing-options
rib inet6.0 {
    static {
        route 3ffe::/64 {
            next-hop fd00:3::1;
            bfd-liveness-detection {
                minimum-interval 300;     <<<<<
                multiplier 3;             <<<<<
            }
        }
    }
}
static {
    route 100.65.0.0/24 {
        next-hop 10.0.2.1;
        bfd-liveness-detection {
            minimum-interval 300;     <<<<<
            multiplier 3;             <<<<<
        }
    }
}
root@crpd1> show bfd session
                                                  Detect   Transmit
Address                  State     Interface      Time     Interval  Multiplier
10.0.2.1                 Up        eth1           6.000     2.000        3     <<<<<   2s interval * 3 (multiplier) = 6s time
fd00:3::1                Up        eth1           6.000     2.000        3     <<<<<   2s interval * 3 (multiplier) = 6s time

2 sessions, 2 clients
Cumulative transmit rate 1.0 pps, cumulative receive rate 1.0 pps

When this mismatch is happening (mgd using 300ms * 3, junos using 2000ms * 3), an indefinite cycle between Up/Down states occurs as mgd marks the peer session down at 900ms instead of the 6000ms it should have calculated.

RFC 5880 Section 6.8.4 defines detection time calculation:

6.8.4.  Calculating the Detection Time

   The Detection Time (the period of time without receiving BFD packets
   after which the session is determined to have failed) is not carried
   explicitly in the protocol.  Rather, it is calculated independently
   in each direction by the receiving system based on the negotiated
   transmit interval and the detection multiplier.  Note that there may
   be different Detection Times in each direction.

   The calculation of the Detection Time is slightly different when in
   Demand mode versus Asynchronous mode.

   In Asynchronous mode, the Detection Time calculated in the local          <<<<<
   system is equal to the value of Detect Mult received from the remote      <<<<<
   system, multiplied by the agreed transmit interval of the remote          <<<<<
   system (the greater of bfd.RequiredMinRxInterval and the last             <<<<<
   received Desired Min TX Interval).  The Detect Mult value is (roughly     <<<<<
   speaking, due to jitter) the number of packets that have to be missed     <<<<<
   in a row to declare the session to be down.                               <<<<<

   If Demand mode is not active, and a period of time equal to the
   Detection Time passes without receiving a BFD Control packet from the
   remote system, and bfd.SessionState is Init or Up, the session has
   gone down -- the local system MUST set bfd.SessionState to Down and
   bfd.LocalDiag to 1 (Control Detection Time Expired).

   In Demand mode, the Detection Time calculated in the local system is
   equal to bfd.DetectMult, multiplied by the agreed transmit interval
   of the local system (the greater of bfd.DesiredMinTxInterval and
   bfd.RemoteMinRxInterval).  bfd.DetectMult is (roughly speaking, due
   to jitter) the number of packets that have to be missed in a row to
   declare the session to be down.

   If Demand mode is active, and a period of time equal to the Detection
   Time passes after the initiation of a Poll Sequence (the transmission
   of the first BFD Control packet with the Poll bit set), the session
   has gone down -- the local system MUST set bfd.SessionState to Down,
   and bfd.LocalDiag to 1 (Control Detection Time Expired).

   (Note that a packet is considered to have been received, for the
   purposes of Detection Time expiration, only if it has not been
   "discarded" according to the rules of section 6.8.6).

We are not using demand mode, so that puts us in async mode.
Paraphrasing the async calculation, it is remoteDetectMult * max(localRequiredMinRx, remoteDesiredMinTx).

Our code always calculates it using our local interval * our local multiplier -- never considering the peer's value:

endpoint.rx.recv_timeout(
    local.required_min_rx * local.detection_multiplier.into(),
)

If we encounter a timeout from this call, we declare the peer has timed out and transition out of the Up state.
A simple diff to ostensibly address Detection Time negotiation looks like this:

diff --git a/bfd/src/sm.rs b/bfd/src/sm.rs
index 0f16915aa7..5d87c07b7e 100644
--- a/bfd/src/sm.rs
+++ b/bfd/src/sm.rs
@@ -9,6 +9,7 @@
 use anyhow::{Result, anyhow};
 use mg_common::lock;
 use slog::Logger;
+use std::cmp::max;
 use std::net::IpAddr;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::mpsc::Sender;
@@ -340,8 +341,13 @@
         log: Logger,
         counters: Arc<SessionCounters>,
     ) -> Result<RecvResult> {
+        let (remote_detection_multiplier, remote_desired_min_tx) = {
+            let remote = lock!(remote);
+            (remote.detection_multiplier, remote.desired_min_tx)
+        };
         match endpoint.rx.recv_timeout(
-            local.required_min_rx * local.detection_multiplier.into(),
+            u32::from(remote_detection_multiplier)
+                * (max(local.required_min_rx, remote_desired_min_tx)),
         ) {
             Ok((addr, msg)) => {
                 sm_log!(

but I don't think this would fully bring us into spec. There are poll modes that start/stop in the middle of a session, and timers can change mid session too. I think this will need a more thorough review of the BFD state machine implementation.

[taspelund]

Follow-up on the timer thing. Juniper does seem to be using the configured 300ms tx/rx * 3 multiplier, but the actual calculated Detect Time and Transmit Interval look to be changing on the fly to values I'm not expecting (2s, 1s, etc.).

What I'm finding is that the BFD session becomes stable if I add no-adaptation to the juniper bfd config, which tells me we are probably doing something to cause the peer to also miscalculate their timers.

[edit]
root@crpd1# run show bfd session detail
                                                  Detect   Transmit
Address                  State     Interface      Time     Interval  Multiplier
10.0.2.1                 Up        eth1           3.000     0.300        3        <<<<<  Detect Time is 3s but transmit is 300ms?
 Client Static, TX interval 0.300, RX interval 0.300        <<<<<  tx/rx is 300ms
 Session up time 00:00:07, previous down time 00:00:04
 Local diagnostic None, remote diagnostic None
 Remote state Up, version 1
 Session type: Single hop BFD
                                                  Detect   Transmit
Address                  State     Interface      Time     Interval  Multiplier
fd00:3::1                Up        eth1           6.000     2.000        3        <<<<<  Detect Time is 6s but transmit is 2s?
 Client Static, TX interval 0.300, RX interval 0.300        <<<<<  tx/rx is 300ms
 Session up time 00:00:02, previous down time 00:00:00
 Local diagnostic None, remote diagnostic None
 Remote state Up, version 1
 Session type: Single hop BFD

2 sessions, 2 clients
Cumulative transmit rate 3.8 pps, cumulative receive rate 1.5 pps

[edit]
root@crpd1# run show bfd session detail
                                                  Detect   Transmit
Address                  State     Interface      Time     Interval  Multiplier
10.0.2.1                 Up        eth1           3.000     0.300        3        <<<<<  Detect Time is 3s but transmit is 300ms?
 Client Static, TX interval 0.300, RX interval 0.300        <<<<<  tx/rx is 300ms
 Session up time 00:00:08, previous down time 00:00:04
 Local diagnostic None, remote diagnostic None
 Remote state Up, version 1
 Session type: Single hop BFD
                                                  Detect   Transmit
Address                  State     Interface      Time     Interval  Multiplier
fd00:3::1                Down      eth1           3.000     1.000        3        <<<<<  Detect Time is 3s? Transmit is 1s?
 Client Static, TX interval 0.300, RX interval 0.300        <<<<<  tx/rx is 300ms
 Session down time 00:00:01, previous up time 00:00:02
 Local diagnostic NbrSignal, remote diagnostic None
 Remote state Down, version 1
 Session type: Single hop BFD

2 sessions, 2 clients
Cumulative transmit rate 4.3 pps, cumulative receive rate 2.0 pps

It looks like juniper uses adaptive BFD timers by default, which reacts to events like adjacency changes. Reference: https://www.juniper.net/documentation/us/en/software/junos/cli-reference/topics/ref/statement/no-adaptation-edit-protocols-bgp-liveness-detection.html

The BFD failure detection timers are adaptive and can be adjusted to be faster or slower. The lower the BFD failure detection timer value, the faster the failure detection and vice versa. For example, the timers can adapt to a higher value if the adjacency fails (that is, the timer detects failures more slowly). Or a neighbor can negotiate a higher value for a timer than the configured value. The timers adapt to a higher value when a BFD session flap occurs more than three times in a span of 15 seconds. A back-off algorithm increases the receive (Rx) interval by two if the local BFD instance is the reason for the session flap. The transmission (Tx) interval is increased by two if the remote BFD instance is the reason for the session flap. However, include the no-adaptation statement in the configuration if you do not want BFD sessions to adapt to changing network conditions.

All that to say this means they assume we correctly implement Polling, timer negotiation, etc. which is not the case currently.
I think having BFD interop tests against juniper's adaptive timers would be a great thing to run our implementation through in order to shake out more bugs.

[taspelund]

Also note: I am able to repro the flapping peers issue if I configure 2s tx/rx intervals with a multiplier of 3 on EOS and FRR.