|
| 1 | +From 5f503e5f5aecc946a168c87f3e02757deb65cbeb Mon Sep 17 00:00:00 2001 |
| 2 | +From: Donald Sharp < [email protected]> |
| 3 | +Date: Wed, 2 Mar 2022 15:41:54 -0500 |
| 4 | +Subject: [PATCH] lib: Fix corruption when routemap delete/add sequence happens |
| 5 | + |
| 6 | +If a operator issues a series of route-map deletions and |
| 7 | +then re-adds, *and* this triggers the hash table to realloc |
| 8 | +to grow to a larger size, then subsuquent route-map operations |
| 9 | +will be against a corrupted hash table. |
| 10 | + |
| 11 | +Why? |
| 12 | + |
| 13 | +Effectively the route-map code was inserting each |
| 14 | +route-map <NAME> into a hash for storage. Upon |
| 15 | +deletion there is this concept of delayed processing |
| 16 | +so the routemap code sets a bit `to-be-processed` |
| 17 | +and marks the route-map for deletion. This is |
| 18 | +1 entry in the hash table. Then if the operator |
| 19 | +recreates the hash, FRR would add another hash |
| 20 | +entry. If another deletion happens then there |
| 21 | +now are 2 deletion entries that are indistinguishable |
| 22 | +from a hash perspective. |
| 23 | + |
| 24 | +FRR stores the deleted name of the route-map so that |
| 25 | +any delayed processing can lookup the name and only process |
| 26 | +those peers that are related to that route-map name. |
| 27 | +This is good as that if in say BGP, we do not want |
| 28 | +to reprocess all the peers that don't use the route-map. |
| 29 | + |
| 30 | +Solution: |
| 31 | +The whole purpose of the delay of deletion and the |
| 32 | +storage of the route-map is to allow the using protocol |
| 33 | +the ability to process the route-map at a later time |
| 34 | +while still retaining the route-map name( for more efficient |
| 35 | +reprocessing ). The problem exists because we are keeping |
| 36 | +multiple copies of deletion events that are indistinguishable |
| 37 | +from each other causing hash havoc. |
| 38 | + |
| 39 | +The truth is that we only need to keep 1 copy of the |
| 40 | +routemap in the table. If the series of events is: |
| 41 | +a) delete ( schedule processing ) |
| 42 | +b) add ( reschedule processing ) |
| 43 | + |
| 44 | +Current code ends up processing the route-map two times |
| 45 | +and in this event we really just need to reprocess everything |
| 46 | +with the new route-map. |
| 47 | + |
| 48 | +If the series of events is: |
| 49 | +a) delete (schedule processing ) |
| 50 | +b) add (reschedule) |
| 51 | +c) delete (reschedule) |
| 52 | +d) add (reschedule) |
| 53 | + |
| 54 | +All this really points to is that FRR just needs to keep the last |
| 55 | +in the series of maps and ensuring that FRR knows that we need |
| 56 | +to continue processing the route-map. So in the creation processing |
| 57 | +if the hash has an entry for this map, the routemap code knows that |
| 58 | +this is a deletion event. Mark this route-map for later processing |
| 59 | +if it was marked so. Also in the lookup function do not return |
| 60 | +a map if the map found was deleted. |
| 61 | + |
| 62 | +Fixes: #10708 |
| 63 | +Signed-off-by: Donald Sharp < [email protected]> |
| 64 | + |
| 65 | +--- |
| 66 | + lib/routemap.c | 25 ++++++++++++++----------- |
| 67 | + 1 file changed, 14 insertions(+), 11 deletions(-) |
| 68 | + |
| 69 | + |
| 70 | +diff --git a/lib/routemap.c b/lib/routemap.c |
| 71 | +index 7f733c811..8f343ccd9 100644 |
| 72 | +--- a/lib/routemap.c |
| 73 | ++++ b/lib/routemap.c |
| 74 | +@@ -101,6 +101,7 @@ static void route_map_del_plist_entries(afi_t afi, |
| 75 | + |
| 76 | + static struct hash *route_map_get_dep_hash(route_map_event_t event); |
| 77 | + |
| 78 | ++static void route_map_free_map(struct route_map *map); |
| 79 | + struct route_map_match_set_hooks rmap_match_set_hook; |
| 80 | + |
| 81 | + /* match interface */ |
| 82 | +@@ -566,15 +567,8 @@ static bool route_map_hash_cmp(const void *p1, const void *p2) |
| 83 | + const struct route_map *map1 = p1; |
| 84 | + const struct route_map *map2 = p2; |
| 85 | + |
| 86 | +- if (map1->deleted == map2->deleted) { |
| 87 | +- if (map1->name && map2->name) { |
| 88 | +- if (!strcmp(map1->name, map2->name)) { |
| 89 | +- return true; |
| 90 | +- } |
| 91 | +- } else if (!map1->name && !map2->name) { |
| 92 | +- return true; |
| 93 | +- } |
| 94 | +- } |
| 95 | ++ if (!strcmp(map1->name, map2->name)) |
| 96 | ++ return true; |
| 97 | + |
| 98 | + return false; |
| 99 | + } |
| 100 | +@@ -636,13 +630,18 @@ static struct route_map *route_map_new(const char *name) |
| 101 | + /* Add new name to route_map. */ |
| 102 | + static struct route_map *route_map_add(const char *name) |
| 103 | + { |
| 104 | +- struct route_map *map; |
| 105 | ++ struct route_map *map, *exist; |
| 106 | + struct route_map_list *list; |
| 107 | + |
| 108 | + map = route_map_new(name); |
| 109 | + list = &route_map_master; |
| 110 | + |
| 111 | + /* Add map to the hash */ |
| 112 | ++ exist = hash_release(route_map_master_hash, map); |
| 113 | ++ if (exist) { |
| 114 | ++ map->to_be_processed = exist->to_be_processed; |
| 115 | ++ route_map_free_map(exist); |
| 116 | ++ } |
| 117 | + hash_get(route_map_master_hash, map, hash_alloc_intern); |
| 118 | + |
| 119 | + /* Add new entry to the head of the list to match how it is added in the |
| 120 | +@@ -752,11 +751,15 @@ struct route_map *route_map_lookup_by_name(const char *name) |
| 121 | + if (!name) |
| 122 | + return NULL; |
| 123 | + |
| 124 | +- // map.deleted is 0 via memset |
| 125 | ++ // map.deleted is false via memset |
| 126 | + memset(&tmp_map, 0, sizeof(struct route_map)); |
| 127 | + tmp_map.name = XSTRDUP(MTYPE_ROUTE_MAP_NAME, name); |
| 128 | + map = hash_lookup(route_map_master_hash, &tmp_map); |
| 129 | + XFREE(MTYPE_ROUTE_MAP_NAME, tmp_map.name); |
| 130 | ++ |
| 131 | ++ if (map && map->deleted) |
| 132 | ++ return NULL; |
| 133 | ++ |
| 134 | + return map; |
| 135 | + } |
| 136 | + |
0 commit comments