Fixes in this commit:

1. RFC violation on session reset.
2. RFC violation on checking of cp->len.
3. RFC violation on the timing of updating remote discriminator.

For 1, the previous logic in `bfd_recv_cb` incorrectly discarded all
packets  with `Your Discriminator` set to 0 if the local session state was
`UP` or `INIT`. This violated RFC 5880 Section 6.8.6, which requires
accepting such packets (specifically when State is DOWN) to handle remote
peer restarts. This discrepancy may cause "zombie sessions" where the local
side remained UP ignoring the peer's reset signal.

The original check was introduced with the intention to prevent session aliasing,
which caused session state flapping. However, the check was too broad.

This patch replaces the state-based check with a more strict address-based check:
    - Removed the conditions of `bfd->ses_state !=` which would block legitimate resets.
    - Added `match_ip_address` to verify that if a zero-discriminator packet arrives for
      an UP session. its destination address matches the session's bound local address.
    - Added the check required by the RFC 5880 that if a zero remote discriminator packet
      arrives with a state that is not DOWN or ADMIN_DOWN, the packet should be dropped.

This allows legitimate peer resets (correct destination) to proceed while dropping
aliased packets that causes vibration.

For 2, this is a relatively minor one and potentially nitpicking. But RFC 5880 requires
that the parser should check `cp->len < 26` if A bit is set, so I include it.

For 3, the patch delays the update of remote discriminator to after `bfd_check_auth` returns
success. This is required by the RFC. Updating remote discriminator before auth check
would incorrectly update local state even when authentication fails, which can be harmful.

This patch also modifies `bfd.h` to add a new pair of marco that get and set A bit. It also
changes the implementation of `BFD_SETCBIT` to align with other marco's way of implementation.

Signed-off-by: spademomo <[email protected]>

Author: SpadeMomo

.github/PULL_REQUEST_TEMPLATE/pr.md …EQUEST_TEMPLATE/pull_request_template.md.github/PULL_REQUEST_TEMPLATE/pr.md renamed to .github/PULL_REQUEST_TEMPLATE/pull_request_template.md .github/PULL_REQUEST_TEMPLATE/pr.md renamed to .github/PULL_REQUEST_TEMPLATE/pull_request_template.md

@@ -0,0 +1,6 @@
+pull_request_rules:
+  - name: Delete merged branch
+    conditions:
+      - merged
+    actions:
+      delete_head_branch:

.github/mergify.yml

@@ -24,6 +24,7 @@ jobs:
         -  { os: 'ubuntu-latest' ,rel: '24.04',  name: 'Ubuntu 24.04 amd64 Build',platform : 'amd64_u24' }
         -  { os: 'ubuntu-22.04-arm' ,rel: '22.04',  name: 'Ubuntu 22.04 arm64 Build',platform : 'arm64_u22' }
         -  { os: 'ubuntu-22.04-arm' ,rel: '24.04',  name: 'Ubuntu 24.04 arm64 Build',platform : 'arm64_u24' }
+        -  { os: 'ubuntu-latest' ,rel: '24.04',  name: 'Ubuntu 24.04 amd64 LTTng Build',platform : 'amd64_u24_lttng', lttng: 'true' }
 
     steps:
       - name: Checkout
@@ -32,7 +33,7 @@ jobs:
           fetch-depth: 1
       - name: Build docker image
         run: |
-          docker build --no-cache -t frr-${{ matrix.cfg.platform }} --build-arg=UBUNTU_VERSION=${{ matrix.cfg.rel }} -f docker/ubuntu-ci/Dockerfile .
+          docker build --no-cache -t frr-${{ matrix.cfg.platform }} --build-arg=UBUNTU_VERSION=${{ matrix.cfg.rel }} --build-arg=ENABLE_LTTNG=${{ matrix.cfg.lttng || 'false' }} -f docker/ubuntu-ci/Dockerfile .
           docker save --output /tmp/frr-${{ matrix.cfg.platform }}.tar frr-${{ matrix.cfg.platform }}
       - name: Upload docker image artifact
         uses: actions/upload-artifact@v4
@@ -86,7 +87,9 @@ jobs:
         run: |
           uname -a
           MODPKGVER=$(uname -r)
-          sudo apt-get update -y
+          # Retry loop for apt-get update in case of transient mirror failures
+          APT_RETRIES=3
+          for i in $(seq 1 ${APT_RETRIES}); do sudo apt-get update -y && break || sleep 10; done
           # Github is running old kernels but installing newer packages :(
           sudo apt-get install -y linux-modules-extra-azure linux-modules-${MODPKGVER} linux-modules-extra-${MODPKGVER} python3-xmltodict
           sudo modprobe vrf || true

.github/workflows/github-ci.yml

@@ -19,7 +19,7 @@ makedepends="ncurses-dev net-snmp-dev gawk texinfo perl
     perl pkgconf python3 python3-dev readline readline-dev sqlite-libs pcre2-dev
     squashfs-tools sudo tar texinfo xorriso xz-libs py-pip rtrlib rtrlib-dev
     py3-sphinx elfutils elfutils-dev protobuf-c-compiler protobuf-c-dev
-    lua5.3-dev lua5.3 gzip"
+    lua5.3-dev lua5.3 gzip pytest"
 checkdepends="pytest py-setuptools"
 install="$pkgname.pre-install $pkgname.pre-deinstall $pkgname.post-deinstall"
 subpackages="$pkgname-dev $pkgname-doc $pkgname-dbg"

alpine/APKBUILD.in

@@ -18,6 +18,7 @@
 #include "lib/network.h"
 
 #include "bfd.h"
+#include "bfd_trace.h"
 
 DEFINE_MTYPE_STATIC(BFDD, BFDD_CONFIG, "long-lived configuration memory");
 DEFINE_MTYPE_STATIC(BFDD, BFDD_PROFILE, "long-lived profile memory");
@@ -153,6 +154,9 @@ void bfd_profile_apply(const char *profname, struct bfd_session *bs)
 	/* Point to profile if it exists. */
 	bs->profile = bp;
 
+	if (bp)
+		frrtrace(2, frr_bfd, profile_apply, bs, profname);
+
 	/* Apply configuration. */
 	bfd_session_apply(bs);
 }
@@ -232,10 +236,16 @@ void bfd_session_apply(struct bfd_session *bs)
 		bfd_set_log_session_changes(bs, bs->peer_profile.log_session_changes);
 
 	/* If session interval changed negotiate new timers. */
-	if (bs->ses_state == PTM_BFD_UP
-	    && (bs->timers.desired_min_tx != min_tx
-		|| bs->timers.required_min_rx != min_rx))
+	if (bs->ses_state == PTM_BFD_UP &&
+	    (bs->timers.desired_min_tx != min_tx || bs->timers.required_min_rx != min_rx)) {
+		if (bglobal.debug_peer_event) {
+			zlog_debug("[%s] Timer changed while UP: old_tx=%u new_tx=%u, old_rx=%u new_rx=%u",
+				   bs_to_string(bs), min_tx, bs->timers.desired_min_tx, min_rx,
+				   bs->timers.required_min_rx);
+			zlog_debug("Starting poll sequence to negotiate new timers");
+		}
 		bfd_set_polling(bs);
+	}
 
 	/* Send updated information to data plane. */
 	bfd_dplane_update_session(bs);
@@ -327,6 +337,7 @@ int bfd_session_enable(struct bfd_session *bs)
 	if (bs->key.vrfname[0]) {
 		vrf = vrf_lookup_by_name(bs->key.vrfname);
 		if (vrf == NULL) {
+			frrtrace(1, frr_bfd, vrf_not_found, bs->key.vrfname);
 			zlog_err(
 				"session-enable: specified VRF %s doesn't exists.",
 				bs->key.vrfname);
@@ -341,6 +352,7 @@ int bfd_session_enable(struct bfd_session *bs)
 	if (bs->key.ifname[0]) {
 		ifp = if_lookup_by_name(bs->key.ifname, vrf->vrf_id);
 		if (ifp == NULL) {
+			frrtrace(2, frr_bfd, interface_not_found, bs->key.ifname, vrf->vrf_id);
 			zlog_err(
 				"session-enable: specified interface %s (VRF %s) doesn't exist.",
 				bs->key.ifname, vrf->name);
@@ -415,6 +427,8 @@ int bfd_session_enable(struct bfd_session *bs)
 	/* initialize RTT */
 	bfd_rtt_init(bs);
 
+	frrtrace(2, frr_bfd, session_enable_event, true, bs);
+
 	return 0;
 }
 
@@ -430,6 +444,8 @@ void bfd_session_disable(struct bfd_session *bs)
 	if (bs->bdc)
 		return;
 
+	frrtrace(2, frr_bfd, session_enable_event, false, bs);
+
 	/* Free up socket resources. */
 	if (bs->sock != -1) {
 		close(bs->sock);
@@ -562,6 +578,8 @@ void ptm_bfd_echo_stop(struct bfd_session *bfd)
 
 	bfd_echo_xmttimer_delete(bfd);
 	bfd_echo_recvtimer_delete(bfd);
+
+	frrtrace(2, frr_bfd, echo_mode_change, bfd, false);
 }
 
 void ptm_bfd_echo_start(struct bfd_session *bfd)
@@ -570,6 +588,7 @@ void ptm_bfd_echo_start(struct bfd_session *bfd)
 	if (bfd->echo_detect_TO > 0) {
 		bfd_echo_recvtimer_update(bfd);
 		ptm_bfd_echo_xmt_TO(bfd);
+		frrtrace(2, frr_bfd, echo_mode_change, bfd, true);
 	}
 }
 
@@ -589,6 +608,8 @@ void ptm_bfd_sess_up(struct bfd_session *bfd)
 
 	ptm_bfd_notify(bfd, bfd->ses_state);
 
+	frrtrace(4, frr_bfd, state_change, bfd, old_state, bfd->ses_state, bfd->local_diag);
+
 	if (old_state != bfd->ses_state) {
 		bfd->stats.session_up++;
 		if (bglobal.debug_peer_event)
@@ -638,6 +659,8 @@ void ptm_bfd_sess_dn(struct bfd_session *bfd, uint8_t diag)
 		bfd_xmttimer_delete(bfd);
 	}
 
+	frrtrace(4, frr_bfd, state_change, bfd, old_state, bfd->ses_state, bfd->local_diag);
+
 	if (old_state != bfd->ses_state) {
 		bfd->stats.session_down++;
 		if (bglobal.debug_peer_event)
@@ -1005,6 +1028,8 @@ void bfd_session_free(struct bfd_session *bs)
 {
 	struct bfd_session_observer *bso;
 
+	frrtrace(2, frr_bfd, session_lifecycle, false, bs);
+
 	bfd_session_disable(bs);
 
 	/* Remove session from data plane if any. */
@@ -1116,6 +1141,8 @@ struct bfd_session *bs_registrate(struct bfd_session *bfd)
 	if (bfd->key.ifname[0] || bfd->key.vrfname[0] || bfd->sock == -1)
 		bs_observer_add(bfd);
 
+	frrtrace(2, frr_bfd, session_lifecycle, true, bfd);
+
 	if (bglobal.debug_peer_event)
 		zlog_debug("session-new: %s", bs_to_string(bfd));
 
@@ -1133,6 +1160,7 @@ int ptm_bfd_sess_del(struct bfd_peer_cfg *bpc)
 
 	/* This pointer is being referenced, don't let it be deleted. */
 	if (bs->refcount > 0) {
+		frrtrace(1, frr_bfd, refcount_error, bs->refcount);
 		zlog_err("session-delete: refcount failure: %" PRIu64" references",
 			 bs->refcount);
 		return -1;
@@ -1157,6 +1185,9 @@ void bfd_set_polling(struct bfd_session *bs)
 	 *
 	 * RFC 5880, Section 6.8.3.
 	 */
+	if (bglobal.debug_peer_event)
+		zlog_debug("[%s] Setting polling=1 to negotiate timer change", bs_to_string(bs));
+
 	bs->polling = 1;
 }
 
@@ -1482,13 +1513,27 @@ void bs_final_handler(struct bfd_session *bs)
 		bs->xmt_TO = bs->timers.desired_min_tx;
 	else
 		bs->xmt_TO = bs->remote_timers.required_min_rx;
-	
+
+	if (bglobal.debug_peer_event)
+		zlog_debug("  calculated xmt_TO=%" PRIu64 " (using slowest rate)", bs->xmt_TO);
+
+	frrtrace(6, frr_bfd, timer_negotiation, bs, bs->xmt_TO, bs->cur_timers.required_min_rx,
+		 bs->detect_TO, bs->echo_xmt_TO, bs->echo_detect_TO);
+
 	/* Only apply increased transmission interval after Poll Sequence */
-	if (bs->ses_state == PTM_BFD_UP && bs->xmt_TO > old_xmt_TO) {
-		bs->xmt_TO = old_xmt_TO;  /* Keep old timing until Poll Sequence done */
+	if (bs->ses_state == PTM_BFD_UP && bs->xmt_TO > old_xmt_TO && bs->polling) {
+		if (bglobal.debug_peer_event) {
+			zlog_debug("  REJECTED increase: xmt_TO=%" PRIu64 " > old=%" PRIu64
+				   " (polling=%d)",
+				   bs->xmt_TO, old_xmt_TO, bs->polling);
+		}
+		bs->xmt_TO = old_xmt_TO; /* Keep old timing DURING Poll Sequence */
 		return;
 	}
 
+	if (bglobal.debug_peer_event)
+		zlog_debug("  APPLYING new xmt_TO=%" PRIu64, bs->xmt_TO);
+
 	/* Apply new transmission timer immediately. */
 	ptm_bfd_start_xmt_timer(bs, false);
 }
@@ -2442,6 +2487,8 @@ static int bfd_vrf_new(struct vrf *vrf)
 	if (bglobal.debug_zebra)
 		zlog_debug("VRF Created: %s(%u)", vrf->name, vrf->vrf_id);
 
+	frrtrace(2, frr_bfd, vrf_lifecycle, 1, vrf->vrf_id);
+
 	bvrf = XCALLOC(MTYPE_BFDD_VRF, sizeof(struct bfd_vrf_global));
 	bvrf->vrf = vrf;
 	vrf->info = bvrf;
@@ -2463,6 +2510,8 @@ static int bfd_vrf_delete(struct vrf *vrf)
 	if (bglobal.debug_zebra)
 		zlog_debug("VRF Deletion: %s(%u)", vrf->name, vrf->vrf_id);
 
+	frrtrace(2, frr_bfd, vrf_lifecycle, 2, vrf->vrf_id);
+
 	XFREE(MTYPE_BFDD_VRF, vrf->info);
 
 	return 0;
@@ -2475,6 +2524,8 @@ static int bfd_vrf_enable(struct vrf *vrf)
 	if (bglobal.debug_zebra)
 		zlog_debug("VRF enable add %s id %u", vrf->name, vrf->vrf_id);
 
+	frrtrace(2, frr_bfd, vrf_lifecycle, 3, vrf->vrf_id);
+
 	/* Don't open sockets when using data plane */
 	if (bglobal.bg_use_dplane)
 		goto skip_sockets;
@@ -2536,6 +2587,8 @@ static int bfd_vrf_disable(struct vrf *vrf)
 	if (bglobal.debug_zebra)
 		zlog_debug("VRF disable %s id %d", vrf->name, vrf->vrf_id);
 
+	frrtrace(2, frr_bfd, vrf_lifecycle, 4, vrf->vrf_id);
+
 	/* Disable read/write poll triggering. */
 	event_cancel(&bvrf->bg_ev[0]);
 	event_cancel(&bvrf->bg_ev[1]);

bfdd/bfd.c

@@ -168,6 +168,7 @@ struct bfd_echo_pkt {
 #define BFD_DEMANDBIT 0x02
 #define BFD_MBIT	      0x01
 #define BFD_GETMBIT(flags)    (CHECK_FLAG(flags, BFD_MBIT))
+#define BFD_GETDEMANDBIT(flags) (CHECK_FLAG(flags, BFD_DEMANDBIT))
 #define BFD_SETDEMANDBIT(flags, val)                                           \
 	{                                                                      \
 		if ((val))                                                     \
@@ -191,12 +192,18 @@ struct bfd_echo_pkt {
 			SET_FLAG(flags, (CHECK_FLAG(val, 0x3) << 6));          \
 	}
 #define BFD_GETSTATE(flags) (CHECK_FLAG((flags >> 6), 0x3))
-#define BFD_SETCBIT(flags, val)                                                \
-	{                                                                      \
-		if ((val))                                                     \
-			SET_FLAG(flags, val);                                  \
+#define BFD_SETCBIT(flags, val)                                                                   \
+	{                                                                                         \
+		if ((val))                                                                        \
+			SET_FLAG(flags, BFD_CBIT);                                                \
 	}
 #define BFD_GETCBIT(flags) (CHECK_FLAG(flags, BFD_CBIT))
+#define BFD_SETABIT(flags, val)                                                                   \
+	{                                                                                         \
+		if ((val))                                                                        \
+			SET_FLAG(flags, BFD_ABIT);                                                \
+	}
+#define BFD_GETABIT(flags) (CHECK_FLAG(flags, BFD_ABIT))
 #define BFD_ECHO_VERSION 1
 #define BFD_ECHO_PKT_LEN sizeof(struct bfd_echo_pkt)
 
@@ -357,7 +364,9 @@ struct bfd_session {
 	struct event *echo_recvtimer_ev;
 	struct event *recvtimer_ev;
 	uint64_t xmt_TO;
+	uint64_t xmt_TO_actual; /* Actual transmit timeout with jitter applied */
 	uint64_t echo_xmt_TO;
+	uint64_t echo_xmt_TO_actual; /* Actual echo transmit timeout with jitter applied */
 	struct event *xmttimer_ev;
 	struct event *echo_xmttimer_ev;
 	uint64_t echo_detect_TO;