On Wed, Feb 24, 2016 at 02:53:19AM -0800, Martin Winter wrote: > On 24 Feb 2016, at 1:07, David Lamparter wrote: > > > On Wed, Feb 24, 2016 at 01:00:03AM -0800, [email protected] wrote: > >> Continous Integration Result: FAILED > > [...] > >> Tested on top of Git : eae18d1 (as of 20151209.135437 UTC) > > > > As mentioned in the original mail, this patchset applies on top of > > "lib: fix vrf_bitmap leak in zclient_free()", not master. The patchset > > has passed a manual run (though I did some style polishing afterwards, I > > hope I didn't accidentally break anything there.) > > Sorry, my CI system doesn’t speak english. > > Resubmitted to be on top of the specified Git > > But patching seemed to fail. (01/10 worked, 02/10 failed) > See CI email.
-- CI email: # Tested on top of Git : f739363 (as of 20160218.155415 UTC) # CI System Testrun URL: https://ci1.netdef.org/browse/QUAGGA-QPWORK-243/ commit f739363231b45d1085b0f243a43a846b465c4f94 Author: David Lamparter <[email protected]> AuthorDate: Wed Dec 16 19:38:23 2015 +0100 Commit: Paul Jakma <[email protected]> CommitDate: Thu Feb 18 15:54:15 2016 +0000 lib: fix vrf_bitmap leak in zclient_free() -- My base commit was: commit f48d5b99571d7ed956d4767047d07a245bec05c1 Author: David Lamparter <[email protected]> AuthorDate: Wed Dec 16 19:38:23 2015 +0100 Commit: Donald Sharp <[email protected]> CommitDate: Wed Feb 10 19:48:02 2016 -0500 lib: fix vrf_bitmap leak in zclient_free() > Let me guess: The rebase changed things? Well... see below. Not even gonna try and guess why these changes had to be secretly snuck into a history rewrite... $ git diff f739363..f48d5b9 diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 692d361..deb24a1 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -40,7 +40,6 @@ decode_rd_type (u_char *pnt) v = ((u_int16_t) *pnt++ << 8); v |= (u_int16_t) *pnt; - return v; } @@ -93,8 +92,8 @@ decode_rd_ip (u_char *pnt, struct rd_ip *rd_ip) } int -bgp_nlri_parse_vpn (afi_t afi, struct peer *peer, struct attr *attr, - struct bgp_nlri *packet, int withdraw) +bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr, + struct bgp_nlri *packet) { u_char *pnt; u_char *lim; @@ -125,13 +124,7 @@ bgp_nlri_parse_vpn (afi_t afi, struct peer *peer, struct attr *attr, /* Fetch prefix length. */ prefixlen = *pnt++; - p.family = afi2family(afi); - if (p.family == 0) - { - /* bad afi, shouldn't happen */ - zlog_warn("%s: bad afi %d, dropping incoming route", __func__, afi); - continue; - } + p.family = AF_INET; psize = PSIZE (prefixlen); if (prefixlen < 88) @@ -174,16 +167,12 @@ bgp_nlri_parse_vpn (afi_t afi, struct peer *peer, struct attr *attr, if (pnt + psize > lim) return -1; - if (!withdraw) - { - bgp_update (peer, &p, attr, afi, SAFI_MPLS_VPN, - ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt, 0); - } + if (attr) + bgp_update (peer, &p, attr, packet->afi, SAFI_MPLS_VPN, + ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt, 0); else - { - bgp_withdraw (peer, &p, attr, afi, SAFI_MPLS_VPN, - ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt); - } + bgp_withdraw (peer, &p, attr, packet->afi, SAFI_MPLS_VPN, + ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt); } /* Packet length consistency check. */ diff --git a/bgpd/bgp_mplsvpn.h b/bgpd/bgp_mplsvpn.h index b2a286c..3299b9c 100644 --- a/bgpd/bgp_mplsvpn.h +++ b/bgpd/bgp_mplsvpn.h @@ -42,8 +42,7 @@ struct rd_ip }; extern void bgp_mplsvpn_init (void); -extern int bgp_nlri_parse_vpn (afi_t, struct peer *, struct attr *, - struct bgp_nlri *, int withdraw); +extern int bgp_nlri_parse_vpn (struct peer *, struct attr *, struct bgp_nlri *); extern u_int32_t decode_label (u_char *); extern int str2prefix_rd (const char *, struct prefix_rd *); extern int str2tag (const char *, u_char *); diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index 4375be9..2406814 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -488,7 +488,7 @@ bgp_scan (afi_t afi, safi_t safi) } } if (rn->info) - bgp_process (bgp, rn, afi, SAFI_UNICAST); + bgp_process (bgp, rn, afi, SAFI_UNICAST); } /* Flash old cache. */ diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index ca38150..7b8b657 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -440,7 +440,7 @@ static const size_t cap_minsizes[] = [CAPABILITY_CODE_MP] = CAPABILITY_CODE_MP_LEN, [CAPABILITY_CODE_REFRESH] = CAPABILITY_CODE_REFRESH_LEN, [CAPABILITY_CODE_ORF] = CAPABILITY_CODE_ORF_LEN, - [CAPABILITY_CODE_RESTART] = 6, + [CAPABILITY_CODE_RESTART] = CAPABILITY_CODE_RESTART_LEN, [CAPABILITY_CODE_AS4] = CAPABILITY_CODE_AS4_LEN, [CAPABILITY_CODE_DYNAMIC] = CAPABILITY_CODE_DYNAMIC_LEN, [CAPABILITY_CODE_REFRESH_OLD] = CAPABILITY_CODE_REFRESH_LEN, diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 6b4e4d7..26e4d8f 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1895,12 +1895,12 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP && mp_update.safi == SAFI_MPLS_LABELED_VPN) - bgp_nlri_parse_vpn (mp_update.afi, peer, &attr, &mp_update, 0); + bgp_nlri_parse_vpn (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP && mp_withdraw.safi == SAFI_MPLS_LABELED_VPN) - bgp_nlri_parse_vpn (mp_withdraw.afi, peer, &attr, &mp_withdraw, 1); + bgp_nlri_parse_vpn (peer, NULL, &mp_withdraw); if (! withdraw_len && mp_withdraw.afi == AFI_IP @@ -1919,12 +1919,12 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP6 && mp_update.safi == SAFI_MPLS_LABELED_VPN) - bgp_nlri_parse_vpn (mp_update.afi, peer, &attr, &mp_update, 0); + bgp_nlri_parse_vpn (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP6 && mp_withdraw.safi == SAFI_MPLS_LABELED_VPN) - bgp_nlri_parse_vpn (mp_withdraw.afi, peer, &attr, &mp_withdraw, 1); + bgp_nlri_parse_vpn (peer, NULL, &mp_withdraw); if (! withdraw_len && mp_withdraw.afi == AFI_IP6 diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index b81d41e..f0c49ca 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -1317,9 +1317,9 @@ bgp_best_selection (struct bgp *bgp, struct bgp_node *rn, struct bgp_info *nextri = NULL; int cmpret, do_mpath; struct list mp_list; - + result->old = result->new = NULL; - + if (rn->info == NULL) { char buf[PREFIX_STRLEN]; @@ -1328,7 +1328,7 @@ bgp_best_selection (struct bgp *bgp, struct bgp_node *rn, prefix2str (&(bgp_node_to_rnode (rn)->p), buf, sizeof(buf))); return; } - + bgp_mp_list_init (&mp_list); do_mpath = bgp_mpath_is_configured (bgp, afi, safi); @@ -1449,7 +1449,6 @@ bgp_best_selection (struct bgp *bgp, struct bgp_node *rn, bgp_mp_list_add (&mp_list, ri); } } - if (!bgp_flag_check (bgp, BGP_FLAG_DETERMINISTIC_MED)) bgp_info_mpath_update (rn, new_select, old_select, &mp_list, afi, safi); @@ -1709,19 +1708,20 @@ bgp_process (struct bgp *bgp, struct bgp_node *rn, afi_t afi, safi_t safi) /* already scheduled for processing? */ if (CHECK_FLAG (rn->flags, BGP_NODE_PROCESS_SCHEDULED)) return; - + if (rn->info == NULL) { /* XXX: Perhaps remove before next release, after we've flushed out * any obvious cases */ + assert (rn->info != NULL); char buf[PREFIX_STRLEN]; zlog_warn ("%s: Called for route_node %s with no routing entries!", __func__, prefix2str (&(bgp_node_to_rnode (rn)->p), buf, sizeof(buf))); - assert (rn->info != NULL); + return; } - + if ( (bm->process_main_queue == NULL) || (bm->process_rsclient_queue == NULL) ) bgp_process_queue_init (); diff --git a/doc/bgpd.texi b/doc/bgpd.texi index 9ced719..c3f7b5b 100644 --- a/doc/bgpd.texi +++ b/doc/bgpd.texi @@ -1,6 +1,7 @@ @c -*-texinfo-*- @c This is part of the Quagga Manual. @c @value{COPYRIGHT_STR} +@c Copyright @copyright{} 2015 Hewlett Packard Enterprise Development LP @c See file quagga.texi for copying conditions. @node BGP @chapter BGP @@ -57,7 +58,7 @@ When program terminates, retain BGP routes added by zebra. @item -l @itemx --listenon -Specify a specific IP address for bgpd to listen on, rather than its +Specify a specific IP address for bgpd to listen on, rather than its default of INADDR_ANY / IN6ADDR_ANY. This can be useful to constrain bgpd to an internal address, or to run multiple bgpd processes on one host. @@ -125,11 +126,11 @@ prefer higher local preference routes to lower. Prefer local routes (statics, aggregates, redistributed) to received routes. @item 4. AS path length check -Prefer shortest hop-count AS_PATHs. +Prefer shortest hop-count AS_PATHs. @item 5. Origin check Prefer the lowest origin type route. That is, prefer IGP origin routes to -EGP, to Incomplete routes. +EGP, to Incomplete routes. @item 6. MED check Where routes with a MED were received from the same AS, @@ -234,6 +235,13 @@ is not recommended nowadays, see @uref{http://www.ripe.net/ripe/docs/ripe-378,,R @node BGP MED @section BGP MED +The BGP MED (Multi_Exit_Discriminator) attribute has properties which can +cause subtle convergence problems in BGP. These properties and problems +have proven to be hard to understand, at least historically, and may still +not be widely understood. The following attempts to collect together and +present what is known about MED, to help operators and Quagga users in +designing and configuring their networks. + The BGP @acronym{MED, Multi_Exit_Discriminator} attribute is intended to allow one AS to indicate its preferences for its ingress points to another AS. The MED attribute will not be propagated on to another AS by the @@ -258,6 +266,23 @@ routes received from different neighbouring ASes. In Quagga this is done by comparing the neighbouring, left-most AS in the received AS_PATHs of the routes and only comparing MED if those are the same. +@c TeXInfo uses the old, non-UTF-8 capable, pdftex, and so +@c doesn't render TeX the unicode precedes character correctly in PDF, etc. +@c Using a TeX code on the other hand doesn't work for non-TeX outputs +@c (plaintext, e.g.). So, use an output-conditional macro. + +@iftex +@macro mprec{} +@math{\\prec} +@end macro +@end iftex + +@ifnottex +@macro mprec{} +@math{≺} +@end macro +@end ifnottex + Unfortunately, this behaviour of MED, of sometimes being compared across routes and sometimes not, depending on the properties of those other routes, means MED can cause the order of preference over all the routes to be @@ -266,15 +291,18 @@ is preferred to C, then a well-defined order should mean the preference is transitive (in the sense of orders @footnote{For some set of objects to have an order, there @emph{must} be some binary ordering relation that is defined for @emph{every} combination of those objects, and that relation @emph{must} -be transitive. I.e.@:, if the relation operator is @math{\prec}, and if -@math{a \prec b} and @math{b \prec c} then that relation must carry over and -it @emph{must} be that @math{a \prec c} for the objects to have an order. -The ordering relation may allow for equality, i.e. if @math{a \prec b} and -@math{b \prec a} may both be true and this implies that @math{a} and -@math{b} are equal in the order and not distinguished by it, in which the -set has a partial order. Otherwise, if there is an order, all the objects -have a distinct place in the order and the set has a total order.}) and that -A would be preferred to C. +be transitive. I.e.@:, if the relation operator is @mprec{}, and if +a @mprec{} b and b @mprec{} c then that relation must carry over +and it @emph{must} be that a @mprec{} c for the objects to have an +order. The ordering relation may allow for equality, i.e. +a @mprec{} b and b @mprec{} a may both be true amd imply that +a and b are equal in the order and not distinguished by it, in +which case the set has a partial order. Otherwise, if there is an order, +all the objects have a distinct place in the order and the set has a total +order.}) and that A would be preferred to C. + +@c No longer need the precedes character definition +@unmacro mprec However, when MED is involved this need not be the case. With MED it is possible that C is actually preferred over A. So A is preferred to B, B is @@ -285,12 +313,13 @@ deterministically preferred route, but there need not be any way to arrange them into any order of preference. With unmodified MED, the order of preference of routes literally becomes undefined. -That MED can induce non-transitive preferences over routes can cause issues. +That MED can induce non-transitive preferences over routes can cause issues. Firstly, it may be perceived to cause routing table churn locally at -speakers; secondly it may cause routing instability in iBGP topologies, -where sets of speakers continually oscillate between different paths. +speakers; secondly, and more seriously, it may cause routing instability in +iBGP topologies, where sets of speakers continually oscillate between +different paths. -The first issue arises from how speakers often implement routing decisions. +The first issue arises from how speakers often implement routing decisions. Though BGP defines a selection process that will deterministically select the same route as best at any given speaker, even with MED, that process requires evaluating all routes together. For performance and ease of @@ -299,14 +328,14 @@ pair-wise fashion instead. Given there is no well-defined order when MED is involved, the best route that will be chosen becomes subject to implementation details, such as the order the routes are stored in. That may be (locally) non-deterministic, e.g.@: it may be the order the routes -were received in. +were received in. This indeterminism may be considered undesirable, though it need not cause problems. It may mean additional routing churn is perceived, as sometimes more updates may be produced than at other times in reaction to some event . This first issue can be fixed with a more deterministic route selection that -ensures routes are ordered by the neighbouring AS during selection. +ensures routes are ordered by the neighbouring AS during selection. @xref{bgp deterministic-med}. This may reduce the number of updates as routes are received, and may in some cases reduce routing churn. Though, it could equally deterministically produce the largest possible set of updates @@ -315,23 +344,24 @@ in response to the most common sequence of received updates. A deterministic order of evaluation tends to imply an additional overhead of sorting over any set of n routes to a destination. The implementation of deterministic MED in Quagga scales significantly worse than most sorting -algorithms at present, with the number of paths to a given destination. +algorithms at present, with the number of paths to a given destination. That number is often low enough to not cause any issues, but where there are many paths, the deterministic comparison may quickly become increasingly expensive in terms of CPU. -Deterministic local evaluation can @emph{not} fix the second issue of MED -however. Which is that the non-transitive preference of routes MED can -cause may lead to routing instability or oscillation across multiple +Deterministic local evaluation can @emph{not} fix the second, more major, +issue of MED however. Which is that the non-transitive preference of routes +MED can cause may lead to routing instability or oscillation across multiple speakers in iBGP topologies. This can occur with full-mesh iBGP, but is particularly problematic in non-full-mesh iBGP topologies that further -reduce the routing information known to each This has primarily been -documented with iBGP route-reflection topologies. However, any route-hiding -technologies potentially could also exacerbate oscillation with MED. +reduce the routing information known to each speaker. This has primarily +been documented with iBGP route-reflection topologies. However, any +route-hiding technologies potentially could also exacerbate oscillation with +MED. -The second issue occurs where speakers each have only a subset of routes, +This second issue occurs where speakers each have only a subset of routes, and there are cycles in the preferences between different combinations of -routes - as the undefined order of preference of MED allows, and the routes +routes - as the undefined order of preference of MED allows - and the routes are distributed in a way that causes the BGP speakers to 'chase' those cycles. This can occur even if all speakers use a deterministic order of evaluation in route selection. @@ -351,8 +381,8 @@ at speaker 3. I.e, using ASN:ID:MED to label the speakers: @end example Assuming all other metrics are equal (AS_PATH, ORIGIN, 0 IGP costs), then -based on the RFC4271 decision process speaker 3 will choose X:2 over -Y:2:100, based on the lower ID of 2. Speaker 3 advertises X:2 to speaker 5. +based on the RFC4271 decision process speaker 4 will choose X:2 over +Y:3:100, based on the lower ID of 2. Speaker 4 advertises X:2 to speaker 5. Speaker 5 will continue to prefer Y:1:200 based on the ID, and advertise this to speaker 4. Speaker 4 will now have the full set of routes, and the Y:1:200 it receives from 5 will beat X:2, but when speaker 4 compares @@ -370,28 +400,28 @@ preferences between the routes: @example - X:2 -----------beats------------> Y:3:100 - ^ / - | / - \----beats---- Y:1:200<---beats--/ + /---> X:2 ---beats---> Y:3:100 --\ + | | + | | + \---beats--- Y:1:200 <---beats---/ @end example This particular type of oscillation in full-mesh iBGP topologies can be avoided by speakers preferring already selected, external routes rather than -choosing to update to new a route based on a post-MED metric (e.g. +choosing to update to new a route based on a post-MED metric (e.g. router-ID), at the cost of a non-deterministic selection process. Quagga implements this, as do many other implementations, so long as it is not -overriden by setting @ref{bgp bestpath compare-routerid}, and see also +overridden by setting @ref{bgp bestpath compare-routerid}, and see also @ref{BGP decision process}, . However, more complex and insidious cycles of oscillation are possible with iBGP route-reflection, which are not so easily avoided. These have been -documented in various places. See, e.g., @cite{McPherson, D. and Gill, V. +documented in various places. See, e.g., @cite{McPherson, D. and Gill, V. and Walton, D., "Border Gateway Protocol (BGP) Persistent Route Oscillation Condition", IETF RFC3345}, and @cite{Flavel, A. and M. Roughan, "Stable -and flexible iBGP", ACM SIGCOMM 2009}, and @cite{Griffin, T. and G. Wilfong, -"On the correctness of IBGP configuration", ACM SIGCOMM 2002} for concrete +and flexible iBGP", ACM SIGCOMM 2009}, and @cite{Griffin, T. and G. Wilfong, +"On the correctness of IBGP configuration", ACM SIGCOMM 2002} for concrete examples and further references. There is as of this writing @emph{no} known way to use MED for its original @@ -401,29 +431,29 @@ non-transitive routing preferences it can induce; in general on arbitrary networks. There may be iBGP topology specific ways to reduce the instability risks, -even while using MED, e.g.@: by constraining the reflection topoology and by -tuning IGP costs between route-reflector clusters, see RFC3345 for details. +even while using MED, e.g.@: by constraining the reflection topology and by +tuning IGP costs between route-reflector clusters, see RFC3345 for details. In the near future, the Add-Path extension to BGP may also solve MED oscillation while still allowing MED to be used as intended, by distributing -"best-paths per neighbour AS". However this would be at the cost of -distributing at least as many routes as a full-mesh iBGP would, if not more, +"best-paths per neighbour AS". This would be at the cost of distributing at +least as many routes to all speakers as a full-mesh iBGP would, if not more, while also imposing similar CPU overheads as the "Deterministic MED" feature at each Add-Path reflector. More generally, the instability problems that MED can introduce on more complex, non-full-mesh, iBGP topologies may be avoided either by: -@itemize +@itemize @item Setting @ref{bgp always-compare-med}, however this allows MED to be compared across values set by different neighbour ASes, which may not produce coherent desirable results, of itself. -@item -Effecively ignoring MED by setting MED to the same value (e.g.@: 0) using +@item +Effectively ignoring MED by setting MED to the same value (e.g.@: 0) using @ref{routemap set metric} on all received routes, in combination with -setting @ref{bgp always-compare-med} on all speakers. This is the simplest +setting @ref{bgp always-compare-med} on all speakers. This is the simplest and most performant way to avoid MED oscillation issues, where an AS is happy not to allow neighbours to inject this problematic metric. @@ -432,7 +462,7 @@ not to allow neighbours to inject this problematic metric. As MED is evaluated after the AS_PATH length check, another possible use for MED is for intra-AS steering of routes with equal AS_PATH length, as an extension of the last case above. As MED is evaluated before IGP metric, -this can allow cold-potato routing to be implemented, sending traffic to +this can allow cold-potato routing to be implemented to send traffic to preferred hand-offs with neighbours, rather than the closest hand-off according to the IGP metric. diff --git a/lib/Makefile.am b/lib/Makefile.am index ac51fc6..57e859d 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -28,7 +28,7 @@ pkginclude_HEADERS = \ str.h stream.h table.h thread.h vector.h version.h vty.h zebra.h \ plist.h zclient.h sockopt.h smux.h md5.h if_rmap.h keychain.h \ privs.h sigevent.h pqueue.h jhash.h zassert.h memtypes.h \ - workqueue.h route_types.h libospf.h vrf.h + workqueue.h route_types.h libospf.h vrf.h fifo.h noinst_HEADERS = \ plist_int.h _______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
