[quagga-dev 11599] [PATCH] bgpd: Fixes for recent well-known-attr check patch.

Paul Jakma paul at opensourcerouting.org
Tue Oct 21 17:31:01 BST 2014


* bgp_attr.c: Recent patch to tighten well-known attr checks and apply that
  to all AFIs has some breakage with MP-extensions and GR, which needs to be
  fixed.
  (bgp_attr_check) Graceful Restart EoR can be an empty UPDATE for IPv4/uni.
  MP-Ext allow UPDATE with just MP_UNREACH_NLRI. Check for these and return
  proceed.
  NEXT_HOP becomes optional, if MP_REACH_NLRI is present and there's no
  v4 NLTI, update NEXT_HOP check accordingly.
  Print the missing attr in string form in the log message.
  (bgp_attr_parse) AS_PATH need not be there, so
  bgp_attr_munge_as4_attrs call needs to be conditional on that.
---
 bgpd/bgp_attr.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 69ca786..da17e82 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1739,15 +1739,32 @@ bgp_attr_check (struct peer *peer, struct attr *attr)
 {
   u_char type = 0;
   
+  /* BGP Graceful-Restart End-of-RIB for IPv4 unicast is signaled as an
+   * empty UPDATE.  */
+  if (CHECK_FLAG (peer->cap, PEER_CAP_RESTART_RCV) && !attr->flag)
+    return BGP_ATTR_PARSE_PROCEED;
+  
+  /* "An UPDATE message that contains the MP_UNREACH_NLRI is not required
+     to carry any other path attributes.", though if MP_REACH_NLRI or NLRI
+     are present, it should.  Check for any other attribute being present
+     instead.
+   */
+  if (attr->flag == ATTR_FLAG_BIT (BGP_ATTR_MP_UNREACH_NLRI))
+    return BGP_ATTR_PARSE_PROCEED;
+  
   if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_ORIGIN)))
     type = BGP_ATTR_ORIGIN;
 
   if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_AS_PATH)))
     type = BGP_ATTR_AS_PATH;
-
-  if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP)))
+  
+  /* RFC 2858 makes Next-Hop optional/ignored, if MP_REACH_NLRI is present and
+   * NLRI is empty. We can't easily check NLRI empty here though.
+   */
+  if (CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_MP_REACH_NLRI))
+      && !CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP)))
     type = BGP_ATTR_NEXT_HOP;
-
+  
   if (peer->sort == BGP_PEER_IBGP
       && ! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF)))
     type = BGP_ATTR_LOCAL_PREF;
@@ -1755,8 +1772,8 @@ bgp_attr_check (struct peer *peer, struct attr *attr)
   if (type)
     {
       zlog (peer->log, LOG_WARNING, 
-	    "%s Missing well-known attribute %d.",
-	    peer->host, type);
+	    "%s Missing well-known attribute %d / %s",
+	    peer->host, type, LOOKUP (attr_str, type));
       bgp_notify_send_with_data (peer, 
 				 BGP_NOTIFY_UPDATE_ERR, 
 				 BGP_NOTIFY_UPDATE_MISS_ATTR,
@@ -2033,10 +2050,14 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
    * So, to be defensive, we are not relying on any order and read
    * all attributes first, including these 32bit ones, and now,
    * afterwards, we look what and if something is to be done for as4.
+   *
+   * It is possible to not have AS_PATH, e.g. GR EoR and sole
+   * MP_UNREACH_NLRI.
    */
   /* actually... this doesn't ever return failure currently, but
    * better safe than sorry */
-  if (bgp_attr_munge_as4_attrs (peer, attr, as4_path,
+  if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT (BGP_ATTR_AS_PATH))
+      && bgp_attr_munge_as4_attrs (peer, attr, as4_path,
                                 as4_aggregator, &as4_aggregator_addr))
     {
       bgp_notify_send (peer, 
-- 
1.7.11.7





More information about the Quagga-dev mailing list