[quagga-dev 10968] Re: Assert in bgp_attr.c

Milan Kocian milon at wq.cz
Mon Dec 16 15:30:33 GMT 2013


hello david,

I have more info about this problem.

On Wed, Dec 04, 2013 at 09:59:31AM +0100, Milan Kocian wrote:
> On Tue, Dec 03, 2013 at 06:38:19PM +0100, David Lamparter wrote:
> > On Fri, Nov 29, 2013 at 04:53:15PM +0100, Milan Kocian wrote:
> > > I get this assert when I try to add extended community on received routes which
> > > have ext attributes set. When received route has no ext community
> > > there is no problem with adding new ext. community.
> > 
> > Urgh.  I think I know what the problem is... and it's quite ugly
> > actually:
> > 
> > > 2013/11/29 16:27:57 BGP: Assertion `ret != ((void *)0)' failed in file
> > > bgp_attr.c, line 693, function bgp_attr_unintern
> > 
> > This is indicating that the attr could not be found in the caching-index
> > attrhash.  If I'm not completely mistaken, the reason for this is that
> > bgp_routemap.c:1559 just modifies the existing attr, without copying it
> > or rehashing.  This may sound innocent enough, but it actually means
> > that ALL routes that share this cached attr instance get the change
> > applied!  Meaning, we're causing other routes to have broken attributes.
> > Whoops.
> > 
> > I'll mail a patch for this in a bit,
> > 
> > 
> > David
> > 
> 
> There is no problem when:
> 
> 1. received routes have no extcomms set (then I can set extcomms in routemaps 
>    on crash side without problem)
> 2. when received routes have extcomms and I don't set extcomms on crash side
> 
> So problematic case is when received routes have extcomms and I am adding
> my extcomms in routemap.
> 
> IMHO problem is in this path in bgp_routemap.c (but I can't see
> any problem there :-( ). But may be I am completely wrong :-).
> 
> static route_map_result_t
> route_set_ecommunity_soo (void *rule, struct prefix *prefix,
>                          route_map_object_t type, void *object)
> {
>   struct ecommunity *ecom, *old_ecom, *new_ecom;
>   struct bgp_info *bgp_info;
> 
>   if (type == RMAP_BGP)
>     {
>       ecom = rule;
>       bgp_info = object;
> 
>       if (! ecom)
>         return RMAP_OKAY;
> 
>       old_ecom = (bgp_attr_extra_get (bgp_info->attr))->ecommunity;
> 
> >>>      if (old_ecom)
> >>>        new_ecom = ecommunity_merge (ecommunity_dup (old_ecom), ecom);
>       else
>         new_ecom = ecommunity_dup (ecom);
> 
>       bgp_info->attr->extra->ecommunity = ecommunity_intern (new_ecom);
> 
> >>>     if (old_ecom)
> >>>        ecommunity_unintern (&old_ecom);
> 

This is problematic piece of code. Here is freed old extcommunity first time.
Second time the same extcom is freed in bgp_attr_unintern_sub (called from
bgp_update_receive). I am not sure how to synchronize pointers in this two places.
In bgp_update_receive is 'attr' local variable so doesn't know about games with
extcom pointer in routemap.

>       bgp_info->attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_EXT_COMMUNITIES);
>     }
>   return RMAP_OKAY;
> }
> 
> 

This patch fixes all my crashes (probably not correct) :

--- a/bgpd/bgp_routemap.c
+++ b/bgpd/bgp_routemap.c
@@ -1558,9 +1558,6 @@ route_set_ecommunity_rt (void *rule, struct prefix *prefix,
 
       bgp_info->attr->extra->ecommunity = ecommunity_intern (new_ecom);
 
-      if (old_ecom)
-       ecommunity_unintern (&old_ecom);
-
       bgp_info->attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_EXT_COMMUNITIES);
     }
   return RMAP_OKAY;
@@ -1622,9 +1619,6 @@ route_set_ecommunity_soo (void *rule, struct prefix *prefix,
 
       bgp_info->attr->extra->ecommunity = ecommunity_intern (new_ecom);
 
-      if (old_ecom)
-       ecommunity_unintern (&old_ecom);
-
       bgp_info->attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_EXT_COMMUNITIES);
     }
   return RMAP_OKAY;


Best regards,

-- 
Milan Kocian




More information about the Quagga-dev mailing list