Prefixes or actually pathes that would cause a rooting loops should not be

dropped when parsed but instead be added to the RIB marked as not eligible.
So the decision process does not pick them up as a valid route.
Tested and some ideas by Tony Sarendal (tony (at) polarcap (dot) org)

4 files changed, 33 insertions, 22 deletions

diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c
index 5f1ed2fb42a..0fd7edb5c84 100644
--- a/usr.sbin/bgpd/rde.c
+++ b/usr.sbin/bgpd/rde.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rde.c,v 1.228 2007/09/16 15:20:50 claudio Exp $ */
+/*	$OpenBSD: rde.c,v 1.229 2007/11/27 01:13:54 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -59,7 +59,7 @@ void		 rde_update_log(const char *,
 		     const struct rde_peer *, const struct bgpd_addr *,
 		     const struct bgpd_addr *, u_int8_t);
 void		 rde_as4byte_fixup(struct rde_peer *, struct rde_aspath *);
-int		 rde_reflector(struct rde_peer *, struct rde_aspath *);
+void		 rde_reflector(struct rde_peer *, struct rde_aspath *);
 
 void		 rde_dump_rib_as(struct prefix *, struct rde_aspath *,pid_t,
 		     int);
@@ -806,10 +806,7 @@ rde_update_dispatch(struct imsg *imsg)
 				goto done;
 			}
 
-		if (rde_reflector(peer, asp) != 1) {
-			error = 0;
-			goto done;
-		}
+		rde_reflector(peer, asp);
 	}
 
 	p = imsg->data;
@@ -920,10 +917,8 @@ rde_update_dispatch(struct imsg *imsg)
 	p += 2 + attrpath_len;
 
 	/* aspath needs to be loop free nota bene this is not a hard error */
-	if (peer->conf.ebgp && !aspath_loopfree(asp->aspath, conf->as)) {
-		error = 0;
-		goto done;
-	}
+	if (peer->conf.ebgp && !aspath_loopfree(asp->aspath, conf->as))
+		asp->flags |= F_ATTR_LOOP;
 
 	/* parse nlri prefix */
 	while (nlri_len > 0) {
@@ -1621,7 +1616,7 @@ rde_as4byte_fixup(struct rde_peer *peer, struct rde_aspath *a)
 /*
  * route reflector helper function
  */
-int
+void
 rde_reflector(struct rde_peer *peer, struct rde_aspath *asp)
 {
 	struct attr	*a;
@@ -1631,9 +1626,11 @@ rde_reflector(struct rde_peer *peer, struct rde_aspath *asp)
 
 	/* check for originator id if eq router_id drop */
 	if ((a = attr_optget(asp, ATTR_ORIGINATOR_ID)) != NULL) {
-		if (memcmp(&conf->bgpid, a->data, sizeof(conf->bgpid)) == 0)
+		if (memcmp(&conf->bgpid, a->data, sizeof(conf->bgpid)) == 0) {
 			/* this is coming from myself */
-			return (0);
+			asp->flags |= F_ATTR_LOOP;
+			return;
+		}
 	} else if (conf->flags & BGPD_FLAG_REFLECTOR) {
 		if (peer->conf.ebgp == 0)
 			id = htonl(peer->remote_bgpid);
@@ -1651,8 +1648,10 @@ rde_reflector(struct rde_peer *peer, struct rde_aspath *asp)
 			    len += sizeof(conf->clusterid))
 				/* check if coming from my cluster */
 				if (memcmp(&conf->clusterid, a->data + len,
-				    sizeof(conf->clusterid)) == 0)
-					return (0);
+				    sizeof(conf->clusterid)) == 0) {
+					asp->flags |= F_ATTR_LOOP;
+					return;
+				}
 
 			/* prepend own clusterid by replacing attribute */
 			len = a->len + sizeof(conf->clusterid);
@@ -1671,7 +1670,6 @@ rde_reflector(struct rde_peer *peer, struct rde_aspath *asp)
 		    &conf->clusterid, sizeof(conf->clusterid)) == -1)
 			fatalx("attr_optadd failed but impossible");
 	}
-	return (1);
 }
 
 /*
@@ -1719,8 +1717,10 @@ rde_dump_rib_as(struct prefix *p, struct rde_aspath *asp, pid_t pid, int flags)
 		rib.flags |= F_RIB_INTERNAL;
 	if (asp->flags & F_PREFIX_ANNOUNCED)
 		rib.flags |= F_RIB_ANNOUNCE;
-	if (asp->nexthop == NULL || asp->nexthop->state == NEXTHOP_REACH)
+	if (asp->nexthop != NULL && asp->nexthop->state == NEXTHOP_REACH)
 		rib.flags |= F_RIB_ELIGIBLE;
+	if (asp->flags & F_ATTR_LOOP)
+		rib.flags &= ~F_RIB_ELIGIBLE;
 	rib.aspath_len = aspath_length(asp->aspath);
 
 	if ((wbuf = imsg_create(ibuf_se_ctl, IMSG_CTL_SHOW_RIB, 0, pid,
diff --git a/usr.sbin/bgpd/rde.h b/usr.sbin/bgpd/rde.h
index 596c91b5868..78dd73d1cf0 100644
--- a/usr.sbin/bgpd/rde.h
+++ b/usr.sbin/bgpd/rde.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rde.h,v 1.100 2007/06/01 04:17:30 claudio Exp $ */
+/*	$OpenBSD: rde.h,v 1.101 2007/11/27 01:13:54 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Claudio Jeker <claudio@openbsd.org> and
@@ -154,6 +154,7 @@ LIST_HEAD(prefix_head, prefix);
 #define	F_ATTR_MP_REACH		0x00040
 #define	F_ATTR_MP_UNREACH	0x00080
 #define	F_ATTR_AS4BYTE_NEW	0x00100	/* NEW_ASPATH or NEW_AGGREGATOR */
+#define	F_ATTR_LOOP		0x00200 /* path would cause a route loop */
 #define	F_PREFIX_ANNOUNCED	0x01000
 #define	F_NEXTHOP_REJECT	0x02000
 #define	F_NEXTHOP_BLACKHOLE	0x04000
diff --git a/usr.sbin/bgpd/rde_decide.c b/usr.sbin/bgpd/rde_decide.c
index 4fab23b48dc..802e69ec2d4 100644
--- a/usr.sbin/bgpd/rde_decide.c
+++ b/usr.sbin/bgpd/rde_decide.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rde_decide.c,v 1.48 2007/05/11 11:27:59 claudio Exp $ */
+/*	$OpenBSD: rde_decide.c,v 1.49 2007/11/27 01:13:54 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Claudio Jeker <claudio@openbsd.org>
@@ -120,6 +120,12 @@ prefix_cmp(struct prefix *p1, struct prefix *p2)
 		return (-1);
 	if (!(p2->flags & F_LOCAL))
 		return (1);
+ 
+	/* only loop free pathes are eligible */
+	if (p1->flags & F_ATTR_LOOP)
+		return (-1);
+	if (p2->flags & F_ATTR_LOOP)
+		return (1);
 
 	asp1 = p1->aspath;
 	asp2 = p2->aspath;
@@ -239,8 +245,9 @@ prefix_evaluate(struct prefix *p, struct pt_entry *pte)
 
 	xp = LIST_FIRST(&pte->prefix_h);
 	if (xp == NULL || !(xp->flags & F_LOCAL) ||
-	    (xp->aspath->nexthop != NULL && xp->aspath->nexthop->state !=
-	    NEXTHOP_REACH))
+	    xp->aspath->flags & F_ATTR_LOOP ||
+	    (xp->aspath->nexthop != NULL &&
+	    xp->aspath->nexthop->state != NEXTHOP_REACH))
 		/* xp is ineligible */
 		xp = NULL;
 
diff --git a/usr.sbin/bgpd/rde_update.c b/usr.sbin/bgpd/rde_update.c
index 6d486b5b9c2..b6b3e8fdae3 100644
--- a/usr.sbin/bgpd/rde_update.c
+++ b/usr.sbin/bgpd/rde_update.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rde_update.c,v 1.60 2007/05/31 04:27:00 claudio Exp $ */
+/*	$OpenBSD: rde_update.c,v 1.61 2007/11/27 01:13:54 claudio Exp $ */
 
 /*
  * Copyright (c) 2004 Claudio Jeker <claudio@openbsd.org>
@@ -270,6 +270,9 @@ up_test_update(struct rde_peer *peer, struct prefix *p)
 		/* Do not send routes back to sender */
 		return (0);
 
+	if (p->aspath->flags & F_ATTR_LOOP)
+		fatalx("try to send out a looped path");
+
 	pt_getaddr(p->prefix, &addr);
 	switch (addr.af) {
 	case AF_INET: