[quagga-dev 10173] Re: [PATCH] bgpd: fix leak from soft reconfig

David Lamparter equinox at opensourcerouting.org
Mon Jan 14 12:38:00 GMT 2013


On Thu, Jan 05, 2012 at 09:32:04AM -0800, Stephen Hemminger wrote:
> 
> When soft-reconfig is used the advertised routes are stored and
> never cleared when session is lost. This means peer still has reference
> and all other memory is held as well.

> --- a/bgpd/bgp_fsm.c	2011-10-10 08:32:32.384321044 -0700
> +++ b/bgpd/bgp_fsm.c	2012-01-05 09:30:16.955532576 -0800
> @@ -546,6 +546,10 @@ bgp_stop (struct peer *peer)
>          /* ORF received prefix-filter pnt */
>          sprintf (orf_name, "%s.%d.%d", peer->host, afi, safi);
>          prefix_bgp_orf_remove_all (orf_name);
> +
> +	/* Drop soft reconfig info */
> +	if (CHECK_FLAG (peer->af_flags[afi][safi], PEER_FLAG_SOFT_RECONFIG))
> +	  bgp_clear_adj_in (peer, afi, safi);
>        }
>  
>    /* Reset keepalive and holdtime */

Also threw this to Leonid for review, his feedback is this:

(begin paste)
Recommend this patch be rejected at this time on these grounds:

1. It appears there already is a call to bgp_clear_adj_in() in the case
of SOFT_RECONFIG from peer_af_flag_modify() in bgpd/bgpd.c:

  /* Execute action when peer is established.  */
  if (! CHECK_FLAG (peer->sflags, PEER_STATUS_GROUP)
      && peer->status == Established)
    {
      if (! set && flag == PEER_FLAG_SOFT_RECONFIG)
        bgp_clear_adj_in (peer, afi, safi);
      else

and

          if (set)
            SET_FLAG (peer->af_flags[afi][safi], flag);
          else
            UNSET_FLAG (peer->af_flags[afi][safi], flag);

          if (peer->status == Established)
            {
              if (! set && flag == PEER_FLAG_SOFT_RECONFIG)
                bgp_clear_adj_in (peer, afi, safi);
              else

2. Deallocation of routing entries from within FSM transition function is
inconsistent with the overall design of the BGP code.

(end paste)

I'm not totally sure what to make of this; peer_af_flag_modify is only
called from vty functions currently, the inconsistency here might be the
other way around in that the vty functions are meddling with stuff that
they shouldn't [but nonetheless do the right thing] and the FSM events
do it wrong.

However, bgp_stop() still doesn't seem like the best place to do this.
What about bgp_clear_route_all()?

--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -2963,5 +2963,9 @@ bgp_clear_route_all (struct peer *peer)
 
   for (afi = AFI_IP; afi < AFI_MAX; afi++)
     for (safi = SAFI_UNICAST; safi < SAFI_MAX; safi++)
-      bgp_clear_route (peer, afi, safi, BGP_CLEAR_ROUTE_NORMAL);
+      {
+        bgp_clear_route (peer, afi, safi, BGP_CLEAR_ROUTE_NORMAL);
+        if (CHECK_FLAG (peer->af_flags[afi][safi], PEER_FLAG_SOFT_RECONFIG))
+          bgp_clear_adj_in (peer, afi, safi);
+      }
 }

Seems more sensible to me at least...

Stephen, can you comment?


-David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 230 bytes
Desc: Digital signature
URL: <http://lists.quagga.net/pipermail/quagga-dev/attachments/20130114/e4281204/attachment-0001.sig>


More information about the Quagga-dev mailing list