SEO
vBulletin Search Engine Optimization
| |||||||
| Register | FAQ | Members List | Calendar | Search | Today's Posts | Mark Forums Read |
02-18-2008, 09:34 AM
| ||||
| ||||
 Re: bgpd ebgp loops On 11/24/07, Claudio Jeker <cjeker@diehard.n-r-g.com> wrote: > > On Fri, Nov 23, 2007 at 01:11:02AM +0100, Tony Sarendal wrote: > > New diff that also fixes the same problem for route-reflecting. > > > > I did not change up_test_update() even though the assumption > > there is that a looped prefixes should be blocked outgoing. > > > > up_test_update() should never be called with a looped path. I just added a > fatalx() in up_test_update() as safty net. > > The rde.c bits looked ok and flag all possible loops but ... > > > Index: rde_rib.c > > ================================================== ================= > > RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v > > retrieving revision 1.96 > > diff -u -r1.96 rde_rib.c > > --- rde_rib.c 1 Jun 2007 04:17:30 -0000 1.96 > > +++ rde_rib.c 23 Nov 2007 00:05:33 -0000 > > @@ -90,6 +90,11 @@ > > struct rde_aspath *asp; > > struct prefix *p, *oldp = NULL; > > > > + if (nasp->flags & F_IMPLICIT_WITHDRAW) { > > + prefix_remove(peer, prefix, prefixlen, flags); > > + return; > > + } > > + > > if (flags & F_LOCAL) { > > rde_send_pftable(nasp->pftableid, prefix, prefixlen, 0); > > rde_send_pftable_commit(); > > > > I don't like this implicit withdraw. It removes a path from the RIB that > should be updated instead and then set to invalid. At least I like to know > and see loops sent by other routers instead of dropping the prefixes. > > My first diff to solve this issue had a stupid stupid misstake (don't > check the wrong flags field). Here is a updated diff that should give the > same result but show the prefix in bgpctl show rib output as inactive or > ineligible. > -- > :wq Claudio > > Index: rde.c > ================================================== ================= > RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v > retrieving revision 1.228 > diff -u -p -r1.228 rde.c > --- rde.c 16 Sep 2007 15:20:50 -0000 1.228 > +++ rde.c 24 Nov 2007 16:10:26 -0000 > @@ -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, > /* > * 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, str > > /* 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, str > 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, str > &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 > 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, > Index: rde.h > ================================================== ================= > RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v > retrieving revision 1.100 > diff -u -p -r1.100 rde.h > --- rde.h 1 Jun 2007 04:17:30 -0000 1.100 > +++ rde.h 24 Nov 2007 16:10:26 -0000 > @@ -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 > Index: rde_decide.c > ================================================== ================= > RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v > retrieving revision 1.48 > diff -u -p -r1.48 rde_decide.c > --- rde_decide.c 11 May 2007 11:27:59 -0000 1.48 > +++ rde_decide.c 24 Nov 2007 16:10:26 -0000 > @@ -120,6 +120,12 @@ prefix_cmp(struct prefix *p1, struct pre > 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 > > 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; > > Index: rde_update.c > ================================================== ================= > RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v > retrieving revision 1.60 > diff -u -p -r1.60 rde_update.c > --- rde_update.c 31 May 2007 04:27:00 -0000 1.60 > +++ rde_update.c 24 Nov 2007 16:10:26 -0000 > @@ -270,6 +270,9 @@ up_test_update(struct rde_peer *peer, st > /* 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: > Tested this in the scenarios that failed before, both ebgp and ibgp-rr. It passed in both cases. The looped prefixes also show up when I do a "show rib". /Tony      |
Linear Mode
