Merge pull request #5141 from opensourcerouting/bfdd-fixes-bundle

bfdd: pack of bug fixes

bfdd/bfd.c

@@ -213,9 +213,13 @@ void bfd_session_disable(struct bfd_session *bs)
 
 	/* Disable all timers. */
 	bfd_recvtimer_delete(bs);
-	bfd_echo_recvtimer_delete(bs);
 	bfd_xmttimer_delete(bs);
-	bfd_echo_xmttimer_delete(bs);
+	ptm_bfd_echo_stop(bs);
+	bs->vrf = NULL;
+	bs->ifp = NULL;
+
+	/* Set session down so it doesn't report UP and disabled. */
+	ptm_bfd_sess_dn(bs, BD_PATH_DOWN);
 }
 
 static uint32_t ptm_bfd_gen_ID(void)
@@ -328,7 +332,14 @@ void ptm_bfd_sess_dn(struct bfd_session *bfd, uint8_t diag)
 	bfd->demand_mode = 0;
 	monotime(&bfd->downtime);
 
-	ptm_bfd_snd(bfd, 0);
+	/*
+	 * Only attempt to send if we have a valid socket:
+	 * this function might be called by session disablers and in
+	 * this case we won't have a valid socket (i.e. interface was
+	 * removed or VRF doesn't exist anymore).
+	 */
+	if (bfd->sock != -1)
+		ptm_bfd_snd(bfd, 0);
 
 	/* Slow down the control packets, the connection is down. */
 	bs_set_slow_timers(bfd);
@@ -1206,19 +1217,10 @@ int bs_observer_add(struct bfd_session *bs)
 	struct bfd_session_observer *bso;
 
 	bso = XCALLOC(MTYPE_BFDD_SESSION_OBSERVER, sizeof(*bso));
-	bso->bso_isaddress = false;
 	bso->bso_bs = bs;
-	bso->bso_isinterface = !BFD_CHECK_FLAG(bs->flags, BFD_SESS_FLAG_MH);
+	bso->bso_addr.family = bs->key.family;
-	if (bso->bso_isinterface)
+	memcpy(&bso->bso_addr.u.prefix, &bs->key.local,
-		strlcpy(bso->bso_entryname, bs->key.ifname,
+	       sizeof(bs->key.local));
-			sizeof(bso->bso_entryname));
-	/* Handle socket binding failures caused by missing local addresses. */
-	if (bs->sock == -1) {
-		bso->bso_isaddress = true;
-		bso->bso_addr.family = bs->key.family;
-		memcpy(&bso->bso_addr.u.prefix, &bs->key.local,
-		       sizeof(bs->key.local));
-	}
 
 	TAILQ_INSERT_TAIL(&bglobal.bg_obslist, bso, bso_entry);
 
@@ -1432,7 +1434,7 @@ struct bfd_session *bfd_key_lookup(struct bfd_key key)
 	if (ctx.result) {
 		bsp = ctx.result;
 		log_debug(" peer %s found, but ifp"
-			  " and/or loc-addr params ignored");
+			  " and/or loc-addr params ignored", peer_buf);
 	}
 	return bsp;
 }
@@ -1708,6 +1710,15 @@ static int bfd_vrf_disable(struct vrf *vrf)
 	}
 
 	log_debug("VRF disable %s id %d", vrf->name, vrf->vrf_id);
+
+	/* Disable read/write poll triggering. */
+	THREAD_OFF(bvrf->bg_ev[0]);
+	THREAD_OFF(bvrf->bg_ev[1]);
+	THREAD_OFF(bvrf->bg_ev[2]);
+	THREAD_OFF(bvrf->bg_ev[3]);
+	THREAD_OFF(bvrf->bg_ev[4]);
+	THREAD_OFF(bvrf->bg_ev[5]);
+
 	/* Close all descriptors. */
 	socket_close(&bvrf->bg_echo);
 	socket_close(&bvrf->bg_shop);

bfdd/bfd.h

@@ -274,12 +274,8 @@ struct bfd_state_str_list {
 
 struct bfd_session_observer {
 	struct bfd_session *bso_bs;
-	bool bso_isinterface;
+	char bso_entryname[MAXNAMELEN];
-	bool bso_isaddress;
+	struct prefix bso_addr;
-	union {
-		char bso_entryname[MAXNAMELEN];
-		struct prefix bso_addr;
-	};
 
 	TAILQ_ENTRY(bfd_session_observer) bso_entry;
 };

bfdd/bfdd_northbound.c

@@ -58,10 +58,36 @@ static int bfd_session_create(enum nb_event event, const struct lyd_node *dnode,
 			      union nb_resource *resource, bool mhop)
 {
 	struct bfd_session *bs;
+	const char *ifname;
 	struct bfd_key bk;
+	struct prefix p;
 
 	switch (event) {
 	case NB_EV_VALIDATE:
+		/*
+		 * When `dest-addr` is IPv6 and link-local we must
+		 * require interface name, otherwise we can't figure
+		 * which interface to use to send the packets.
+		 */
+		yang_dnode_get_prefix(&p, dnode, "./dest-addr");
+
+		/*
+		 * To support old FRR versions we must allow empty
+		 * interface to be specified, however that should
+		 * change in the future.
+		 */
+		if (yang_dnode_exists(dnode, "./interface"))
+			ifname = yang_dnode_get_string(dnode, "./interface");
+		else
+			ifname = "";
+
+		if (p.family == AF_INET6
+		    && IN6_IS_ADDR_LINKLOCAL(&p.u.prefix6)
+		    && strlen(ifname) == 0) {
+			zlog_warn("%s: when using link-local you must specify "
+				  "an interface.", __func__);
+			return NB_ERR_VALIDATION;
+		}
 		break;
 
 	case NB_EV_PREPARE:

bfdd/ptm_adapter.c

@@ -579,8 +579,6 @@ static void bfdd_sessions_enable_interface(struct interface *ifp)
 
 	TAILQ_FOREACH(bso, &bglobal.bg_obslist, bso_entry) {
 		bs = bso->bso_bs;
-		if (bso->bso_isinterface == false)
-			continue;
 		/* Interface name mismatch. */
 		if (strcmp(ifp->name, bs->key.ifname))
 			continue;
@@ -605,10 +603,6 @@ static void bfdd_sessions_disable_interface(struct interface *ifp)
 	struct bfd_session *bs;
 
 	TAILQ_FOREACH(bso, &bglobal.bg_obslist, bso_entry) {
-		if (bso->bso_isinterface == false)
-			continue;
-
-		/* Interface name mismatch. */
 		bs = bso->bso_bs;
 		if (strcmp(ifp->name, bs->key.ifname))
 			continue;
@@ -616,7 +610,6 @@ static void bfdd_sessions_disable_interface(struct interface *ifp)
 		if (bs->sock == -1)
 			continue;
 
-		/* Try to enable it. */
 		bfd_session_disable(bs);
 
 	}
@@ -658,8 +651,6 @@ void bfdd_sessions_disable_vrf(struct vrf *vrf)
 	struct bfd_session *bs;
 
 	TAILQ_FOREACH(bso, &bglobal.bg_obslist, bso_entry) {
-		if (bso->bso_isinterface)
-			continue;
 		bs = bso->bso_bs;
 		if (bs->key.vrfname[0] &&
 		    strcmp(vrf->name, bs->key.vrfname))
@@ -668,7 +659,6 @@ void bfdd_sessions_disable_vrf(struct vrf *vrf)
 		if (bs->sock == -1)
 			continue;
 
-		/* Try to enable it. */
 		bfd_session_disable(bs);
 	}
 }
@@ -701,9 +691,6 @@ static void bfdd_sessions_enable_address(struct connected *ifc)
 	struct prefix prefix;
 
 	TAILQ_FOREACH(bso, &bglobal.bg_obslist, bso_entry) {
-		if (bso->bso_isaddress == false)
-			continue;
-
 		/* Skip enabled sessions. */
 		bs = bso->bso_bs;
 		if (bs->sock != -1)

lib/yang_wrappers.c

@@ -799,7 +799,7 @@ struct yang_data *yang_data_new_prefix(const char *xpath,
 	return yang_data_new(xpath, value_str);
 }
 
-void yang_dnode_get_prefix(union prefixptr prefix, const struct lyd_node *dnode,
+void yang_dnode_get_prefix(struct prefix *prefix, const struct lyd_node *dnode,
 			   const char *xpath_fmt, ...)
 {
 	const struct lyd_node_leaf_list *dleaf;
@@ -816,9 +816,15 @@ void yang_dnode_get_prefix(union prefixptr prefix, const struct lyd_node *dnode,
 		YANG_DNODE_GET_ASSERT(dnode, xpath);
 	}
 
+	/*
+	 * Initialize prefix to avoid static analyzer complaints about
+	 * uninitialized memory.
+	 */
+	memset(prefix, 0, sizeof(*prefix));
+
 	dleaf = (const struct lyd_node_leaf_list *)dnode;
 	assert(dleaf->value_type == LY_TYPE_STRING);
-	(void)str2prefix(dleaf->value_str, prefix.p);
+	(void)str2prefix(dleaf->value_str, prefix);
 }
 
 void yang_get_default_prefix(union prefixptr var, const char *xpath_fmt, ...)

lib/yang_wrappers.h

@@ -118,7 +118,7 @@ extern void yang_get_default_string_buf(char *buf, size_t size,
 extern void yang_str2prefix(const char *value, union prefixptr prefix);
 extern struct yang_data *yang_data_new_prefix(const char *xpath,
 					      union prefixconstptr prefix);
-extern void yang_dnode_get_prefix(union prefixptr prefix,
+extern void yang_dnode_get_prefix(struct prefix *prefix,
 				  const struct lyd_node *dnode,
 				  const char *xpath_fmt, ...);
 extern void yang_get_default_prefix(union prefixptr var, const char *xpath_fmt,