From 4a0167fee54750693c7d20c84636bcef34e1ebb8 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Fri, 8 Oct 2021 09:05:28 -0300 Subject: [PATCH 1/4] ospfd: fix flushing of Grace-LSAs on broadcast interfaces The ospfd opaque LSA infrastruture has an issue where it can't store different versions of the same Type-9 LSA for different interfaces. When flushing the self-originated Grace-LSAs upon exiting from the GR mode, the code was looking up the single self-originated Grace-LSA from the LSDB, setting its age to MaxAge and sending it out on all interfaces. The problem is that Grace-LSAs sent on broadcast interfaces have their own unique "IP interface address" TLV that is used to identify the restarting router. That way, just reusing the same Grace-LSA for all interfaces doesn't work. Fix this by generating a new Grace-LSA with its age manually set to MaxAge whenever one needs to be flushed. This will allow the "IP interface address" TLV to be set correctly and make GR work even in the presence of multiple broadcast interfaces. In the long term, the opaque LSA infrastructure should be updated to support Type-9 link-local LSAs correctly so that we don't need to resort to hacks like this. Signed-off-by: Renato Westphal --- ospfd/ospf_gr.c | 48 ++++++------------------------------------------ 1 file changed, 6 insertions(+), 42 deletions(-) diff --git a/ospfd/ospf_gr.c b/ospfd/ospf_gr.c index c108040303..fed8bddb5f 100644 --- a/ospfd/ospf_gr.c +++ b/ospfd/ospf_gr.c @@ -150,7 +150,7 @@ static struct ospf_lsa *ospf_gr_lsa_new(struct ospf_interface *oi) } /* Originate and install Grace-LSA for a given interface. */ -static void ospf_gr_lsa_originate(struct ospf_interface *oi) +static void ospf_gr_lsa_originate(struct ospf_interface *oi, bool maxage) { struct ospf_lsa *lsa, *old; @@ -164,6 +164,9 @@ static void ospf_gr_lsa_originate(struct ospf_interface *oi) return; } + if (maxage) + lsa->data->ls_age = htons(OSPF_LSA_MAXAGE); + /* Find the old LSA and increase the seqno. */ old = ospf_gr_lsa_lookup(oi->ospf, oi->area); if (old) @@ -183,37 +186,6 @@ static void ospf_gr_lsa_originate(struct ospf_interface *oi) ospf_flood_through_interface(oi, NULL, lsa); } -/* Flush a given self-originated Grace-LSA. */ -static struct ospf_lsa *ospf_gr_flush_grace_lsa(struct ospf_interface *oi, - struct ospf_lsa *old) -{ - struct ospf_lsa *lsa; - - if (ospf_interface_neighbor_count(oi) == 0) - return NULL; - - if (IS_DEBUG_OSPF_GR) - zlog_debug( - "GR: flushing self-originated Grace-LSAs [interface %s]", - oi->ifp->name); - - lsa = ospf_lsa_dup(old); - lsa->data->ls_age = htons(OSPF_LSA_MAXAGE); - lsa->data->ls_seqnum = lsa_seqnum_increment(lsa); - - /* Install updated LSA into LSDB. */ - if (ospf_lsa_install(oi->ospf, oi, lsa) == NULL) { - zlog_warn("%s: ospf_lsa_install() failed", __func__); - ospf_lsa_unlock(&lsa); - return NULL; - } - - /* Flood the LSA through out the interface */ - ospf_flood_through_interface(oi, NULL, lsa); - - return lsa; -} - /* Flush all self-originated Grace-LSAs. */ static void ospf_gr_flush_grace_lsas(struct ospf *ospf) { @@ -221,7 +193,6 @@ static void ospf_gr_flush_grace_lsas(struct ospf *ospf) struct listnode *anode; for (ALL_LIST_ELEMENTS_RO(ospf->areas, anode, area)) { - struct ospf_lsa *lsa; struct ospf_interface *oi; struct listnode *inode; @@ -230,15 +201,8 @@ static void ospf_gr_flush_grace_lsas(struct ospf *ospf) "GR: flushing self-originated Grace-LSAs [area %pI4]", &area->area_id); - lsa = ospf_gr_lsa_lookup(ospf, area); - if (!lsa) { - zlog_warn("%s: Grace-LSA not found [area %pI4]", - __func__, &area->area_id); - continue; - } - for (ALL_LIST_ELEMENTS_RO(area->oiflist, inode, oi)) - ospf_gr_flush_grace_lsa(oi, lsa); + ospf_gr_lsa_originate(oi, true); } } @@ -750,7 +714,7 @@ static void ospf_gr_prepare(void) /* Send a Grace-LSA to all neighbors. */ for (ALL_LIST_ELEMENTS_RO(ospf->oiflist, inode, oi)) - ospf_gr_lsa_originate(oi); + ospf_gr_lsa_originate(oi, false); /* Record end of the grace period in non-volatile memory. */ ospf_gr_nvm_update(ospf); From d6f60d2276de10d985deceeab5804d24de3c952b Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Sat, 9 Oct 2021 18:59:58 -0300 Subject: [PATCH 2/4] ospf6d: fix LSA name in debug message Signed-off-by: Renato Westphal --- ospf6d/ospf6_gr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ospf6d/ospf6_gr.c b/ospf6d/ospf6_gr.c index 40893ed998..c3e6f62f06 100644 --- a/ospf6d/ospf6_gr.c +++ b/ospf6d/ospf6_gr.c @@ -58,7 +58,7 @@ static int ospf6_gr_lsa_originate(struct ospf6_interface *oi) char buffer[OSPF6_MAX_LSASIZE]; if (IS_OSPF6_DEBUG_ORIGINATE(LINK)) - zlog_debug("Originate Link-LSA for Interface %s", + zlog_debug("Originate Grace-LSA for Interface %s", oi->interface->name); /* prepare buffer */ From eedc80c1f59c060d0f313dee9910f32f99c606c3 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Sat, 9 Oct 2021 20:02:16 -0300 Subject: [PATCH 3/4] ospfd: introduce additional opaque capability check in the GR code Before starting the graceful restart procedures, ospf_gr_prepare() verifies for each configured OSPF instance whether it has the opaque capability enabled (a pre-requisite for GR). If not, a warning is emitted and GR isn't performed on that instance. This PR introduces an additional opaque capability check that will return a CLI error when the opaque capability isn't enabled. The idea is to make it easier for the user to identify when the GR activation has failed, instead of requiring him or her to check the logs for errors. The original opaque capability check from ospf_gr_prepare() was retaining as it's possible that that function might be called from other contexts in the future. Signed-off-by: Renato Westphal --- ospfd/ospf_gr.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/ospfd/ospf_gr.c b/ospfd/ospf_gr.c index fed8bddb5f..aae7e33867 100644 --- a/ospfd/ospf_gr.c +++ b/ospfd/ospf_gr.c @@ -734,6 +734,18 @@ DEFPY(graceful_restart_prepare, graceful_restart_prepare_cmd, IP_STR "Prepare to restart the OSPF process") { + struct ospf *ospf; + struct listnode *node; + + for (ALL_LIST_ELEMENTS_RO(om->ospf, node, ospf)) { + if (!CHECK_FLAG(ospf->config, OSPF_OPAQUE_CAPABLE)) { + vty_out(vty, + "%% Can't start graceful restart: opaque capability not enabled (VRF %s)\n\n", + ospf_get_name(ospf)); + return CMD_WARNING; + } + } + ospf_gr_prepare(); return CMD_SUCCESS; From 3ebf9d34161833d5ab102d3d4f9107b1d8c01481 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Tue, 12 Oct 2021 16:08:23 -0300 Subject: [PATCH 4/4] ospfd: fix another DR election issue during graceful restart Commit 3551ee9e90304 introduced a regression that causes GR to fail under certain circumstances. In short, while ISM events should be ignored while acting as a helper for a restarting router, the DR/BDR fields of the neighbor structure should still be updated while processing a Hello packet. If that isn't done, it can cause the helper to elect the wrong DR while exiting from the helper mode, leading to a situation where there are two DRs for the same network segment (and a failed GR by consequence). Fix this. Signed-off-by: Renato Westphal --- ospfd/ospf_packet.c | 58 +++++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/ospfd/ospf_packet.c b/ospfd/ospf_packet.c index eacc30eba0..ea356a2bef 100644 --- a/ospfd/ospf_packet.c +++ b/ospfd/ospf_packet.c @@ -1096,39 +1096,41 @@ static void ospf_hello(struct ip *iph, struct ospf_header *ospfh, zlog_debug( "%s, Neighbor is under GR Restart, hence ignoring the ISM Events", __PRETTY_FUNCTION__); + } else { + /* If neighbor itself declares DR and no BDR exists, + cause event BackupSeen */ + if (IPV4_ADDR_SAME(&nbr->address.u.prefix4, &hello->d_router)) + if (hello->bd_router.s_addr == INADDR_ANY + && oi->state == ISM_Waiting) + OSPF_ISM_EVENT_SCHEDULE(oi, ISM_BackupSeen); - return; - } - - /* If neighbor itself declares DR and no BDR exists, - cause event BackupSeen */ - if (IPV4_ADDR_SAME(&nbr->address.u.prefix4, &hello->d_router)) - if (hello->bd_router.s_addr == INADDR_ANY - && oi->state == ISM_Waiting) + /* neighbor itself declares BDR. */ + if (oi->state == ISM_Waiting + && IPV4_ADDR_SAME(&nbr->address.u.prefix4, + &hello->bd_router)) OSPF_ISM_EVENT_SCHEDULE(oi, ISM_BackupSeen); - /* neighbor itself declares BDR. */ - if (oi->state == ISM_Waiting - && IPV4_ADDR_SAME(&nbr->address.u.prefix4, &hello->bd_router)) - OSPF_ISM_EVENT_SCHEDULE(oi, ISM_BackupSeen); + /* had not previously. */ + if ((IPV4_ADDR_SAME(&nbr->address.u.prefix4, &hello->d_router) + && IPV4_ADDR_CMP(&nbr->address.u.prefix4, &nbr->d_router)) + || (IPV4_ADDR_CMP(&nbr->address.u.prefix4, &hello->d_router) + && IPV4_ADDR_SAME(&nbr->address.u.prefix4, + &nbr->d_router))) + OSPF_ISM_EVENT_SCHEDULE(oi, ISM_NeighborChange); - /* had not previously. */ - if ((IPV4_ADDR_SAME(&nbr->address.u.prefix4, &hello->d_router) - && IPV4_ADDR_CMP(&nbr->address.u.prefix4, &nbr->d_router)) - || (IPV4_ADDR_CMP(&nbr->address.u.prefix4, &hello->d_router) - && IPV4_ADDR_SAME(&nbr->address.u.prefix4, &nbr->d_router))) - OSPF_ISM_EVENT_SCHEDULE(oi, ISM_NeighborChange); + /* had not previously. */ + if ((IPV4_ADDR_SAME(&nbr->address.u.prefix4, &hello->bd_router) + && IPV4_ADDR_CMP(&nbr->address.u.prefix4, &nbr->bd_router)) + || (IPV4_ADDR_CMP(&nbr->address.u.prefix4, + &hello->bd_router) + && IPV4_ADDR_SAME(&nbr->address.u.prefix4, + &nbr->bd_router))) + OSPF_ISM_EVENT_SCHEDULE(oi, ISM_NeighborChange); - /* had not previously. */ - if ((IPV4_ADDR_SAME(&nbr->address.u.prefix4, &hello->bd_router) - && IPV4_ADDR_CMP(&nbr->address.u.prefix4, &nbr->bd_router)) - || (IPV4_ADDR_CMP(&nbr->address.u.prefix4, &hello->bd_router) - && IPV4_ADDR_SAME(&nbr->address.u.prefix4, &nbr->bd_router))) - OSPF_ISM_EVENT_SCHEDULE(oi, ISM_NeighborChange); - - /* Neighbor priority check. */ - if (nbr->priority >= 0 && nbr->priority != hello->priority) - OSPF_ISM_EVENT_SCHEDULE(oi, ISM_NeighborChange); + /* Neighbor priority check. */ + if (nbr->priority >= 0 && nbr->priority != hello->priority) + OSPF_ISM_EVENT_SCHEDULE(oi, ISM_NeighborChange); + } /* Set neighbor information. */ nbr->priority = hello->priority;