On Tue, 23 Jun 2015 13:02:27 -0700
Vivek Venkatraman <[email protected]> wrote:
> The change for not considering paths from a peer that has been
> deleted and hence not in Established state is as follows:
Right. I'm not sure if it'd fix it or not, but quite possible it would.
My original su_remote == NULL check is not correct. It seems that some
bgp_info is compared with su_remote=NULL and it's supposed to be
perfectly legal. E.g. configured subnet announces ("network a.b.c.d/n").
I ended up committing:
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 34ba1ab..7ade22f 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -553,6 +553,11 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
struct bgp_info *exist,
return 0;
/* 13. Neighbor address comparision. */
+ if (new->peer->su_remote == NULL)
+ return 0;
+ if (exist->peer->su_remote == NULL)
+ return 1;
+
ret = sockunion_cmp (new->peer->su_remote, exist->peer->su_remote);
if (ret == 1)
> 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;
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
