ospfd: Fix OSPF link-local opaque LSA crash and opaque memory corruption

  1. When an OSPF interface is deleted, remove the references in link-local
     LSA. Delete the LSA from the LSDB so that the callback has accessibily
     to the interface prior to deletion.
  2. Fix a double free for the opaque function table data structure.
  3. Assure that the opaque per-type information and opaque function table
     structures are removed at the same time since they have back pointers
     to one another.
  4. Add a topotest variation for the link-local opaque LSA crash.

Signed-off-by: Acee <aceelindem@gmail.com>

diff --git a/ospfd/ospf_interface.c b/ospfd/ospf_interface.c
index 3ee1db755077033645e5a32564c8f35112b40268..0a60fa364a87709e7c2183d2e4d06367c5caa725 100644 (file)
--- a/ospfd/ospf_interface.c
+++ b/ospfd/ospf_interface.c
@@ -339,6 +339,8 @@ void ospf_if_free(struct ospf_interface *oi)
 
        assert(oi->state == ISM_Down);
 
+       ospf_opaque_type9_lsa_if_cleanup(oi);
+
        ospf_opaque_type9_lsa_term(oi);
 
        QOBJ_UNREG(oi);
@@ -380,26 +382,6 @@ int ospf_if_is_up(struct ospf_interface *oi)
        return if_is_up(oi->ifp);
 }
 
-struct ospf_interface *ospf_if_exists(struct ospf_interface *oic)
-{
-       struct listnode *node;
-       struct ospf *ospf;
-       struct ospf_interface *oi;
-
-       if (!oic)
-               return NULL;
-
-       ospf = oic->ospf;
-       if (ospf == NULL)
-               return NULL;
-
-       for (ALL_LIST_ELEMENTS_RO(ospf->oiflist, node, oi))
-               if (oi == oic)
-                       return oi;
-
-       return NULL;
-}
-
 /* Lookup OSPF interface by router LSA posistion */
 struct ospf_interface *ospf_if_lookup_by_lsa_pos(struct ospf_area *area,
                                                 int lsa_pos)

diff --git a/ospfd/ospf_interface.h b/ospfd/ospf_interface.h
index e2290a881cc410b3ecc432f4bb84f1c68551e683..08a2b11273e1d7e2310cc078754379acda579433 100644 (file)
--- a/ospfd/ospf_interface.h
+++ b/ospfd/ospf_interface.h
@@ -300,7 +300,6 @@ extern int ospf_if_up(struct ospf_interface *oi);
 extern int ospf_if_down(struct ospf_interface *oi);
 
 extern int ospf_if_is_up(struct ospf_interface *oi);
-extern struct ospf_interface *ospf_if_exists(struct ospf_interface *oi);
 extern struct ospf_interface *ospf_if_lookup_by_lsa_pos(struct ospf_area *area,
                                                        int lsa_pos);
 extern struct ospf_interface *

diff --git a/ospfd/ospf_nsm.c b/ospfd/ospf_nsm.c
index 7c3f289e026f01b4d212f53d623cfb4a8df1766e..08aa1036861313ac94da269247a1ee7f0b1905e1 100644 (file)
--- a/ospfd/ospf_nsm.c
+++ b/ospfd/ospf_nsm.c
@@ -219,7 +219,7 @@ static int ospf_db_summary_add(struct ospf_neighbor *nbr, struct ospf_lsa *lsa)
        case OSPF_OPAQUE_LINK_LSA:
                /* Exclude type-9 LSAs that does not have the same "oi" with
                 * "nbr". */
-               if (ospf_if_exists(lsa->oi) != nbr->oi)
+               if (lsa->oi != nbr->oi)
                        return 0;
                break;
        case OSPF_OPAQUE_AREA_LSA:

diff --git a/ospfd/ospf_opaque.c b/ospfd/ospf_opaque.c
index 27f47a6d79a3d5f7f91a8494a1a053fdf6f3b065..86492c7b13a48b32946bd02134a0f5353833d6bc 100644 (file)
--- a/ospfd/ospf_opaque.c
+++ b/ospfd/ospf_opaque.c
@@ -427,10 +427,12 @@ void ospf_delete_opaque_functab(uint8_t lsa_type, uint8_t opaque_type)
                                if (functab->oipt != NULL)
                                        free_opaque_info_per_type(functab->oipt,
                                                                  true);
-                               /* Dequeue listnode entry from the list. */
+                               /* Dequeue listnode entry from the function table
+                                * list coreesponding to the opaque LSA type.
+                                * Note that the list deletion callback frees
+                                * the functab entry memory.
+                                */
                                listnode_delete(funclist, functab);
-
-                               XFREE(MTYPE_OSPF_OPAQUE_FUNCTAB, functab);
                                break;
                        }
                }
@@ -614,6 +616,17 @@ static void free_opaque_info_per_type(struct opaque_info_per_type *oipt,
                }
                listnode_delete(l, oipt);
        }
+
+       /*
+        * Delete the function table corresponding to the LSA type and opaque type
+        * as well. The pointer to the opaque per-type information structure in
+        * the function table structure be set to NULL to avoid recursion during
+        * deletion.
+        */
+       if (oipt->functab) {
+               oipt->functab->oipt = NULL;
+               ospf_delete_opaque_functab(oipt->lsa_type, oipt->opaque_type);
+       }
        XFREE(MTYPE_OPAQUE_INFO_PER_TYPE, oipt);
        return;
 }
@@ -746,6 +759,44 @@ int ospf_opaque_is_owned(struct ospf_lsa *lsa)
        return (oipt != NULL && lookup_opaque_info_by_id(oipt, lsa) != NULL);
 }
 
+/*
+ * Cleanup Link-Local LSAs assocaited with an interface that is being deleted.
+ * Since these LSAs are stored in the area link state database (LSDB) as opposed
+ * to a separate per-interface, they must be deleted from the area database.
+ * Since their flooding scope is solely the deleted OSPF interface, there is no
+ * need to attempt to flush them from the routing domain. For link local LSAs
+ * originated via the OSPF server API, LSA deletion before interface deletion
+ * is required so that the callback can access the OSPF interface address.
+ */
+void ospf_opaque_type9_lsa_if_cleanup(struct ospf_interface *oi)
+{
+       struct route_node *rn;
+       struct ospf_lsdb *lsdb;
+       struct ospf_lsa *lsa;
+
+       lsdb = oi->area->lsdb;
+       LSDB_LOOP (OPAQUE_LINK_LSDB(oi->area), rn, lsa)
+               /*
+                * While the LSA shouldn't be referenced on any LSA
+                * lists since the flooding scoped is confined to the
+                * interface being deleted, clear the pointer to the
+                * deleted interface for safety's sake after it is
+                * removed from the area LSDB.
+                */
+               if (lsa->oi == oi) {
+                       if (IS_DEBUG_OSPF_EVENT)
+                               zlog_debug("Delete Type-9 Opaque-LSA on interface delete: [opaque-type=%u, opaque-id=%x]",
+                                          GET_OPAQUE_TYPE(
+                                                  ntohl(lsa->data->id.s_addr)),
+                                          GET_OPAQUE_ID(ntohl(
+                                                  lsa->data->id.s_addr)));
+                       ospf_lsa_lock(lsa);
+                       ospf_lsdb_delete(lsdb, lsa);
+                       lsa->oi = NULL;
+                       ospf_lsa_unlock(&lsa);
+               }
+}
+
 /*------------------------------------------------------------------------*
  * Following are (vty) configuration functions for Opaque-LSAs handling.
  *------------------------------------------------------------------------*/

diff --git a/ospfd/ospf_opaque.h b/ospfd/ospf_opaque.h
index e0c78cd6dc95834d5b6216ab94ef4419bf41481c..54651f8f58a6c7f97f6fe4aca959a36e89ce139f 100644 (file)
--- a/ospfd/ospf_opaque.h
+++ b/ospfd/ospf_opaque.h
@@ -109,6 +109,7 @@ extern void ospf_opaque_term(void);
 extern void ospf_opaque_finish(void);
 extern int ospf_opaque_type9_lsa_init(struct ospf_interface *oi);
 extern void ospf_opaque_type9_lsa_term(struct ospf_interface *oi);
+extern void ospf_opaque_type9_lsa_if_cleanup(struct ospf_interface *oi);
 extern int ospf_opaque_type10_lsa_init(struct ospf_area *area);
 extern void ospf_opaque_type10_lsa_term(struct ospf_area *area);
 extern int ospf_opaque_type11_lsa_init(struct ospf *ospf);

diff --git a/tests/topotests/ospfapi/test_ospf_clientapi.py b/tests/topotests/ospfapi/test_ospf_clientapi.py
index 7a7ea85e2fd5a55a148659a70b550d1283ccce54..41c18df7d35d033960d9057d402931bfcde4df9f 100644 (file)
--- a/tests/topotests/ospfapi/test_ospf_clientapi.py
+++ b/tests/topotests/ospfapi/test_ospf_clientapi.py
@@ -1205,7 +1205,10 @@ def _test_opaque_interface_disable(tgen, apibin):
             }
         }
         test_func = partial(
-            topotest.router_json_cmp, r1, "show ip ospf interface json", r1_interface_without_opaque
+            topotest.router_json_cmp,
+            r1,
+            "show ip ospf interface json",
+            r1_interface_without_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r1 OSPF interface doesn't have opaque capability disabled"
@@ -1232,7 +1235,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 4 in test_ospf_opaque_interface_disable and STEP 59 in CI tests
         step("Verify that the r1 neighbor options don't include opaque")
         test_func = partial(
-            topotest.router_json_cmp, r1, "show ip ospf neighbor detail json", r1_neighbor_without_opaque
+            topotest.router_json_cmp,
+            r1,
+            "show ip ospf neighbor detail json",
+            r1_neighbor_without_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r1 OSPF neighbor has opaque option in optionsList"
@@ -1241,7 +1247,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 5 in test_ospf_opaque_interface_disable and STEP 60 in CI tests
         step("Verify that the r1 neighbor options don't include opaque")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf neighbor detail json", r2_neighbor_without_opaque
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf neighbor detail json",
+            r2_neighbor_without_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF neighbor has opaque option in optionsList"
@@ -1282,7 +1291,10 @@ def _test_opaque_interface_disable(tgen, apibin):
             }
         }
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf interface json", r2_interface_with_opaque
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf interface json",
+            r2_interface_with_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF interface has opaque capability disabled"
@@ -1338,19 +1350,17 @@ def _test_opaque_interface_disable(tgen, apibin):
             "asExternalOpaqueLsaCount": 1,
         }
         opaque_area_empty_database = {
-            "routerId":"2.0.0.0",
-            "areaLocalOpaqueLsa":{
-                "areas":{
-                    "1.2.3.4":[
-                    ]
-                }
-            }
+            "routerId": "2.0.0.0",
+            "areaLocalOpaqueLsa": {"areas": {"1.2.3.4": []}},
         }
 
         # STEP 9 in test_ospf_opaque_interface_disable and STEP 64 in CI tests
         step("Check that LSAs are added on r1")
         test_func = partial(
-            topotest.router_json_cmp, r1, "show ip ospf database json", opaque_LSAs_in_database
+            topotest.router_json_cmp,
+            r1,
+            "show ip ospf database json",
+            opaque_LSAs_in_database,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r1 OSPF database doesn't contain opaque LSAs"
@@ -1359,8 +1369,11 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 10 in test_ospf_opaque_interface_disable and STEP 65 in CI tests
         step("Check that LSAs are not added on r2")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf database opaque-area json",
-            opaque_area_empty_database, True
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf database opaque-area json",
+            opaque_area_empty_database,
+            True,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF area database contains opaque LSAs"
@@ -1382,7 +1395,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 13 in test_ospf_opaque_interface_disable and STEP 68 in CI tests
         step("Verify the ospf opaque option is applied to the r1 interface")
         test_func = partial(
-            topotest.router_json_cmp, r1, "show ip ospf interface json", r1_interface_with_opaque
+            topotest.router_json_cmp,
+            r1,
+            "show ip ospf interface json",
+            r1_interface_with_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r1 OSPF interface doesn't have opaque capability disabled"
@@ -1409,7 +1425,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 14 in test_ospf_opaque_interface_disable and STEP 69 in CI tests
         step("Verify that the r1 neighbor options include opaque")
         test_func = partial(
-            topotest.router_json_cmp, r1, "show ip ospf neighbor detail json", r1_neighbor_with_opaque
+            topotest.router_json_cmp,
+            r1,
+            "show ip ospf neighbor detail json",
+            r1_neighbor_with_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r1 OSPF neighbor doesn't have opaque option in optionsList"
@@ -1418,7 +1437,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 15 in test_ospf_opaque_interface_disable and STEP 70 in CI tests
         step("Verify that the r2 neighbor options include opaque")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf neighbor detail json", r2_neighbor_with_opaque
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf neighbor detail json",
+            r2_neighbor_with_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF neighbor doesn't have opaque option in optionsList"
@@ -1427,7 +1449,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 16 in test_ospf_opaque_interface_disable and STEP 71 in CI tests
         step("Check that LSAs are now added to r2")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf database json", opaque_LSAs_in_database
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf database json",
+            opaque_LSAs_in_database,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF database doesn't contains opaque LSAs"
@@ -1463,7 +1488,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 20 in test_ospf_opaque_interface_disable and STEP 75 in CI tests
         step("Verify the ospf opaque option is not applied to the r2 interface")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf interface json", r2_interface_without_opaque
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf interface json",
+            r2_interface_without_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r1 OSPF interface doesn't have opaque capability disabled"
@@ -1472,7 +1500,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 21 in test_ospf_opaque_interface_disable and STEP 76 in CI tests
         step("Verify that the r1 neighbor options don't include opaque")
         test_func = partial(
-            topotest.router_json_cmp, r1, "show ip ospf neighbor detail json", r1_neighbor_without_opaque
+            topotest.router_json_cmp,
+            r1,
+            "show ip ospf neighbor detail json",
+            r1_neighbor_without_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r1 OSPF neighbor has opaque option in optionsList"
@@ -1481,7 +1512,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 22 in test_ospf_opaque_interface_disable and STEP 77 in CI tests
         step("Verify that the r2 neighbor options don't include opaque")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf neighbor detail json", r2_neighbor_without_opaque
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf neighbor detail json",
+            r2_neighbor_without_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF neighbor has opaque option in optionsList"
@@ -1490,7 +1524,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 23 in test_ospf_opaque_interface_disable and STEP 78 in CI tests
         step("Verify that r1 still has the opaque LSAs")
         test_func = partial(
-            topotest.router_json_cmp, r1, "show ip ospf database json", opaque_LSAs_in_database
+            topotest.router_json_cmp,
+            r1,
+            "show ip ospf database json",
+            opaque_LSAs_in_database,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r1 OSPF database doesn't contain opaque LSAs"
@@ -1499,8 +1536,11 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 24 in test_ospf_opaque_interface_disable and STEP 79 in CI tests
         step("Verify that r2 doesn't have the opaque LSAs")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf database opaque-area json",
-            opaque_area_empty_database, True
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf database opaque-area json",
+            opaque_area_empty_database,
+            True,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF area database contains opaque LSAs"
@@ -1524,7 +1564,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 27 in test_ospf_opaque_interface_disable and STEP 82 in CI tests
         step("Verify the ospf opaque option is applied to the r2 interface")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf interface json", r2_interface_with_opaque
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf interface json",
+            r2_interface_with_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF interface doesn't have opaque capability disabled"
@@ -1533,7 +1576,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 28 in test_ospf_opaque_interface_disable and STEP 83 in CI tests
         step("Verify that the r2 neighbor options include opaque")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf neighbor detail json", r2_neighbor_with_opaque
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf neighbor detail json",
+            r2_neighbor_with_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF neighbor doesn't have opaque option in optionsList"
@@ -1542,7 +1588,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 29 in test_ospf_opaque_interface_disable and STEP 84 in CI tests
         step("Verify that the r1 neighbor options include opaque")
         test_func = partial(
-            topotest.router_json_cmp, r1, "show ip ospf neighbor detail json", r1_neighbor_with_opaque
+            topotest.router_json_cmp,
+            r1,
+            "show ip ospf neighbor detail json",
+            r1_neighbor_with_opaque,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r1 OSPF neighbor doesn't have opaque option in optionsList"
@@ -1551,7 +1600,10 @@ def _test_opaque_interface_disable(tgen, apibin):
         # STEP 30 in test_ospf_opaque_interface_disable and STEP 85 in CLI tests
         step("Verify that r2 now has the opaque LSAs")
         test_func = partial(
-            topotest.router_json_cmp, r2, "show ip ospf database json", opaque_LSAs_in_database
+            topotest.router_json_cmp,
+            r2,
+            "show ip ospf database json",
+            opaque_LSAs_in_database,
         )
         _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
         assertmsg = "r2 OSPF database doesn't contain opaque LSAs"
@@ -1581,6 +1633,89 @@ def test_ospf_opaque_interface_disable(tgen):
     _test_opaque_interface_disable(tgen, apibin)
 
 
+def _test_opaque_link_local_lsa_crash(tgen, apibin):
+    "Test disabling opaque capability on an interface"
+
+    r1 = tgen.gears["r1"]
+    r2 = tgen.gears["r2"]
+    tc_name = "opaque_interface_disable"
+
+    p = None
+    # Log to our stdin, stderr
+    pout = open(os.path.join(r1.net.logdir, "r1/intf-disable.log"), "a+")
+    try:
+        step("Add a link-local opaque LSA for r1-eth0")
+        pread = r1.popen([apibin, "-v", "add,9,10.0.1.1,230,1,feedaceedeadbeef"])
+
+        input_dict = {
+            "linkLocalOpaqueLsa": {
+                "areas": {
+                    "1.2.3.4": [
+                        {
+                            "linkStateId": "230.0.0.1",
+                            "advertisingRouter": "1.0.0.0",
+                            "lsaSeqNumber": "80000001",
+                            "opaqueData": "feedaceedeadbeef",
+                        },
+                    ],
+                }
+            },
+        }
+
+        # verify content
+        json_cmd = "show ip ospf da opaque-link json"
+        assert verify_ospf_database(tgen, r1, input_dict, json_cmd) is None
+
+        step("Shut down r1-eth0 and verify there is no crash")
+        r1.vtysh_multicmd("conf t\ninterface r1-eth0\nshut")
+        time.sleep(2)
+
+        step("Bring r1-eth0 back up and verify there is no crash")
+        r1.vtysh_multicmd("conf t\ninterface r1-eth0\nno shut")
+
+        step("Add another link-local opaque LSA for r1-eth0")
+        pread = r1.popen([apibin, "-v", "add,9,10.0.1.1,230,1,feedaceecafebeef"])
+
+        input_dict = {
+            "linkLocalOpaqueLsa": {
+                "areas": {
+                    "1.2.3.4": [
+                        {
+                            "linkStateId": "230.0.0.1",
+                            "advertisingRouter": "1.0.0.0",
+                            "lsaSeqNumber": "80000001",
+                            "opaqueData": "feedaceecafebeef",
+                        },
+                    ],
+                }
+            },
+        }
+        # verify content
+        json_cmd = "show ip ospf da opaque-link json"
+        assert verify_ospf_database(tgen, r1, input_dict, json_cmd) is None
+
+    except Exception:
+        if p:
+            p.terminate()
+            if p.wait():
+                comm_error(p)
+            p = None
+        raise
+    finally:
+        if p:
+            p.terminate()
+            p.wait()
+    p = None
+
+
+@pytest.mark.parametrize("tgen", [2], indirect=True)
+def test_ospf_opaque_link_local_lsa_crash(tgen):
+    apibin = os.path.join(CLIENTDIR, "ospfclient.py")
+    rc, o, e = tgen.gears["r2"].net.cmd_status([apibin, "--help"])
+    logging.debug("%s --help: rc: %s stdout: '%s' stderr: '%s'", apibin, rc, o, e)
+    _test_opaque_link_local_lsa_crash(tgen, apibin)
+
+
 if __name__ == "__main__":
     args = ["-s"] + sys.argv[1:]
     sys.exit(pytest.main(args))