Hi Timo, The change for not considering paths from a peer that has been deleted and hence not in Established state is as follows:
Author: Donald Sharp <[email protected]> Date: Tue May 19 18:03:51 2015 -0700 Ensure that routes from a peer are not considered for best path comparison if the peer is not in an Established state. There can be a window between a peer being deleted and the background thread that actually clears the routes (marks them as "removed") runs during which best path may run. If this path selection compared two prefixes all the way down to peer IP addresses and one of these two peers had just been deleted, that peer would not have its sockunion structures, especially su_remote, resulting in a BGPD exception. Signed-off-by: Vivek Venkatraman <[email protected]> diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 7e0c604..fe39d47 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -1991,6 +1991,9 @@ bgp_best_selection (struct bgp *bgp, struct bgp_node *rn, continue; if (BGP_INFO_HOLDDOWN (ri1)) continue; + if (ri1->peer && ri1->peer != bgp->peer_self) + if (ri1->peer->status != Established) + continue; new_select = ri1; if (do_mpath) @@ -2003,6 +2006,11 @@ bgp_best_selection (struct bgp *bgp, struct bgp_node *rn, continue; if (BGP_INFO_HOLDDOWN (ri2)) continue; + if (ri2->peer && + ri2->peer != bgp->peer_self && + !CHECK_FLAG (ri2->peer->sflags, PEER_STATUS_NSF_WAIT)) + if (ri2->peer->status != Established) + continue; if (aspath_cmp_left (ri1->attr->aspath, ri2->attr->aspath) || aspath_cmp_left_confed (ri1->attr->aspath, @@ -2055,6 +2063,12 @@ bgp_best_selection (struct bgp *bgp, struct bgp_node *rn, continue; } + if (ri->peer && + ri->peer != bgp->peer_self && + !CHECK_FLAG (ri->peer->sflags, PEER_STATUS_NSF_WAIT)) + if (ri->peer->status != Established) + continue; + if (bgp_flag_check (bgp, BGP_FLAG_DETERMINISTIC_MED) && (! CHECK_FLAG (ri->flags, BGP_INFO_DMED_SELECTED))) { Your check for su_remote being present would also prevent the crash. I'll also post the change we've made for fixing the issue related to multiple Clearing_Completion events (item (c) in my earlier mail) shortly. Vivek On Tue, Jun 23, 2015 at 12:55 AM, Timo Teras <[email protected]> wrote: > Hi, > > Unfortunately just having bgpd-gr-route-selection-fix.patch does not > fully fix the issue. It seems that su_remote can be null when > BGP_INFO_STALE is not set. > > The peer would probably be in Connected state, so depending on how > bgpd-fix-route-selection-for-deleted-peer.patch is done, that would > probably fix it. I was not able to find this patch anywhere. Would it > be possible to post it? > > Otherwise, I'll probably add explicit su_remote null checks; something > like: > > diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c > index 34ba1ab..ad8642e 100644 > --- a/bgpd/bgp_route.c > +++ b/bgpd/bgp_route.c > @@ -348,9 +348,9 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new, > struct bgp_info *exist, > *paths_eq = 0; > > /* 0. Null check. */ > - if (new == NULL) > + if (new == NULL || new->peer->su_remote == NULL) > return 0; > - if (exist == NULL) > + if (exist == NULL || exists->peer->su_remote == NULL) > return 1; > > newattr = new->attr; > > Thanks, > Timo > > > On Wed, 17 Dec 2014 13:24:55 +0200 > Timo Teras <[email protected]> wrote: > > > Hi Vivek, > > > > Thanks for the info. I believe the stale flag was set, so sounds like > > the patch you posted should fix it. I'll apply that and see how it > > goes. > > > > I'd happy to look at the other two patches if possible. > > > > Thanks, > > Timo > > > > On Tue, 16 Dec 2014 11:58:35 -0800 (PST) > > Vivek Venkatraman <[email protected]> wrote: > > > > > Hi Timo, > > > > > > We (Cumulus) have seen a very similar crash (with same stack trace) > > > in a couple of different situations. For one scenario, we have > > > already submitted a patch which is up on the Cumulus quagga github > > > tree and will hopefully be rolled in soon. For the second scenario, > > > we have a patch that is currently in test and should be pushed by us > > > soon. In addition, there is a third scenario that we uncovered > > > recently that may potentially lead to this situation too, though we > > > haven't actually seen this manifestation. A fix for this is also in > > > test and should be part of the next set of patches that we push. > > > > > > None of these 3 patches has anything to do with any other Cumulus > > > patches. > > > > > > a) bgpd-gr-route-selection-fix.patch > > > > > > This is the patch that is up on our tree and can be viewed at > > > > https://github.com/CumulusNetworks/quagga/commit/94480b6b74ce32cf57e317630f2bafbfbd853eba > . > > > > > > This patch is to address the scenario when one of the paths in the > > > comparison has become "stale" as the peer which advertised this has > > > gracefully restarted. Stale paths should be considered in best path > > > selection in the same manner as non-stale paths, but cannot have the > > > neighbor address compared as the peer may not have reconnected and > > > may not have its su_remote information. > > > > > > You should be able to identify if this was your scenario based on > > > your configuration and/or looking at the "flags" of the route with > > > the NULL su_remote, if you are able to. > > > > > > > > > b) bgpd-fix-route-selection-for-deleted-peer.patch > > > > > > Ensure that routes from a peer are not considered for best path > > > comparison if the peer is not in an Established state. There can be > > > a window between a peer being deleted and the background thread that > > > actually clears the routes (marks them as "removed") runs during > > > which best path may run. If this path selection compared two > > > prefixes all the way down to peer IP addresses and one of these two > > > peers had just been deleted, that peer would not have its sockunion > > > structures, especially su_remote, resulting in a BGPD exception. > > > > > > Again, this would only matter to you if your scenario had peer > > > deletion taking place ocassionally. > > > > > > > > > c) bgpd-fix-clearing-completion.patch > > > > > > This is potentially a more serious issue, though a few things have > > > to align for it to be hit. The Clearing_Completed event is meant to > > > be for the entire peering and expected to be received when the peer > > > is in Clearing state and upon receipt, transition to Idle state. > > > However, the way bgp_clear_route() is written, this event could get > > > generated multiple times, per AFI/SAFI. An example scenario is if > > > the peer has only negotiated IPv6 unicast, it will/could end up > > > receiving Clearing_Completed events for each SAFI of IPv4 > > > address-family as nothing got added to the clear-node queue. The > > > first such event will transition the peer prematurely to Idle > > > state, while Clearing is still in progress (for the IPv6 > > > address-family in the stated example). Subsequent events could > > > cause other bad things - e.g., a new connection is Established and > > > then the "real" Clearing_Completed event is received after > > > completion of clearing for the IPv6 address-family. This would > > > "silently" (i.e., not through bgp_stop()) transition the peer to > > > Idle and subsequent actions could clear "su_remote", but there may > > > be prefixes from the peer hanging around from the time the > > > connection got setup the 2nd time. This could then cause a crash > > > with stack similar to what you mention below. > > > > > > Thanks, > > > Vivek > > > > > > > > > ----- Original Message ----- > > > From: "Timo Teras" <[email protected]> > > > To: "quagga-dev" <[email protected]> > > > Sent: Monday, December 15, 2014 11:29:59 PM > > > Subject: [quagga-dev 11909] rare bgp crash > > > > > > Hi, > > > > > > I've experience a rare (once in a few weeks to a month) crash with > > > bgpd. Quagga version 0.99.23.1. > > > > > > Back trace is as follows: > > > > > > (gdb) where > > > #0 0x0000733b9354a130 in sockunion_cmp () > > > from /usr/lib/libzebra.so.0 #1 0x00000f803aabaf1f in bgp_info_cmp > > > () #2 0x00000f803aabb5ed in bgp_best_selection () > > > #3 0x00000f803aabb955 in bgp_process_main () > > > #4 0x0000733b93568eb1 in work_queue_run () > > > from /usr/lib/libzebra.so.0 #5 0x0000733b9354d4c7 in thread_call () > > > from /usr/lib/libzebra.so.0 #6 0x00000f803aaa2b5c in main (argc=6, > > > argv=<optimized out>) at bgp_main.c:463 > > > > > > Unfortunately it seems the build did not honor CFLAGS="-g" fully, so > > > the debug info is not complete. > > > > > > Based on disassembly, and register dumps, sockunion_cmp() is called > > > with su1=0, su2=0xf803ca52500. > > > > > > bgp_info_cmp() calls sockunion_cmp() only in once place. This would > > > imply that the previously selected route's peer entry's su_remote is > > > null. > > > > > > And the call to bgp_info_cmp() comes from the later code path ("no > > > bgp deterministic-med"). > > > > > > But clearly the crash is because selected prefix's peer has > > > su_remote = 0. I believe there are only two code paths when this > > > can happen: > > > - bgp_start() is called and clears su_remote due to reconnect > > > request (other code paths seem to come from Idle state transitions) > > > - there's bgp unbalanced bgp peer unreference, and the peer was > > > deleted prematurely > > > > > > The first option sounds more plausible. As there are certain > > > scenarios when the box scripts clear certain bgp peers using vtysh. > > > And most of the other places of bgpd code do seem to check for > > > su_remote != 0 before doing anything with it. > > > > > > Any suggestions how to fix this? Just add check for su_remote != 0? > > > > > > Thanks, > > > Timo > > > > > > _______________________________________________ > > > Quagga-dev mailing list > > > [email protected] > > > https://lists.quagga.net/mailman/listinfo/quagga-dev > > > >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
