lib: Fix corruption when routemap delete/add sequence happens

If a operator issues a series of route-map deletions and
then re-adds, *and* this triggers the hash table to realloc
to grow to a larger size, then subsuquent route-map operations
will be against a corrupted hash table.

Why?

Effectively the route-map code was inserting each
route-map <NAME> into a hash for storage.  Upon
deletion there is this concept of delayed processing
so the routemap code sets a bit `to-be-processed`
and marks the route-map for deletion.  This is
1 entry in the hash table.  Then if the operator
recreates the hash, FRR would add another hash
entry.  If another deletion happens then there
now are 2 deletion entries that are indistinguishable
from a hash perspective.

FRR stores the deleted name of the route-map so that
any delayed processing can lookup the name and only process
those peers that are related to that route-map name.
This is good as that if in say BGP, we do not want
to reprocess all the peers that don't use the route-map.

Solution:
The whole purpose of the delay of deletion and the
storage of the route-map is to allow the using protocol
the ability to process the route-map at a later time
while still retaining the route-map name( for more efficient
reprocessing ).  The problem exists because we are keeping
multiple copies of deletion events that are indistinguishable
from each other causing hash havoc.

The truth is that we only need to keep 1 copy of the
routemap in the table.  If the series of events is:
a) delete ( schedule processing )
b) add ( reschedule processing )

Current code ends up processing the route-map two times
and in this event we really just need to reprocess everything
with the new route-map.

If the series of events is:
a) delete (schedule processing )
b) add (reschedule)
c) delete (reschedule)
d) add (reschedule)

All this really points to is that FRR just needs to keep the last
in the series of maps and ensuring that FRR knows that we need
to continue processing the route-map.  So in the creation processing
if the hash has an entry for this map, the routemap code knows that
this is a deletion event.  Mark this route-map for later processing
if it was marked so.  Also in the lookup function do not return
a map if the map found was deleted.

Fixes: #10708
Signed-off-by: Donald Sharp <sharpd@nvidia.com>

diff --git a/lib/routemap.c b/lib/routemap.c
index 7f733c8114aaebb8aeaafc8110f9a9396d9923d3..b1e33c788f078e0ad09af99b9e9f953520a0284b 100644 (file)
--- a/lib/routemap.c
+++ b/lib/routemap.c
@@ -100,6 +100,7 @@ static void route_map_del_plist_entries(afi_t afi,
                                        struct prefix_list_entry *entry);
 
 static struct hash *route_map_get_dep_hash(route_map_event_t event);
+static void route_map_free_map(struct route_map *map);
 
 struct route_map_match_set_hooks rmap_match_set_hook;
 
@@ -566,15 +567,8 @@ static bool route_map_hash_cmp(const void *p1, const void *p2)
        const struct route_map *map1 = p1;
        const struct route_map *map2 = p2;
 
-       if (map1->deleted == map2->deleted) {
-               if (map1->name && map2->name) {
-                       if (!strcmp(map1->name, map2->name)) {
-                               return true;
-                       }
-               } else if (!map1->name && !map2->name) {
-                       return true;
-               }
-       }
+       if (!strcmp(map1->name, map2->name))
+               return true;
 
        return false;
 }
@@ -636,13 +630,25 @@ static struct route_map *route_map_new(const char *name)
 /* Add new name to route_map. */
 static struct route_map *route_map_add(const char *name)
 {
-       struct route_map *map;
+       struct route_map *map, *exist;
        struct route_map_list *list;
 
        map = route_map_new(name);
        list = &route_map_master;
 
-       /* Add map to the hash */
+       /*
+        * Add map to the hash
+        *
+        * If the map already exists in the hash, then we know that
+        * FRR is now in a sequence of delete/create.
+        * All FRR needs to do here is set the to_be_processed
+        * bit (to inherit from the old one
+        */
+       exist = hash_release(route_map_master_hash, map);
+       if (exist) {
+               map->to_be_processed = exist->to_be_processed;
+               route_map_free_map(exist);
+       }
        hash_get(route_map_master_hash, map, hash_alloc_intern);
 
        /* Add new entry to the head of the list to match how it is added in the
@@ -752,11 +758,15 @@ struct route_map *route_map_lookup_by_name(const char *name)
        if (!name)
                return NULL;
 
-       // map.deleted is 0 via memset
+       // map.deleted is false via memset
        memset(&tmp_map, 0, sizeof(struct route_map));
        tmp_map.name = XSTRDUP(MTYPE_ROUTE_MAP_NAME, name);
        map = hash_lookup(route_map_master_hash, &tmp_map);
        XFREE(MTYPE_ROUTE_MAP_NAME, tmp_map.name);
+
+       if (map && map->deleted)
+               return NULL;
+
        return map;
 }