platform: support weakly tracked routing rules in NMPRulesManager

Policy routing rules are global, and unlike routes not tied to an interface by ifindex.
That means, while we take full control over all routes of an interface during a sync,
we need to consider that multiple parties can contribute to the global set of rules.
That might be muliple connection profiles providing the same rule, or rules that are added
externally by the user. NMPRulesManager mediates for that.

This is done by NMPRulesManager "tracking" rules.

Rules that are not tracked by NMPRulesManager are completely ignored (and
considered externally added).

When tracking a rule, the caller provides a track-priority. If multiple
parties track a rule, then the highest (absolute value of the) priority
wins.

If the highest track-priority is positive, NMPRulesManager will add the rule if
it's not present.

When the highest track-priority is negative, then NMPRulesManager will remove the
rule if it's present (enforce its absence).

The complicated part is, when a rule that was previously tracked becomes no
longer tracked. In that case, we need to restore the previous state.

If NetworkManager added the rule earlier, then untracking the rule
NMPRulesManager will remove the rule again (restore its previous absent
state).

By default, if NetworkManager had a negative tracking-priority and removed the
rule earlier (enforced it to be absent), then when the rule becomes no
longer tracked, NetworkManager will not restore the rule.
Consider: the user adds a rule externally, and then activates a profile that
enforces the absence of the rule (causing NetworkManager to remove it).
When deactivating the profile, by default NetworkManager will not
restore such a rule! It's unclear whether that is a good idea, but it's
also unclear why the rule is there and whether NetworkManager should
really restore it.

Add weakly tracked rules to account for that. A tracking-priority of
zero indicates such weakly tracked rules. The only difference between an untracked
rule and a weakly tracked rule is, that when NetworkManager earlier removed the
rule (due to a negative tracking-priority), it *will* restore weakly
tracked rules when the rules becomes no longer (negatively) tracked.
And it attmpts to do that only once.

Likewise, if the rule is weakly tracked and already exists when
NMPRulesManager starts posively tracking the rule, then it would not
remove again, when no longer positively tracking it.

2 files changed, 65 insertions, 23 deletions

diff --git a/src/nm-netns.c b/src/nm-netns.c
index e7aca6f66c..4552fcc136 100644
--- a/src/nm-netns.c
+++ b/src/nm-netns.c
@@ -127,10 +127,18 @@ constructed (GObject *object)
 	priv->platform_netns = nm_platform_netns_get (priv->platform);
 
 	priv->rules_manager = nmp_rules_manager_new (priv->platform);
+
+	/* Weakly track the default rules and rules that were added
+	 * outside of NetworkManager. */
 	nmp_rules_manager_track_default (priv->rules_manager,
 	                                 AF_UNSPEC,
 	                                 0,
 	                                 nm_netns_parent_class /* static dummy user-tag */);
+	nmp_rules_manager_track_from_platform (priv->rules_manager,
+	                                       NULL,
+	                                       AF_UNSPEC,
+	                                       0,
+	                                       nm_netns_parent_class /* static dummy user-tag */);
 
 	G_OBJECT_CLASS (nm_netns_parent_class)->constructed (object);
 }
diff --git a/src/platform/nmp-rules-manager.c b/src/platform/nmp-rules-manager.c
index 33218b982c..aceed31dac 100644
--- a/src/platform/nmp-rules-manager.c
+++ b/src/platform/nmp-rules-manager.c
@@ -76,26 +76,44 @@ typedef struct {
 	CList obj_lst;
 	CList user_tag_lst;
 
+	/* track_priority_val zero is special: those are weakly tracked rules.
+	 * That means: NetworkManager will restore them only if it removed them earlier.
+	 * But it will not remove or add them otherwise.
+	 *
+	 * Otherwise, the track_priority_val goes together with track_priority_present.
+	 * In case of one rule being tracked multile times (with different priorities),
+	 * the one with higher priority wins. See _rules_obj_get_best_data().
+	 * Then, the winning present state either enforces that the rule is present
+	 * or absent.
+	 *
+	 * If a rules is not tracked at all, it is ignored by NetworkManager. Assuming
+	 * that it was added externally by the user. But unlike weakly tracked rules,
+	 * NM will *not* restore such rules if NetworkManager themself removed them. */
 	guint32 track_priority_val;
 	bool track_priority_present:1;
 
 	bool dirty:1;
 } RulesData;
 
+typedef enum {
+	CONFIG_STATE_NONE          = 0,
+	CONFIG_STATE_ADDED_BY_US   = 1,
+	CONFIG_STATE_REMOVED_BY_US = 2,
+} ConfigState;
+
 typedef struct {
 	const NMPObject *obj;
 	CList obj_lst_head;
 
-	/* indicates that we configured the rule (during sync()). We need that, so
-	 * if the rule gets untracked, that we know to remove it on the next
-	 * sync().
+	/* indicates whether we configured/removed the rule (during sync()). We need that, so
+	 * if the rule gets untracked, that we know to remove/restore it.
 	 *
 	 * This makes NMPRulesManager stateful (beyond the configuration that indicates
 	 * which rules are tracked).
 	 * After a restart, NetworkManager would no longer remember which rules were added
 	 * by us. That would need to be fixed by persisting the state and reloading it after
 	 * restart. */
-	bool added_by_us:1;
+	ConfigState config_state;
 } RulesObjData;
 
 typedef struct {
@@ -164,8 +182,6 @@ _rules_obj_get_best_data (RulesObjData *obj_data)
 	RulesData *rules_data;
 	const RulesData *rd_best = NULL;
 
-	nm_assert (!c_list_is_empty (&obj_data->obj_lst_head));
-
 	c_list_for_each_entry (rules_data, &obj_data->obj_lst_head, obj_lst) {
 
 		_rules_data_assert (rules_data, TRUE);
@@ -175,8 +191,11 @@ _rules_obj_get_best_data (RulesObjData *obj_data)
 				continue;
 			if (rd_best->track_priority_val == rules_data->track_priority_val) {
 				if (   rd_best->track_priority_present
-				    || !rules_data->track_priority_present)
+				    || !rules_data->track_priority_present) {
+					/* if the priorities are identical, then "present" wins over
+					 * "!present" (absent). */
 					continue;
+				}
 			}
 		}
 
@@ -313,7 +332,7 @@ nmp_rules_manager_track (NMPRulesManager *self,
 			*obj_data = (RulesObjData) {
 				.obj          = nmp_object_ref (rules_data->obj),
 				.obj_lst_head = C_LIST_INIT (obj_data->obj_lst_head),
-				.added_by_us  = FALSE,
+				.config_state = CONFIG_STATE_NONE,
 			};
 			g_hash_table_add (self->by_obj, obj_data);
 		}
@@ -343,9 +362,13 @@ nmp_rules_manager_track (NMPRulesManager *self,
 	_rules_data_assert (rules_data, TRUE);
 
 	if (changed) {
-		_LOGD ("routing-rule: track ["NM_HASH_OBFUSCATE_PTR_FMT",%c%u] \"%s\")",
+		_LOGD ("routing-rule: track ["NM_HASH_OBFUSCATE_PTR_FMT",%s%u] \"%s\")",
 		       _USER_TAG_LOG (rules_data->user_tag),
-		       rules_data->track_priority_present ? '+' : '-',
+		       ( rules_data->track_priority_val == 0
+		        ? ""
+		        : (  rules_data->track_priority_present
+		           ? "+"
+		           : "-")),
 		       (guint) rules_data->track_priority_val,
 		       nmp_object_to_string (rules_data->obj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0));
 	}
@@ -387,9 +410,9 @@ _rules_data_untrack (NMPRulesManager *self,
 	nm_assert (c_list_contains (&obj_data->obj_lst_head, &rules_data->obj_lst));
 	nm_assert (obj_data == g_hash_table_lookup (self->by_obj, &rules_data->obj));
 
-	/* if obj_data is marked to be "added_by_us", we need to keep this entry around
-	 * for the next sync -- so that we can remove the rule that was added. */
-	if (   !obj_data->added_by_us
+	/* if obj_data is marked to be "added_by_us" or "removed_by_us", we need to keep this entry
+	 * around for the next sync -- so that we can undo what we did earlier. */
+	if (   obj_data->config_state == CONFIG_STATE_NONE
 	    && c_list_length_is (&rules_data->obj_lst, 1))
 		g_hash_table_remove (self->by_obj, &rules_data->obj);
 
@@ -480,6 +503,7 @@ nmp_rules_manager_sync (NMPRulesManager *self,
 	RulesObjData *obj_data;
 	GHashTableIter h_iter;
 	guint i;
+	const RulesData *rd_best;
 
 	g_return_if_fail (NMP_IS_RULES_MANAGER (self));
 
@@ -499,13 +523,15 @@ nmp_rules_manager_sync (NMPRulesManager *self,
 				continue;
 			}
 
-			if (c_list_is_empty (&obj_data->obj_lst_head)) {
-				nm_assert (obj_data->added_by_us);
-				g_hash_table_remove (self->by_obj, obj_data);
-			} else {
-				if (_rules_obj_get_best_data (obj_data)->track_priority_present)
+			rd_best = _rules_obj_get_best_data (obj_data);
+			if (rd_best) {
+				if (rd_best->track_priority_present)
 					continue;
-				obj_data->added_by_us = FALSE;
+				if (rd_best->track_priority_val == 0) {
+					if (obj_data->config_state != CONFIG_STATE_ADDED_BY_US)
+						continue;
+					obj_data->config_state = CONFIG_STATE_NONE;
+				}
 			}
 
 			if (keep_deleted_rules) {
@@ -517,6 +543,8 @@ nmp_rules_manager_sync (NMPRulesManager *self,
 				rules_to_delete = g_ptr_array_new_with_free_func ((GDestroyNotify) nmp_object_unref);
 
 			g_ptr_array_add (rules_to_delete, (gpointer) nmp_object_ref (plobj));
+
+			obj_data->config_state = CONFIG_STATE_REMOVED_BY_US;
 		}
 	}
 
@@ -528,20 +556,26 @@ nmp_rules_manager_sync (NMPRulesManager *self,
 	g_hash_table_iter_init (&h_iter, self->by_obj);
 	while (g_hash_table_iter_next (&h_iter, (gpointer *) &obj_data, NULL)) {
 
-		if (c_list_is_empty (&obj_data->obj_lst_head)) {
-			nm_assert (obj_data->added_by_us);
+		rd_best = _rules_obj_get_best_data (obj_data);
+
+		if (!rd_best) {
 			g_hash_table_iter_remove (&h_iter);
 			continue;
 		}
 
-		if (!_rules_obj_get_best_data (obj_data)->track_priority_present)
+		if (!rd_best->track_priority_present)
 			continue;
+		if (rd_best->track_priority_val == 0) {
+			if (obj_data->config_state != CONFIG_STATE_REMOVED_BY_US)
+				continue;
+			obj_data->config_state = CONFIG_STATE_NONE;
+		}
 
 		plobj = nm_platform_lookup_obj (self->platform, NMP_CACHE_ID_TYPE_OBJECT_TYPE, obj_data->obj);
 		if (plobj)
 			continue;
 
-		obj_data->added_by_us = TRUE;
+		obj_data->config_state = CONFIG_STATE_ADDED_BY_US;
 		nm_platform_routing_rule_add (self->platform, NMP_NLM_FLAG_ADD, NMP_OBJECT_CAST_ROUTING_RULE (obj_data->obj));
 	}
 }