[quagga-dev 11706] Re: [PATCH 1/6] Buggy Unlock in ospfd/ospf_opaque.c

David Lamparter equinox at opensourcerouting.org
Thu Oct 30 17:39:09 GMT 2014


On Thu, Oct 30, 2014 at 04:44:23PM +0000, Paul Jakma wrote:
> Hi Olivier,
> 
> I think your mailer, or the server it spoke to perhaps, has mangled the 
> formating of your patches. E.g. there is line-wrap (probably your MUA), 
> and there is also text at the end (presumably added by the server).
> 
> Could you export the 6 commits to files and send them to me?

Hi Olivier,


On a related note, it'd be really cool if your git repository
https://github.com/Orange-OpenSource/Quagga-TE/tree/master/TE-patches
would actually contain these patches - not as files as it is right now,
but actually as part of the history.  I.e. the way it will look when it
is merged into Quagga.  That way we could indeed merge your tree.

(Easiest way to get there: create new quagga repo, apply these patches
you posted, publish result to github.)

Cheers,


-David

> On Tue, 7 Oct 2014, olivier.dugeon at orange.com wrote:
> 
> > An old bug in ospfd/ospf_opaque.c blocks opaque LSA flooding when the Quagga 
> > router is restarted or when an interface goes down then up in less than 3600 
> > seconds (i.e. LSA MAX AGE). In such situation, the neighbour router have 
> > acquired TE LSA from the Quagga router and keep them in its Link State Data 
> > Base (LSDB). When the Quagga router restart or interface goes up, the 
> > neighbour router send back the original TE LSA. Quagga detects these self 
> > generated opaque LSA and signal to its neighbour router with LS ACK + LSA age 
> > = MAX AGE that the LSA must not be kept during the LSDB synchronisation 
> > phase.
> >
> > The problem comes from opaque_lsa.c code that blocks subsequent opaque LSA 
> > flooding until the neighbour router acknowledge that it removes the old 
> > opaque LSA from its LSDB. The bug comes from the fact that the lock is never 
> > release, thus avoiding subsequent opaque LSA flooding. The condition to 
> > unlock flooding is never raise: the original code count the number of self 
> > originate opaque LSA in LSDB and release the lock only if count equal 0. 
> > However, as the configuration file ask to flood Opaque LSA, there is already 
> > at least one opaque LSA in LSDB and count could never be equal to 0.The only 
> > solution is to restart again the ospfd daemon.
> >
> > The correction skip counting the self originate opaque LSA in LSDB and unlock 
> > opaque LSA flooding once neighbour router acknowledge the removal of old 
> > opaque LSA in its LSDB.
> >
> > As there is no suck mechanism for standard LSA i.e. type 1 to 7, I would 
> > propose to remove completely this mechanism. For the moment, I just comment 
> > it. Once acknowledge, I will submit a new patch.
> >
> > I test the patch with Cisco and Juniper and don't observe any problem. I also 
> > made some Wireshark capture and seen nothing strange. I also compare Quagga 
> > behaviour with Cisco and Juniper behaviour in the same condition, and 
> > Wireshark capture are similar.
> >
> > Signed-off-by: Olivier Dugeon <olivier.dugeon at orange.com> 
> > <mailto:olivier.dugeon at orange.com>
> > --
> > ospf_opaque.c |   20 +++++++++++++-------
> > 1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/ospfd/ospf_opaque.c b/ospfd/ospf_opaque.c
> > index 744952c..d9c96db 100644
> > --- a/ospfd/ospf_opaque.c
> > +++ b/ospfd/ospf_opaque.c
> > @@ -2126,10 +2156,12 @@ out:
> > *------------------------------------------------------------------------*/
> >
> > static void ospf_opaque_exclude_lsa_from_lsreq (struct route_table *nbrs, 
> > struct ospf_neighbor *inbr, struct ospf_lsa *lsa);
> > +#ifdef BUGGY_UNLOCK
> > static void ospf_opaque_type9_lsa_rxmt_nbr_check (struct ospf_interface 
> > *oi);
> > static void ospf_opaque_type10_lsa_rxmt_nbr_check (struct ospf_area *area);
> > static void ospf_opaque_type11_lsa_rxmt_nbr_check (struct ospf *top);
> > static unsigned long ospf_opaque_nrxmt_self (struct route_table *nbrs, int 
> > lsa_type);
> > +#endif /* BUGGY_UNLOCK */
> >
> > void
> > ospf_opaque_adjust_lsreq (struct ospf_neighbor *nbr, struct list *lsas)
> > @@ -2296,17 +2328,20 @@ ospf_opaque_ls_ack_received (struct ospf_neighbor 
> > *nbr, struct ospf_lsa *lsa)
> >     {
> >     case OSPF_OPAQUE_LINK_LSA:
> >       if (CHECK_FLAG (top->opaque, OPAQUE_BLOCK_TYPE_09_LSA_BIT))
> > -        ospf_opaque_type9_lsa_rxmt_nbr_check (nbr->oi);
> > +        UNSET_FLAG (top->opaque, OPAQUE_BLOCK_TYPE_09_LSA_BIT);
> > +        /* BUGGY_UNLOCK: ospf_opaque_type9_lsa_rxmt_nbr_check (nbr->oi); */
> >       /* Callback function... */
> >       break;
> >     case OSPF_OPAQUE_AREA_LSA:
> >       if (CHECK_FLAG (top->opaque, OPAQUE_BLOCK_TYPE_10_LSA_BIT))
> > -        ospf_opaque_type10_lsa_rxmt_nbr_check (nbr->oi->area);
> > +        UNSET_FLAG (top->opaque, OPAQUE_BLOCK_TYPE_10_LSA_BIT);
> > +        /* BUGGY_UNLOCK: ospf_opaque_type10_lsa_rxmt_nbr_check 
> > (nbr->oi->area); */
> >       /* Callback function... */
> >       break;
> >     case OSPF_OPAQUE_AS_LSA:
> >       if (CHECK_FLAG (top->opaque, OPAQUE_BLOCK_TYPE_11_LSA_BIT))
> > -        ospf_opaque_type11_lsa_rxmt_nbr_check (top);
> > +        UNSET_FLAG (top->opaque, OPAQUE_BLOCK_TYPE_11_LSA_BIT);
> > +        /* BUGGY_UNLOCK: ospf_opaque_type11_lsa_rxmt_nbr_check (top); */
> >       /* Callback function... */
> >       break;
> >     default:
> > @@ -2315,11 +2350,10 @@ ospf_opaque_ls_ack_received (struct ospf_neighbor 
> > *nbr, struct ospf_lsa *lsa)
> >     }
> >
> >   if (IS_OPAQUE_LSA_ORIGINATION_BLOCKED (top->opaque))
> > -    {
> > -      if (IS_DEBUG_OSPF_EVENT)
> > -        zlog_debug ("Block Opaque-LSA origination: ON -> OFF");
> >       return; /* Blocking still in progress. */
> > -    }
> > +
> > +  if (IS_DEBUG_OSPF_EVENT)
> > +    zlog_debug ("Block Opaque-LSA origination: ON -> OFF");
> >
> >   if (! CHECK_FLAG (top->config, OSPF_OPAQUE_CAPABLE))
> >     return; /* Opaque capability condition must have changed. */
> > @@ -2339,6 +2373,7 @@ ospf_opaque_ls_ack_received (struct ospf_neighbor *nbr, 
> > struct ospf_lsa *lsa)
> >   return;
> > }
> >
> > +#ifdef BUGGY_UNLOCK
> > static void
> > ospf_opaque_type9_lsa_rxmt_nbr_check (struct ospf_interface *oi)
> > {
> > @@ -2439,6 +2474,7 @@ ospf_opaque_nrxmt_self (struct route_table *nbrs, int 
> > lsa_type)
> >
> >   return n;
> > }
> > +#endif /* BUGGY_UNLOCK */
> >
> > /*------------------------------------------------------------------------*
> >  * Followings are util functions; probably be used by Opaque-LSAs only...
> >
> >
> >
> > _________________________________________________________________________________________________________________________
> >
> > Ce message et ses pieces jointes peuvent contenir des informations 
> > confidentielles ou privilegiees et ne doivent donc
> > pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu 
> > ce message par erreur, veuillez le signaler
> > a l'expediteur et le detruire ainsi que les pieces jointes. Les messages 
> > electroniques etant susceptibles d'alteration,
> > Orange decline toute responsabilite si ce message a ete altere, deforme ou 
> > falsifie. Merci.
> >
> > This message and its attachments may contain confidential or privileged 
> > information that may be protected by law;
> > they should not be distributed, used or copied without authorisation.
> > If you have received this email in error, please notify the sender and delete 
> > this message and its attachments.
> > As emails may be altered, Orange is not liable for messages that have been 
> > modified, changed or falsified.
> > Thank you.
> >
> >
> 
> -- 
> Paul Jakma	paul at jakma.org	@pjakma	Key ID: 64A2FF6A
> Fortune:
> Something unpleasant is coming when men are anxious to tell the truth.
>  		-- Benjamin Disraeli
> 
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev at lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev




More information about the Quagga-dev mailing list