zebra: separate zebra_vrf_lookup_table_with_id()

We were creating `other` tables in rib_del(), vty commands, and
dataplane return callback via the zebra_vrf_table_with_table_id()
API.

Seperate the API into only a lookup, never create
and added another with `get` in the name (following the standard
we use in other table APIs).

Then changed the rib_del(), rib_find_rn_from_ctx(), and show route
summary vty command to use the lookup API instead.

This was found via a crash where two different vrfs though they owned
the table. On delete, one free'd all the nodes, and then the other tried
to use them. It required specific timing of a VRF existing, going away,
and coming back again to cause the crash.

=23464== Invalid read of size 8
==23464==    at 0x179EA4: rib_dest_from_rnode (rib.h:433)
==23464==    by 0x17ACB1: zebra_vrf_delete (zebra_vrf.c:253)
==23464==    by 0x48F3D45: vrf_delete (vrf.c:243)
==23464==    by 0x48F4468: vrf_terminate (vrf.c:532)
==23464==    by 0x13D8C5: sigint (main.c:172)
==23464==    by 0x48DD25C: quagga_sigevent_process (sigevent.c:105)
==23464==    by 0x48F0502: thread_fetch (thread.c:1417)
==23464==    by 0x48AC82B: frr_run (libfrr.c:1023)
==23464==    by 0x13DD02: main (main.c:483)
==23464==  Address 0x5152788 is 104 bytes inside a block of size 112 free'd
==23464==    at 0x48369AB: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==23464==    by 0x48B25B8: qfree (memory.c:129)
==23464==    by 0x48EA335: route_node_destroy (table.c:500)
==23464==    by 0x48E967F: route_node_free (table.c:90)
==23464==    by 0x48E9742: route_table_free (table.c:124)
==23464==    by 0x48E9599: route_table_finish (table.c:60)
==23464==    by 0x170CEA: zebra_router_free_table (zebra_router.c:165)
==23464==    by 0x170DB4: zebra_router_release_table (zebra_router.c:188)
==23464==    by 0x17AAD2: zebra_vrf_disable (zebra_vrf.c:222)
==23464==    by 0x48F3F0C: vrf_disable (vrf.c:313)
==23464==    by 0x48F3CCF: vrf_delete (vrf.c:223)
==23464==    by 0x48F4468: vrf_terminate (vrf.c:532)
==23464==  Block was alloc'd at
==23464==    at 0x4837B65: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==23464==    by 0x48B24A2: qcalloc (memory.c:110)
==23464==    by 0x48EA2FE: route_node_create (table.c:488)
==23464==    by 0x48E95C7: route_node_new (table.c:66)
==23464==    by 0x48E95E5: route_node_set (table.c:75)
==23464==    by 0x48E9EA9: route_node_get (table.c:326)
==23464==    by 0x48E1EDB: srcdest_rnode_get (srcdest_table.c:244)
==23464==    by 0x16EA4B: rib_add_multipath (zebra_rib.c:2730)
==23464==    by 0x1A5310: zread_route_add (zapi_msg.c:1592)
==23464==    by 0x1A7B8E: zserv_handle_commands (zapi_msg.c:2579)
==23464==    by 0x19D689: zserv_process_messages (zserv.c:523)
==23464==    by 0x48F09F8: thread_call (thread.c:1599)

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>

diff --git a/zebra/redistribute.c b/zebra/redistribute.c
index 4e0163f8ac267765a62a97cfee21d2c1d29a39ee..98603c9693d1885c63c96457893b4a33ba529ec8 100644 (file)
--- a/zebra/redistribute.c
+++ b/zebra/redistribute.c
@@ -718,8 +718,8 @@ int zebra_import_table(afi_t afi, vrf_id_t vrf_id, uint32_t table_id,
        if (afi >= AFI_MAX)
                return (-1);
 
-       table = zebra_vrf_table_with_table_id(afi, SAFI_UNICAST, vrf_id,
-                                             table_id);
+       table = zebra_vrf_get_table_with_table_id(afi, SAFI_UNICAST, vrf_id,
+                                                 table_id);
        if (table == NULL) {
                return 0;
        } else if (IS_ZEBRA_DEBUG_RIB) {
@@ -830,8 +830,8 @@ static void zebra_import_table_rm_update_vrf_afi(struct zebra_vrf *zvrf,
        if ((!rmap_name) || (strcmp(rmap_name, rmap) != 0))
                return;
 
-       table = zebra_vrf_table_with_table_id(afi, SAFI_UNICAST,
-                                             zvrf->vrf->vrf_id, table_id);
+       table = zebra_vrf_get_table_with_table_id(afi, SAFI_UNICAST,
+                                                 zvrf->vrf->vrf_id, table_id);
        if (!table) {
                if (IS_ZEBRA_DEBUG_RIB_DETAILED)
                        zlog_debug("%s: Table id=%d not found", __func__,

diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
index e0bf1a58f244e273248b2cdf300480bea8ac0e21..c24a518afb2167379be65bb3bfba898d9a1a8849 100644 (file)
--- a/zebra/zebra_rib.c
+++ b/zebra/zebra_rib.c
@@ -1575,10 +1575,9 @@ rib_find_rn_from_ctx(const struct zebra_dplane_ctx *ctx)
 
        /* Locate rn and re(s) from ctx */
 
-       table = zebra_vrf_table_with_table_id(dplane_ctx_get_afi(ctx),
-                                             dplane_ctx_get_safi(ctx),
-                                             dplane_ctx_get_vrf(ctx),
-                                             dplane_ctx_get_table(ctx));
+       table = zebra_vrf_lookup_table_with_table_id(
+               dplane_ctx_get_afi(ctx), dplane_ctx_get_safi(ctx),
+               dplane_ctx_get_vrf(ctx), dplane_ctx_get_table(ctx));
        if (table == NULL) {
                if (IS_ZEBRA_DEBUG_DPLANE) {
                        zlog_debug("Failed to find route for ctx: no table for afi %d, safi %d, vrf %u",
@@ -2664,7 +2663,8 @@ int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p,
        assert(!src_p || !src_p->prefixlen || afi == AFI_IP6);
 
        /* Lookup table.  */
-       table = zebra_vrf_table_with_table_id(afi, safi, re->vrf_id, re->table);
+       table = zebra_vrf_get_table_with_table_id(afi, safi, re->vrf_id,
+                                                 re->table);
        if (!table) {
                if (re->ng)
                        nexthop_group_delete(&re->ng);
@@ -2809,7 +2809,8 @@ void rib_delete(afi_t afi, safi_t safi, vrf_id_t vrf_id, int type,
        assert(!src_p || !src_p->prefixlen || afi == AFI_IP6);
 
        /* Lookup table.  */
-       table = zebra_vrf_table_with_table_id(afi, safi, vrf_id, table_id);
+       table = zebra_vrf_lookup_table_with_table_id(afi, safi, vrf_id,
+                                                    table_id);
        if (!table)
                return;
 

diff --git a/zebra/zebra_vrf.c b/zebra/zebra_vrf.c
index f425c0e49e39465ca882ab5c7d4d03755edcb996..c392303760503c4b33812a66aeb408b6cceef46a 100644 (file)
--- a/zebra/zebra_vrf.c
+++ b/zebra/zebra_vrf.c
@@ -343,13 +343,12 @@ int zebra_vrf_has_config(struct zebra_vrf *zvrf)
  * - case VRF backend is default : on default VRF only
  * - case VRF backend is netns : on all VRFs
  */
-struct route_table *zebra_vrf_table_with_table_id(afi_t afi, safi_t safi,
-                                                 vrf_id_t vrf_id,
-                                                 uint32_t table_id)
+struct route_table *zebra_vrf_lookup_table_with_table_id(afi_t afi, safi_t safi,
+                                                        vrf_id_t vrf_id,
+                                                        uint32_t table_id)
 {
        struct zebra_vrf *zvrf = vrf_info_lookup(vrf_id);
        struct other_route_table ort, *otable;
-       struct route_table *table;
 
        if (!zvrf)
                return NULL;
@@ -364,9 +363,28 @@ struct route_table *zebra_vrf_table_with_table_id(afi_t afi, safi_t safi,
        ort.safi = safi;
        ort.table_id = table_id;
        otable = otable_find(&zvrf->other_tables, &ort);
+
        if (otable)
                return otable->table;
 
+       return NULL;
+}
+
+struct route_table *zebra_vrf_get_table_with_table_id(afi_t afi, safi_t safi,
+                                                     vrf_id_t vrf_id,
+                                                     uint32_t table_id)
+{
+       struct zebra_vrf *zvrf = vrf_info_lookup(vrf_id);
+       struct other_route_table *otable;
+       struct route_table *table;
+
+       table = zebra_vrf_lookup_table_with_table_id(afi, safi, vrf_id,
+                                                    table_id);
+
+       if (table)
+               goto done;
+
+       /* Create it as an `other` table */
        table = zebra_router_get_table(zvrf, table_id, afi, safi);
 
        otable = XCALLOC(MTYPE_OTHER_TABLE, sizeof(*otable));
@@ -376,6 +394,7 @@ struct route_table *zebra_vrf_table_with_table_id(afi_t afi, safi_t safi,
        otable->table = table;
        otable_add(&zvrf->other_tables, otable);
 
+done:
        return table;
 }
 

diff --git a/zebra/zebra_vrf.h b/zebra/zebra_vrf.h
index 6c80f9bcb449ac502da809a91cda1bc7326c3601..5448e170735aea719d8d1ff349297df22f2f976a 100644 (file)
--- a/zebra/zebra_vrf.h
+++ b/zebra/zebra_vrf.h
@@ -233,9 +233,13 @@ zvrf_other_table_compare_func(const struct other_route_table *a,
 DECLARE_RBTREE_UNIQ(otable, struct other_route_table, next,
                    zvrf_other_table_compare_func)
 
-struct route_table *zebra_vrf_table_with_table_id(afi_t afi, safi_t safi,
-                                                 vrf_id_t vrf_id,
-                                                 uint32_t table_id);
+extern struct route_table *
+zebra_vrf_lookup_table_with_table_id(afi_t afi, safi_t safi, vrf_id_t vrf_id,
+                                    uint32_t table_id);
+extern struct route_table *zebra_vrf_get_table_with_table_id(afi_t afi,
+                                                            safi_t safi,
+                                                            vrf_id_t vrf_id,
+                                                            uint32_t table_id);
 
 extern void zebra_vrf_update_all(struct zserv *client);
 extern struct zebra_vrf *zebra_vrf_lookup_by_id(vrf_id_t vrf_id);

diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c
index 9d174547305e77c186202ae5cfacf28b34530287..12517f3135adb76b573d7e83612dc8cc9c22b9e9 100644 (file)
--- a/zebra/zebra_vty.c
+++ b/zebra/zebra_vty.c
@@ -1708,10 +1708,9 @@ DEFPY (show_route_summary,
                        if ((zvrf = vrf->info) == NULL)
                                continue;
 
-                       table = zebra_vrf_table_with_table_id(afi,
-                                                             SAFI_UNICAST,
-                                                             zvrf->vrf->vrf_id,
-                                                             table_id);
+                       table = zebra_vrf_lookup_table_with_table_id(
+                               afi, SAFI_UNICAST, zvrf->vrf->vrf_id, table_id);
+
                        if (!table)
                                continue;
 
@@ -1726,9 +1725,8 @@ DEFPY (show_route_summary,
                if (vrf_name)
                        VRF_GET_ID(vrf_id, vrf_name, false);
 
-               table = zebra_vrf_table_with_table_id(afi,
-                                                     SAFI_UNICAST,
-                                                     vrf_id, table_id);
+               table = zebra_vrf_lookup_table_with_table_id(afi, SAFI_UNICAST,
+                                                            vrf_id, table_id);
                if (!table)
                        return CMD_SUCCESS;