Rework the handling of Hello packets.

With the introduction of IPv6 support by RFC 7552, the handling of Hello
packets in ldpd became something incredibly complex. Neighbors can change
from single-stack LDP to dual-stack and vice-versa. They can change
their transport preference, their transport addresses (IPv4 and IPv6)
and even start or stop sending the Dual-Stack TLV. We also have to take
care to reject things like multiple adjacencies advertising different
transport-addresses for the same neighbor. ldpd was failing for some of
the cases mentioned above, this patch fixes these issues and attempts
to make the code easier to read.

3 files changed, 84 insertions, 69 deletions

diff --git a/usr.sbin/ldpd/adjacency.c b/usr.sbin/ldpd/adjacency.c
index 6cc73af45ef..80120903a30 100644
--- a/usr.sbin/ldpd/adjacency.c
+++ b/usr.sbin/ldpd/adjacency.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: adjacency.c,v 1.23 2016/06/09 17:26:32 renato Exp $ */
+/*	$OpenBSD: adjacency.c,v 1.24 2016/06/13 20:13:34 renato Exp $ */
 
 /*
  * Copyright (c) 2013, 2015 Renato Westphal <renato@openbsd.org>
@@ -70,6 +70,9 @@ adj_new(struct in_addr lsr_id, struct hello_source *source,
 void
 adj_del(struct adj *adj, int send_notif, uint32_t notif_status)
 {
+	struct nbr	*nbr = adj->nbr;
+	struct adj	*atmp;
+
 	log_debug("%s: lsr-id %s, %s", __func__, inet_ntoa(adj->lsr_id),
 	    log_hello_src(&adj->source));
 
@@ -80,15 +83,24 @@ adj_del(struct adj *adj, int send_notif, uint32_t notif_status)
 		LIST_REMOVE(adj, nbr_entry);
 	if (adj->source.type == HELLO_LINK)
 		LIST_REMOVE(adj, ia_entry);
+	free(adj);
 
 	/* last adjacency deleted */
-	if (adj->nbr && LIST_EMPTY(&adj->nbr->adj_list)) {
+	if (nbr && LIST_EMPTY(&nbr->adj_list)) {
 		if (send_notif)
-			session_shutdown(adj->nbr, notif_status, 0, 0);
-		nbr_del(adj->nbr);
+			session_shutdown(nbr, notif_status, 0, 0);
+		nbr_del(nbr);
+		nbr = NULL;
 	}
 
-	free(adj);
+	/*
+	 * If the neighbor still exists but none of its remaining adjacencies
+	 * are from the preferred address-family, then delete it.
+	 */
+	if (nbr && nbr_adj_count(nbr, nbr->af) == 0) {
+		LIST_FOREACH_SAFE(adj, &nbr->adj_list, nbr_entry, atmp)
+			adj_del(adj, 0, 0);
+	}
 }
 
 struct adj *
diff --git a/usr.sbin/ldpd/hello.c b/usr.sbin/ldpd/hello.c
index c74a1f3e24d..c8ae8d205b3 100644
--- a/usr.sbin/ldpd/hello.c
+++ b/usr.sbin/ldpd/hello.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: hello.c,v 1.47 2016/06/11 02:06:46 renato Exp $ */
+/*	$OpenBSD: hello.c,v 1.48 2016/06/13 20:13:34 renato Exp $ */
 
 /*
  * Copyright (c) 2013, 2016 Renato Westphal <renato@openbsd.org>
@@ -139,7 +139,7 @@ recv_hello(struct in_addr lsr_id, struct ldp_msg *lm, int af,
     uint16_t len)
 {
 	struct adj		*adj = NULL;
-	struct nbr		*nbr;
+	struct nbr		*nbr, *nbrt;
 	uint16_t		 holdtime, flags;
 	int			 tlvs_rcvd;
 	int			 ds_tlv;
@@ -201,14 +201,18 @@ recv_hello(struct in_addr lsr_id, struct ldp_msg *lm, int af,
 	if (af == AF_INET6 && IN6_IS_SCOPE_EMBED(&trans_addr.v6)) {
 		/*
 	 	 * RFC 7552 - Section 6.1:
-		 * An LSR MUST use a global unicast IPv6 address in an IPv6
+		 * "An LSR MUST use a global unicast IPv6 address in an IPv6
 		 * Transport Address optional object of outgoing targeted
 		 * Hellos and check for the same in incoming targeted Hellos
 		 * (i.e., MUST discard the targeted Hello if it failed the
 		 * check)".
 		 */
-		if (flags & TARGETED_HELLO)
+		if (flags & TARGETED_HELLO) {
+			log_debug("%s: lsr-id %s: invalid targeted hello "
+			    "transport address %s", __func__, inet_ntoa(lsr_id),
+			     log_addr(af, &trans_addr));
 			return;
+		}
 		scope_id = iface->ifindex;
 	}
 
@@ -282,74 +286,73 @@ recv_hello(struct in_addr lsr_id, struct ldp_msg *lm, int af,
 		return;
 	}
 
-	if (adj == NULL) {
-		adj = adj_new(lsr_id, &source, &trans_addr);
-		if (nbr) {
-			adj->nbr = nbr;
-			LIST_INSERT_HEAD(&nbr->adj_list, adj, nbr_entry);
+	/*
+	 * Check for noncompliant dual-stack neighbor according to
+	 * RFC 7552 section 6.1.1.
+	 */
+	if (nbr && !ds_tlv) {
+		switch (af) {
+		case AF_INET:
+			if (nbr_adj_count(nbr, AF_INET6) > 0) {
+				session_shutdown(nbr, S_DS_NONCMPLNCE,
+				    lm->msgid, lm->type);
+				return;
+			}
+			break;
+		case AF_INET6:
+			if (nbr_adj_count(nbr, AF_INET) > 0) {
+				session_shutdown(nbr, S_DS_NONCMPLNCE,
+				    lm->msgid, lm->type);
+				return;
+			}
+			break;
+		default:
+			fatalx("recv_hello: unknown af");
 		}
 	}
 
+	/*
+	 * Protections against misconfigured networks and buggy implementations.
+	 */
+	if (nbr && nbr->af == af &&
+	    (ldp_addrcmp(af, &nbr->raddr, &trans_addr) ||
+	    nbr->raddr_scope != scope_id)) {
+		log_warnx("%s: lsr-id %s: hello packet advertising a different "
+		    "transport address", __func__, inet_ntoa(lsr_id));
+		if (adj)
+			adj_del(adj, 0, 0);
+		return;
+	}
 	if (nbr == NULL) {
-		/*
-		 * The hello adjacency's address-family doesn't match the local
-		 * preference.
-		 */
-		if (ds_tlv &&
-		    ((trans_pref == DUAL_STACK_LDPOV4 && af != AF_INET) ||
-		    (trans_pref == DUAL_STACK_LDPOV6 && af != AF_INET6)))
-			return;
-
-		nbr = nbr_find_addr(af, &trans_addr);
-		if (nbr) {
+		nbrt = nbr_find_addr(af, &trans_addr);
+		if (nbrt) {
 			log_debug("%s: transport address %s is already being "
 			    "used by lsr-id %s", __func__, log_addr(af,
-			    &trans_addr), inet_ntoa(nbr->id));
+			    &trans_addr), inet_ntoa(nbrt->id));
+			if (adj)
+				adj_del(adj, 0, 0);
 			return;
 		}
+	}
 
-		/* create new adjacency and new neighbor */
-		nbr = nbr_new(lsr_id, af, ds_tlv, &trans_addr, scope_id);
-	} else {
-		/*
-		 * Check for noncompliant dual-stack neighbor according to
-		 * RFC 7552 section 6.1.1.
-		 */
-		if (!ds_tlv) {
-			switch (af) {
-			case AF_INET:
-				if (nbr_adj_count(nbr, AF_INET6) > 0) {
-					session_shutdown(nbr, S_DS_NONCMPLNCE,
-					    lm->msgid, lm->type);
-					return;
-				}
-				break;
-			case AF_INET6:
-				if (nbr_adj_count(nbr, AF_INET) > 0) {
-					session_shutdown(nbr, S_DS_NONCMPLNCE,
-					    lm->msgid, lm->type);
-					return;
-				}
-				break;
-			default:
-				fatalx("recv_hello: unknown af");
-			}
-		}
-
-		/*
-		 * Protection against misconfigured networks and buggy
-		 * implementations.
-		 */
-		if (af == nbr->af &&
-		    (ldp_addrcmp(af, &nbr->raddr, &trans_addr) ||
-		    nbr->raddr_scope != scope_id)) {
-			log_warnx("%s: lsr-id %s: ignoring hello packet "
-			    "advertising different transport address", __func__,
-			    inet_ntoa(lsr_id));
-			return;
+	if (adj == NULL) {
+		adj = adj_new(lsr_id, &source, &trans_addr);
+		if (nbr) {
+			adj->nbr = nbr;
+			LIST_INSERT_HEAD(&nbr->adj_list, adj, nbr_entry);
 		}
 	}
 
+	/*
+	 * If the hello adjacency's address-family doesn't match the local
+	 * preference, then an adjacency is still created but we don't attempt
+	 * to start an LDP session.
+	 */
+	if (nbr == NULL && (!ds_tlv ||
+	    ((trans_pref == DUAL_STACK_LDPOV4 && af != AF_INET) ||
+	    (trans_pref == DUAL_STACK_LDPOV6 && af != AF_INET6))))
+		nbr = nbr_new(lsr_id, af, ds_tlv, &trans_addr, scope_id);
+
 	/* always update the holdtime to properly handle runtime changes */
 	switch (source.type) {
 	case HELLO_LINK:
@@ -369,8 +372,8 @@ recv_hello(struct in_addr lsr_id, struct ldp_msg *lm, int af,
 	else
 		adj_stop_itimer(adj);
 
-	if (nbr->state == NBR_STA_PRESENT && nbr_session_active_role(nbr) &&
-	    !nbr_pending_connect(nbr) && !nbr_pending_idtimer(nbr))
+	if (nbr && nbr->state == NBR_STA_PRESENT && !nbr_pending_idtimer(nbr) &&
+	    nbr_session_active_role(nbr) && !nbr_pending_connect(nbr))
 		nbr_establish_connection(nbr);
 }
 
diff --git a/usr.sbin/ldpd/packet.c b/usr.sbin/ldpd/packet.c
index e71c161094e..f8a7f45549b 100644
--- a/usr.sbin/ldpd/packet.c
+++ b/usr.sbin/ldpd/packet.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: packet.c,v 1.61 2016/06/09 17:57:55 renato Exp $ */
+/*	$OpenBSD: packet.c,v 1.62 2016/06/13 20:13:34 renato Exp $ */
 
 /*
  * Copyright (c) 2013, 2016 Renato Westphal <renato@openbsd.org>
@@ -624,7 +624,7 @@ session_shutdown(struct nbr *nbr, uint32_t status, uint32_t msgid,
 	case NBR_STA_INITIAL:
 	case NBR_STA_OPENREC:
 	case NBR_STA_OPENSENT:
-	case NBR_STA_OPER:	
+	case NBR_STA_OPER:
 		log_debug("%s: lsr-id %s", __func__, inet_ntoa(nbr->id));
 
 		send_notification_nbr(nbr, status, msgid, type);