Properly start reference counting struct nexthop. This removes the need for

some ugly workaround to make sure nexthop objects don't disapear while
still being referenced. During initial lookup of a nexthop a extra reference
is pulled but even that is now a bit cleaner than before.
Tested by job@, dennis@, benno@ OK job@ dennis@

5 files changed, 83 insertions, 97 deletions

diff --git a/usr.sbin/bgpd/bgpd.h b/usr.sbin/bgpd/bgpd.h
index 5ec5ebf30d7..17108cbe0f4 100644
--- a/usr.sbin/bgpd/bgpd.h
+++ b/usr.sbin/bgpd/bgpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: bgpd.h,v 1.318 2018/06/13 09:33:51 claudio Exp $ */
+/*	$OpenBSD: bgpd.h,v 1.319 2018/06/25 14:28:33 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -915,20 +915,22 @@ enum action_types {
 	ACTION_SET_ORIGIN
 };
 
+struct nexthop;
 struct filter_set {
 	TAILQ_ENTRY(filter_set)		entry;
 	union {
-		u_int8_t		prepend;
-		u_int16_t		id;
-		u_int32_t		metric;
-		int32_t			relative;
-		struct bgpd_addr	nexthop;
-		struct filter_community	community;
-		struct filter_largecommunity	large_community;
-		struct filter_extcommunity	ext_community;
-		char			pftable[PFTABLE_LEN];
-		char			rtlabel[RTLABEL_LEN];
-		u_int8_t		origin;
+		u_int8_t			 prepend;
+		u_int16_t			 id;
+		u_int32_t			 metric;
+		int32_t				 relative;
+		struct bgpd_addr		 nexthop;
+		struct nexthop			*nh;
+		struct filter_community		 community;
+		struct filter_largecommunity	 large_community;
+		struct filter_extcommunity	 ext_community;
+		char				 pftable[PFTABLE_LEN];
+		char				 rtlabel[RTLABEL_LEN];
+		u_int8_t			 origin;
 	} action;
 	enum action_types		type;
 };
diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c
index 446dbe9654e..da5b9978054 100644
--- a/usr.sbin/bgpd/rde.c
+++ b/usr.sbin/bgpd/rde.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rde.c,v 1.380 2018/06/13 09:33:51 claudio Exp $ */
+/*	$OpenBSD: rde.c,v 1.381 2018/06/25 14:28:33 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -377,7 +377,6 @@ rde_dispatch_imsg_session(struct imsgbuf *ibuf)
 	struct rde_peer		*peer;
 	struct rde_aspath	*asp;
 	struct filter_set	*s;
-	struct nexthop		*nh;
 	u_int8_t		*asdata;
 	ssize_t			 n;
 	int			 verbose;
@@ -570,12 +569,9 @@ badnet:
 			if ((s = malloc(sizeof(struct filter_set))) == NULL)
 				fatal(NULL);
 			memcpy(s, imsg.data, sizeof(struct filter_set));
+			if (s->type == ACTION_SET_NEXTHOP)
+				s->action.nh = nexthop_get(&s->action.nexthop);
 			TAILQ_INSERT_TAIL(session_set, s, entry);
-
-			if (s->type == ACTION_SET_NEXTHOP) {
-				nh = nexthop_get(&s->action.nexthop);
-				nh->refcnt++;
-			}
 			break;
 		case IMSG_CTL_SHOW_NETWORK:
 		case IMSG_CTL_SHOW_RIB:
@@ -668,7 +664,6 @@ rde_dispatch_imsg_parent(struct imsgbuf *ibuf)
 	struct filter_head	*nr;
 	struct filter_rule	*r;
 	struct filter_set	*s;
-	struct nexthop		*nh;
 	struct rib		*rib;
 	struct prefixset	*ps;
 	struct prefixset_item	*psi;
@@ -907,12 +902,9 @@ rde_dispatch_imsg_parent(struct imsgbuf *ibuf)
 			if ((s = malloc(sizeof(struct filter_set))) == NULL)
 				fatal(NULL);
 			memcpy(s, imsg.data, sizeof(struct filter_set));
+			if (s->type == ACTION_SET_NEXTHOP)
+				s->action.nh = nexthop_get(&s->action.nexthop);
 			TAILQ_INSERT_TAIL(parent_set, s, entry);
-
-			if (s->type == ACTION_SET_NEXTHOP) {
-				nh = nexthop_get(&s->action.nexthop);
-				nh->refcnt++;
-			}
 			break;
 		case IMSG_MRT_OPEN:
 		case IMSG_MRT_REOPEN:
@@ -1263,8 +1255,7 @@ rde_update_dispatch(struct imsg *imsg)
 		 * But first unlock the previously locked nexthop.
 		 */
 		if (asp->nexthop) {
-			asp->nexthop->refcnt--;
-			(void)nexthop_delete(asp->nexthop);
+			(void)nexthop_put(asp->nexthop);
 			asp->nexthop = NULL;
 		}
 		if ((pos = rde_get_mp_nexthop(mpp, mplen, aid, asp)) == -1) {
@@ -1361,11 +1352,6 @@ rde_update_dispatch(struct imsg *imsg)
 
 done:
 	if (attrpath_len != 0) {
-		/* unlock the previously locked entry */
-		if (asp->nexthop) {
-			asp->nexthop->refcnt--;
-			(void)nexthop_delete(asp->nexthop);
-		}
 		/* free allocated attribute memory that is no longer used */
 		path_put(asp);
 	}
@@ -1569,12 +1555,6 @@ bad_flags:
 			return (-1);
 		}
 		a->nexthop = nexthop_get(&nexthop);
-		/*
-		 * lock the nexthop because it is not yet linked else
-		 * withdraws may remove this nexthop which in turn would
-		 * cause a use after free error.
-		 */
-		a->nexthop->refcnt++;
 		break;
 	case ATTR_MED:
 		if (attr_len != 4)
@@ -1908,12 +1888,6 @@ rde_get_mp_nexthop(u_char *data, u_int16_t len, u_int8_t aid,
 	}
 
 	asp->nexthop = nexthop_get(&nexthop);
-	/*
-	 * lock the nexthop because it is not yet linked else
-	 * withdraws may remove this nexthop which in turn would
-	 * cause a use after free error.
-	 */
-	asp->nexthop->refcnt++;
 
 	/* ignore reserved (old SNPA) field as per RFC4760 */
 	totlen += nhlen + 1;
diff --git a/usr.sbin/bgpd/rde.h b/usr.sbin/bgpd/rde.h
index 39a2359f499..7b23453c2ab 100644
--- a/usr.sbin/bgpd/rde.h
+++ b/usr.sbin/bgpd/rde.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rde.h,v 1.169 2018/06/21 17:26:16 claudio Exp $ */
+/*	$OpenBSD: rde.h,v 1.170 2018/06/25 14:28:33 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Claudio Jeker <claudio@openbsd.org> and
@@ -224,7 +224,7 @@ struct nexthop {
 	 */
 	u_int32_t		costs;
 #endif
-	int			refcnt;	/* filterset reference counter */
+	int			refcnt;
 	enum nexthop_state	state;
 	u_int8_t		nexthop_netlen;
 	u_int8_t		flags;
@@ -506,13 +506,14 @@ prefix_peer(struct prefix *p)
 
 void		 nexthop_init(u_int32_t);
 void		 nexthop_shutdown(void);
-void		 nexthop_modify(struct rde_aspath *, struct bgpd_addr *,
+void		 nexthop_modify(struct rde_aspath *, struct nexthop *,
 		     enum action_types, u_int8_t);
 void		 nexthop_link(struct rde_aspath *);
 void		 nexthop_unlink(struct rde_aspath *);
-int		 nexthop_delete(struct nexthop *);
 void		 nexthop_update(struct kroute_nexthop *);
 struct nexthop	*nexthop_get(struct bgpd_addr *);
+struct nexthop	*nexthop_ref(struct nexthop *);
+int		 nexthop_put(struct nexthop *);
 int		 nexthop_compare(struct nexthop *, struct nexthop *);
 
 /* rde_update.c */
diff --git a/usr.sbin/bgpd/rde_filter.c b/usr.sbin/bgpd/rde_filter.c
index 129a6dd10c8..4766685e2ac 100644
--- a/usr.sbin/bgpd/rde_filter.c
+++ b/usr.sbin/bgpd/rde_filter.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rde_filter.c,v 1.88 2018/06/21 17:28:02 claudio Exp $ */
+/*	$OpenBSD: rde_filter.c,v 1.89 2018/06/25 14:28:33 claudio Exp $ */
 
 /*
  * Copyright (c) 2004 Claudio Jeker <claudio@openbsd.org>
@@ -130,8 +130,7 @@ rde_apply_set(struct rde_aspath *asp, struct filter_set_head *sh,
 		case ACTION_SET_NEXTHOP_BLACKHOLE:
 		case ACTION_SET_NEXTHOP_NOMODIFY:
 		case ACTION_SET_NEXTHOP_SELF:
-			nexthop_modify(asp, &set->action.nexthop, set->type,
-			    aid);
+			nexthop_modify(asp, set->action.nh, set->type, aid);
 			break;
 		case ACTION_SET_COMMUNITY:
 			switch (set->action.community.as) {
@@ -607,7 +606,6 @@ void
 filterset_free(struct filter_set_head *sh)
 {
 	struct filter_set	*s;
-	struct nexthop		*nh;
 
 	if (sh == NULL)
 		return;
@@ -619,11 +617,8 @@ filterset_free(struct filter_set_head *sh)
 		else if (s->type == ACTION_PFTABLE_ID)
 			pftable_unref(s->action.id);
 		else if (s->type == ACTION_SET_NEXTHOP &&
-		    bgpd_process == PROC_RDE) {
-			nh = nexthop_get(&s->action.nexthop);
-			--nh->refcnt;
-			(void)nexthop_delete(nh);
-		}
+		    bgpd_process == PROC_RDE)
+			nexthop_put(s->action.nh);
 		free(s);
 	}
 }
diff --git a/usr.sbin/bgpd/rde_rib.c b/usr.sbin/bgpd/rde_rib.c
index 0483e67d4d7..69a227b0b09 100644
--- a/usr.sbin/bgpd/rde_rib.c
+++ b/usr.sbin/bgpd/rde_rib.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rde_rib.c,v 1.160 2018/06/21 17:26:16 claudio Exp $ */
+/*	$OpenBSD: rde_rib.c,v 1.161 2018/06/25 14:28:33 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Claudio Jeker <claudio@openbsd.org>
@@ -607,7 +607,6 @@ path_destroy(struct rde_aspath *asp)
 	LIST_REMOVE(asp, path_l);
 	TAILQ_REMOVE(&asp->peer->path_h, asp, peer_l);
 	asp->peer = NULL;
-	asp->nexthop = NULL;
 	asp->flags &= ~F_ATTR_LINKED;
 
 	path_put(asp);
@@ -654,7 +653,7 @@ path_copy(struct rde_aspath *asp)
 		nasp->aspath->refcnt++;
 		rdemem.aspath_refs++;
 	}
-	nasp->nexthop = asp->nexthop;
+	nasp->nexthop = nexthop_ref(asp->nexthop);
 	nasp->med = asp->med;
 	nasp->lpref = asp->lpref;
 	nasp->weight = asp->weight;
@@ -705,6 +704,7 @@ path_put(struct rde_aspath *asp)
 	rtlabel_unref(asp->rtlabelid);
 	pftable_unref(asp->pftableid);
 	aspath_put(asp->aspath);
+	nexthop_put(asp->nexthop);
 	attr_freeall(asp);
 	rdemem.path_cnt--;
 	free(asp);
@@ -1161,10 +1161,14 @@ nexthop_shutdown(void)
 		    nh != NULL; nh = nnh) {
 			nnh = LIST_NEXT(nh, nexthop_l);
 			nh->state = NEXTHOP_UNREACH;
-			(void)nexthop_delete(nh);
+			(void)nexthop_put(nh);
+		}
+		if (!LIST_EMPTY(&nexthoptable.nexthop_hashtbl[i])) {
+			nh = LIST_FIRST(&nexthoptable.nexthop_hashtbl[i]);
+			log_warnx("nexthop_shutdown: non-free table, "
+			    "nexthop %s refcnt %d",
+			    log_addr(&nh->exit_nexthop), nh->refcnt);
 		}
-		if (!LIST_EMPTY(&nexthoptable.nexthop_hashtbl[i]))
-			log_warnx("nexthop_shutdown: non-free table");
 	}
 
 	free(nexthoptable.nexthop_hashtbl);
@@ -1185,6 +1189,7 @@ nexthop_update(struct kroute_nexthop *msg)
 	}
 
 	oldstate = nh->state;
+
 	if (msg->valid)
 		nh->state = NEXTHOP_REACH;
 	else
@@ -1202,9 +1207,10 @@ nexthop_update(struct kroute_nexthop *msg)
 	    sizeof(nh->nexthop_net));
 	nh->nexthop_netlen = msg->netlen;
 
-	if (nexthop_delete(nh))
-		/* nexthop no longer used */
-		return;
+	if (oldstate == NEXTHOP_LOOKUP)
+		/* drop reference which was hold during the lookup */
+		if (nexthop_put(nh))
+			return;
 
 	if (rde_noevaluate())
 		/*
@@ -1219,12 +1225,13 @@ nexthop_update(struct kroute_nexthop *msg)
 }
 
 void
-nexthop_modify(struct rde_aspath *asp, struct bgpd_addr *nexthop,
+nexthop_modify(struct rde_aspath *asp, struct nexthop *nh,
     enum action_types type, u_int8_t aid)
 {
-	struct nexthop	*nh;
+	if (asp->flags & F_ATTR_LINKED)
+		fatalx("nexthop_modify: trying to modify linked asp");
 
-	if (type == ACTION_SET_NEXTHOP && aid != nexthop->aid)
+	if (type == ACTION_SET_NEXTHOP && aid != nh->exit_nexthop.aid)
 		return;
 
 	asp->flags &= ~F_NEXTHOP_MASK;
@@ -1242,12 +1249,8 @@ nexthop_modify(struct rde_aspath *asp, struct bgpd_addr *nexthop,
 		asp->flags |= F_NEXTHOP_SELF;
 		break;
 	case ACTION_SET_NEXTHOP:
-		nh = nexthop_get(nexthop);
-		if (asp->flags & F_ATTR_LINKED)
-			nexthop_unlink(asp);
-		asp->nexthop = nh;
-		if (asp->flags & F_ATTR_LINKED)
-			nexthop_link(asp);
+		nexthop_put(asp->nexthop);
+		asp->nexthop = nexthop_ref(nh);
 		break;
 	default:
 		break;
@@ -1273,30 +1276,11 @@ nexthop_unlink(struct rde_aspath *asp)
 
 	LIST_REMOVE(asp, nexthop_l);
 
-	/* see if list is empty */
+	/* remove reference to nexthop */
 	nh = asp->nexthop;
 	asp->nexthop = NULL;
 
-	(void)nexthop_delete(nh);
-}
-
-int
-nexthop_delete(struct nexthop *nh)
-{
-	/* nexthop still used by some other aspath */
-	if (!LIST_EMPTY(&nh->path_h))
-		return (0);
-
-	/* either pinned or in a state where it may not be deleted */
-	if (nh->refcnt > 0 || nh->state == NEXTHOP_LOOKUP)
-		return (0);
-
-	LIST_REMOVE(nh, nexthop_l);
-	rde_send_nexthop(&nh->exit_nexthop, 0);
-
-	rdemem.nexthop_cnt--;
-	free(nh);
-	return (1);
+	(void)nexthop_put(nh);
 }
 
 struct nexthop *
@@ -1313,6 +1297,7 @@ nexthop_get(struct bgpd_addr *nexthop)
 
 		LIST_INIT(&nh->path_h);
 		nh->state = NEXTHOP_LOOKUP;
+		nexthop_ref(nh);	/* take reference for lookup */
 		nh->exit_nexthop = *nexthop;
 		LIST_INSERT_HEAD(nexthop_hash(nexthop), nh,
 		    nexthop_l);
@@ -1320,7 +1305,36 @@ nexthop_get(struct bgpd_addr *nexthop)
 		rde_send_nexthop(&nh->exit_nexthop, 1);
 	}
 
-	return (nh);
+	return nexthop_ref(nh);
+}
+
+struct nexthop *
+nexthop_ref(struct nexthop *nexthop)
+{
+	if (nexthop)
+		nexthop->refcnt++;
+	return (nexthop);
+}
+
+int
+nexthop_put(struct nexthop *nh)
+{
+	if (nh == NULL)
+		return (0);
+
+	if (--nh->refcnt > 0)
+		return (0);
+
+	/* sanity check */
+	if (!LIST_EMPTY(&nh->path_h) || nh->state == NEXTHOP_LOOKUP)
+		fatalx("nexthop_put: refcnt error");
+
+	LIST_REMOVE(nh, nexthop_l);
+	rde_send_nexthop(&nh->exit_nexthop, 0);
+
+	rdemem.nexthop_cnt--;
+	free(nh);
+	return (1);
 }
 
 int