matthieu/frr
1
0
Fork 0
forked from Mirror/frr
Code Pull requests Activity

zebra: set SELECTED before going into dplane code

Browse source 
There is a bit of an impedance mismatch in the sequence of events here.
Depending on the dplane behavior, the `ROUTE_ENTRY_SELECTED` bit will be
inconsistent for rib_process_result().

With an asynchronous dataplane:
0. rib_process() is called
1. rib_install_kernel() is called, dplane action is queued
2. rib_install_kernel() returns
3. rib_process() sets the SELECTED bit appropriately, returns
4. dplane is done, triggers rib_process_result()
5. SELECTED bit is seen in "after" state
(5a. NHT code looks at the SELECTED bit, works correctly.)

With a synchronous dataplane:
0. rib_process() is called
1. rib_install_kernel() is called, dplane action is executed
2. dplane (should) trigger rib_process_result()
3. SELECTED bit is seen in "before" state
(3a. NHT code looks at the SELECTED bit, fails.)
4. rib_install_kernel() returns
5. rib_process() sets the SELECTED bit appropriately, too late.

Essentially, poking the dataplane is a sequencing point where control is
handed over to the dplane.  Control may or may not return immediately.
Doing /anything/ after triggering the dataplane is a recipe for odd race
conditions.

(FWIW, I'm not sure rib_process_result() is called correctly in the
synchronous case, but that's a separate problem.)

Unfortunately, this change might have some unforeseen side effects.  I
haven't dug through the code to see if anything breaks.  There
/shouldn't/ be anything looking at the SELECTED bit here, but who knows.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
This commit is contained in:
David Lamparter 2021-10-21 17:54:42 +02:00
parent 3c8161eaa8
commit f68ce97627
1 changed files with 8 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
zebra/zebra_rib.c
View file

@ -1255,14 +1255,6 @@ static void rib_process(struct route_node *rn)
bool selected_changed = new_selected && CHECK_FLAG(new_selected->status, bool selected_changed = new_selected && CHECK_FLAG(new_selected->status,
ROUTE_ENTRY_CHANGED); ROUTE_ENTRY_CHANGED);
/* Update fib according to selection results */
if (new_fib && old_fib)
rib_process_update_fib(zvrf, rn, old_fib, new_fib);
else if (new_fib)
rib_process_add_fib(zvrf, rn, new_fib);
else if (old_fib)
rib_process_del_fib(zvrf, rn, old_fib);
/* Update SELECTED entry */ /* Update SELECTED entry */
if (old_selected != new_selected || selected_changed) { if (old_selected != new_selected || selected_changed) {
@ -1290,6 +1282,14 @@ static void rib_process(struct route_node *rn)
} }
} }
/* Update fib according to selection results */
if (new_fib && old_fib)
rib_process_update_fib(zvrf, rn, old_fib, new_fib);
else if (new_fib)
rib_process_add_fib(zvrf, rn, new_fib);
else if (old_fib)
rib_process_del_fib(zvrf, rn, old_fib);
/* Remove all RE entries queued for removal */ /* Remove all RE entries queued for removal */
RNODE_FOREACH_RE_SAFE (rn, re, next) { RNODE_FOREACH_RE_SAFE (rn, re, next) {
if (CHECK_FLAG(re->status, ROUTE_ENTRY_REMOVED)) { if (CHECK_FLAG(re->status, ROUTE_ENTRY_REMOVED)) {