OSPF: Fix handling of unnumbered PtPs

This issue has a long history. In 2012, we changed data field for
unnumbered PtP links from iface id (specified by RFC) to IP address based
on reports of bugs in Quagga that required it, and we used out-of-band
information to distinquish unnumberred PtPs with the same local IP
address.

Then with OSPF graceful restart implementation, we found that we can no
longer use out-of-band information, and we need to use only LSAdb info
for routing table calculation, but i forgot to finish handling of this
case, so multiple unnumbered PtPs with the same local IP addresses were
broken.

Considering that even recent Mikrotik RouterOS has broken next hop
calculation that depends on IP address in PtP link data field, we
cannot just switch back to the iface id for unnumbered PtP links.

The patch makes two changes: First, it goes back to use out-of-band
(position) info for distinguishing local interfaces in SPF when graceful
restart is not enabled, while still uses LSAdb-only approach for SPF
calculation when graceful restart is enabled.

Second, it adds OSPF interface option 'ptp address', which controls
whether IP address or iface id is used in data field. It is enabled
by default except for unnumbered PtP links with enabled graceful
restart.

Thanks to Kenth Eriksson for the bugreport and Joakim Tjernlund for
suggestions.

diff --git a/doc/bird.sgml b/doc/bird.sgml
index 401b4035e27d7a95df16b801204998cbd21533d8..552f650927f31abcb081d091d19af14f57a580cf 100644 (file)
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -3760,11 +3760,28 @@ protocol ospf [v2|v3] <name> {
        In <cf/type ptp/ network configurations, OSPFv2 implementations should
        ignore received netmask field in hello packets and should send hello
        packets with zero netmask field on unnumbered PtP links. But some OSPFv2
-       implementations perform netmask checking even for PtP links. This option
-       specifies whether real netmask will be used in hello packets on <cf/type
-       ptp/ interfaces. You should ignore this option unless you meet some
-       compatibility problems related to this issue. Default value is no for
-       unnumbered PtP links, yes otherwise.
+       implementations perform netmask checking even for PtP links.
+
+       This option specifies whether real netmask will be used in hello packets
+       on <cf/type ptp/ interfaces. You should ignore this option unless you
+       meet some compatibility problems related to this issue. Default value is
+       no for unnumbered PtP links, yes otherwise.
+
+       <tag><label id="ospf-ptp-address">ptp address <m/switch/</tag>
+       In <cf/type ptp/ network configurations, OSPFv2 implementations should
+       use IP address for regular PtP links and interface id for unnumbered PtP
+       links in data field of link description records in router LSA. This data
+       field has only local meaning for PtP links, but some broken OSPFv2
+       implementations assume there is an IP address and use it as a next hop
+       in SPF calculations. Note that interface id for unnumbered PtP links is
+       necessary when graceful restart is enabled to distinguish PtP links with
+       the same local IP address.
+
+       This option specifies whether an IP address will be used in data field
+       for <cf/type ptp/ interfaces, it is ignored for other interfaces. You
+       should ignore this option unless you meet some compatibility problems
+       related to this issue. Default value is no for unnumbered PtP links when
+       graceful restart is enabled, yes otherwise.
 
        <tag><label id="ospf-check-link">check link <M>switch</M></tag>
        If set, a hardware link state (reported by OS) is taken into consideration.

diff --git a/proto/ospf/config.Y b/proto/ospf/config.Y
index f631a64934562c6722684efa85d3c7668322bafd..ce9245a1fc09a963d7b7e5a6cc04c7695ce375bd 100644 (file)
--- a/proto/ospf/config.Y
+++ b/proto/ospf/config.Y
@@ -199,7 +199,7 @@ CF_KEYWORDS(ELIGIBLE, POLL, NETWORKS, HIDDEN, VIRTUAL, CHECK, LINK, ONLY, BFD)
 CF_KEYWORDS(RX, BUFFER, LARGE, NORMAL, STUBNET, HIDDEN, SUMMARY, TAG, EXTERNAL)
 CF_KEYWORDS(WAIT, DELAY, LSADB, ECMP, LIMIT, WEIGHT, NSSA, TRANSLATOR, STABILITY)
 CF_KEYWORDS(GLOBAL, LSID, ROUTER, SELF, INSTANCE, REAL, NETMASK, TX, PRIORITY, LENGTH)
-CF_KEYWORDS(MERGE, LSA, SUPPRESSION, MULTICAST, RFC5838, VPN, PE)
+CF_KEYWORDS(MERGE, LSA, SUPPRESSION, MULTICAST, RFC5838, VPN, PE, ADDRESS)
 CF_KEYWORDS(GRACEFUL, RESTART, AWARE, TIME)
 
 %type <ld> lsadb_args
@@ -396,6 +396,7 @@ ospf_iface_item:
  | TYPE PTMP { OSPF_PATT->type = OSPF_IT_PTMP ; }
  | REAL BROADCAST bool { OSPF_PATT->real_bcast = $3; if (!ospf_cfg_is_v2()) cf_error("Real broadcast option requires OSPFv2"); }
  | PTP NETMASK bool { OSPF_PATT->ptp_netmask = $3; if (!ospf_cfg_is_v2()) cf_error("PtP netmask option requires OSPFv2"); }
+ | PTP ADDRESS bool { OSPF_PATT->ptp_address = $3; if (!ospf_cfg_is_v2()) cf_error("PtP address option requires OSPFv2"); }
  | TRANSMIT DELAY expr { OSPF_PATT->inftransdelay = $3 ; if (($3<=0) || ($3>65535)) cf_error("Transmit delay must be in range 1-65535"); }
  | PRIORITY expr { OSPF_PATT->priority = $2 ; if ($2>255) cf_error("Priority must be in range 0-255"); }
  | STRICT NONBROADCAST bool { OSPF_PATT->strictnbma = $3 ; }
@@ -475,6 +476,7 @@ ospf_iface_start:
   init_list(&OSPF_PATT->nbma_list);
   OSPF_PATT->check_link = 1;
   OSPF_PATT->ptp_netmask = 2; /* not specified */
+  OSPF_PATT->ptp_address = 2; /* not specified */
   OSPF_PATT->tx_tos = IP_PREC_INTERNET_CONTROL;
   OSPF_PATT->tx_priority = sk_priority_control;
   reset_passwords();

diff --git a/proto/ospf/iface.c b/proto/ospf/iface.c
index d58747c6f3b09bae90fd078bf8aa5fa541b41a16..666140b5ab8b6bd972f2405f188adf3962dee001 100644 (file)
--- a/proto/ospf/iface.c
+++ b/proto/ospf/iface.c
@@ -602,6 +602,11 @@ ospf_iface_new(struct ospf_area *oa, struct ifa *addr, struct ospf_iface_patt *i
   if (ip->ptp_netmask < 2)
     ifa->ptp_netmask = ip->ptp_netmask;
 
+  /* For compatibility, we may use ptp_address even for unnumbered links */
+  ifa->ptp_address = !(addr->flags & IA_PEER) || (p->gr_mode != OSPF_GR_ABLE);
+  if (ip->ptp_address < 2)
+    ifa->ptp_address = ip->ptp_address;
+
   ifa->drip = ifa->bdrip = ospf_is_v2(p) ? IPA_NONE4 : IPA_NONE6;
 
   ifa->type = ospf_iface_classify(ip->type, addr);
@@ -1004,6 +1009,29 @@ ospf_iface_reconfigure(struct ospf_iface *ifa, struct ospf_iface_patt *new)
     ospf_notify_link_lsa(ifa);
   }
 
+  /* PtP netmask */
+  int new_ptp_netmask = (new->ptp_netmask < 2) ? new->ptp_netmask :
+    !(ifa->addr->flags & IA_PEER);
+  if (ifa->ptp_netmask != new_ptp_netmask)
+  {
+    OSPF_TRACE(D_EVENTS, "Changing PtP netmask option of %s from %d to %d",
+              ifname, ifa->ptp_netmask, new_ptp_netmask);
+    ifa->ptp_netmask = new_ptp_netmask;
+  }
+
+  /* PtP address */
+  int new_ptp_address = (new->ptp_address < 2) ? new->ptp_address :
+    (!(ifa->addr->flags & IA_PEER) || (p->gr_mode != OSPF_GR_ABLE));
+  if (ifa->ptp_address != new_ptp_address)
+  {
+    /* Keep it silent for implicit changes */
+    if (new->ptp_address < 2)
+      OSPF_TRACE(D_EVENTS, "Changing PtP address option of %s from %d to %d",
+                ifname, ifa->ptp_address, new_ptp_address);
+
+    ifa->ptp_address = new_ptp_address;
+  }
+
   /* BFD */
   if (ifa->bfd != new->bfd)
   {

diff --git a/proto/ospf/ospf.h b/proto/ospf/ospf.h
index d0286f72af029745424d86ed2e6603cca70aa465..db55aa6a1d8cebc0b4f638b761f6c0247d089813 100644 (file)
--- a/proto/ospf/ospf.h
+++ b/proto/ospf/ospf.h
@@ -191,6 +191,7 @@ struct ospf_iface_patt
   u8 link_lsa_suppression;
   u8 real_bcast;               /* Not really used in OSPFv3 */
   u8 ptp_netmask;              /* bool + 2 for unspecified */
+  u8 ptp_address;              /* bool + 2 for unspecified */
   u8 ttl_security;             /* bool + 2 for TX only */
   u8 bfd;
   list *passwords;
@@ -348,6 +349,7 @@ struct ospf_iface
   u8 ecmp_weight;              /* Weight used for ECMP */
   u8 link_lsa_suppression;     /* Suppression of Link-LSA origination */
   u8 ptp_netmask;              /* Send real netmask for P2P */
+  u8 ptp_address;              /* Send IP address in data field for PtP */
   u8 check_ttl;                        /* Check incoming packets for TTL 255 */
   u8 bfd;                      /* Use BFD on iface */
 };
@@ -1018,6 +1020,20 @@ struct nbma_node *find_nbma_node_(list *nnl, ip_addr ip);
 static inline struct nbma_node * find_nbma_node(struct ospf_iface *ifa, ip_addr ip)
 { return find_nbma_node_(&ifa->nbma_list, ip); }
 
+static inline u32 ospf_iface_get_data(struct ospf_iface *ifa)
+{
+  /*
+   * Return expected value of the link data field in Rt-LSA for given iface.
+   * It should be ifa->iface_id for unnumbered PtP links, IP address otherwise
+   * (see RFC 2328 12.4.1.1). It is controlled by ifa->ptp_address field so it
+   * can be overriden for compatibility purposes.
+   */
+
+  return ((ifa->type == OSPF_IT_PTP) && !ifa->ptp_address) ?
+    ifa->iface_id :
+    ipa_to_u32(ifa->addr->ip);
+}
+
 /* neighbor.c */
 struct ospf_neighbor *ospf_neighbor_new(struct ospf_iface *ifa);
 void ospf_neigh_sm(struct ospf_neighbor *n, int event);

diff --git a/proto/ospf/rt.c b/proto/ospf/rt.c
index b5787b540d6f9f104e213fc17d91ea7ea25b9f7a..b3e49a11540db58e79e09a1d2327c18d0d6510a5 100644 (file)
--- a/proto/ospf/rt.c
+++ b/proto/ospf/rt.c
@@ -395,12 +395,10 @@ px_pos_to_ifa(struct ospf_area *oa, int pos)
 static inline struct ospf_iface *
 rt_find_iface2(struct ospf_area *oa, uint data)
 {
-  ip_addr addr = ipa_from_u32(data);
-
   /* We should handle it differently for unnumbered PTP links */
   struct ospf_iface *ifa;
   WALK_LIST(ifa, oa->po->iface_list)
-    if ((ifa->oa == oa) && ifa->addr && (ipa_equal(ifa->addr->ip, addr)))
+    if ((ifa->oa == oa) && ifa->addr && (ospf_iface_get_data(ifa) == data))
       return ifa;
 
   return NULL;
@@ -420,7 +418,13 @@ rt_find_iface3(struct ospf_area *oa, uint lif)
 static struct ospf_iface *
 rt_find_iface(struct ospf_area *oa, int pos, uint data, uint lif)
 {
-  if (0)
+  /*
+   * We can use both position based lookup (which is more reliable) and data/lif
+   * based lookup (which works even during graceful restart). We will prefer the
+   * first approach, but use the second one for GR-enabled instances.
+   */
+
+  if (oa->po->gr_mode != OSPF_GR_ABLE)
     return rt_pos_to_ifa(oa, pos);
   else
     return ospf_is_v2(oa->po) ? rt_find_iface2(oa, data) : rt_find_iface3(oa, lif);

diff --git a/proto/ospf/topology.c b/proto/ospf/topology.c
index c8ec730a1e44d581bb2da4827e069c35ebf78292..f59db49d554355c3a1f76e4ccf4c1a0f304ef5bf 100644 (file)
--- a/proto/ospf/topology.c
+++ b/proto/ospf/topology.c
@@ -797,14 +797,11 @@ prepare_rt2_lsa_body(struct ospf_proto *p, struct ospf_area *oa)
        if (neigh->state == NEIGHBOR_FULL)
        {
          /*
-          * ln->data should be ifa->iface_id in case of no/ptp
-          * address (ifa->addr->flags & IA_PEER) on PTP link (see
-          * RFC 2328 12.4.1.1.), but the iface ID value has no use,
-          * while using IP address even in this case is here for
-          * compatibility with some broken implementations that use
-          * this address as a next-hop.
+          * ln->data field should be ifa->iface_id for unnumbered PtP links,
+          * IP address otherwise (see RFC 2328 12.4.1.1). This is controlled
+          * by ifa->ptp_address field.
           */
-         add_rt2_lsa_link(p, LSART_PTP, neigh->rid, ipa_to_u32(ifa->addr->ip), link_cost);
+         add_rt2_lsa_link(p, LSART_PTP, neigh->rid, ospf_iface_get_data(ifa), link_cost);
          i++;
        }
       break;