Skip to content

Commit 6d1fd79

Browse files
authored
Merge pull request sonic-net#106 from mssonicbld/sonicbld/202205-merge
[code sync] Merge code from sonic-net/sonic-buildimage:202205 to 202205
2 parents 6dfcca3 + a7f974f commit 6d1fd79

2 files changed

Lines changed: 137 additions & 0 deletions

File tree

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
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+

src/sonic-frr/patch/series

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,4 @@ Disable-ipv6-src-address-test-in-pceplib.patch
1515
0028-bgpd-Ensure-that-bgp-open-message-stream-has-enough-data-to-read.patch
1616
0029-bgpd-Change-log-level-for-graceful-restart-events.patch
1717
0030-zebra-remove-duplicated-nexthops-when-sending-fpm-msg.patch
18+
0031-lib-Fix-corruption-when-routemap-delete-add-sequence.patch

0 commit comments

Comments
 (0)