From: David Lamparter <equinox@opensourcerouting.org>
Date: Thu, 21 Oct 2021 15:54:42 +0000 (+0200)
Subject: zebra: set SELECTED before going into dplane code
X-Git-Tag: base_8.2~280^2
X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=f68ce9762785c0d33a4a983e6aa7e64c1107f198;p=mirror%2Ffrr.git

zebra: set SELECTED before going into dplane code

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>
---

diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
index fcac3328c9..84c78a3692 100644
--- a/zebra/zebra_rib.c
+++ b/zebra/zebra_rib.c
@@ -1255,14 +1255,6 @@ static void rib_process(struct route_node *rn)
 	bool selected_changed = new_selected && CHECK_FLAG(new_selected->status,
 							   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 */
 	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 */
 	RNODE_FOREACH_RE_SAFE (rn, re, next) {
 		if (CHECK_FLAG(re->status, ROUTE_ENTRY_REMOVED)) {