platform: merge branch 'th/platform-rules-cleanup'

https://github.com/NetworkManager/NetworkManager/pull/341

4 files changed, 150 insertions, 67 deletions

diff --git a/src/nm-netns.c b/src/nm-netns.c
index ce32ac6b6f..4552fcc136 100644
--- a/src/nm-netns.c
+++ b/src/nm-netns.c
@@ -126,7 +126,19 @@ constructed (GObject *object)
 
 	priv->platform_netns = nm_platform_netns_get (priv->platform);
 
-	priv->rules_manager = nmp_rules_manager_new (priv->platform, TRUE);
+	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 9e3435507b..aceed31dac 100644
--- a/src/platform/nmp-rules-manager.c
+++ b/src/platform/nmp-rules-manager.c
@@ -33,7 +33,6 @@ struct _NMPRulesManager {
 	GHashTable *by_user_tag;
 	GHashTable *by_data;
 	guint ref_count;
-	bool track_default:1;
 };
 
 /*****************************************************************************/
@@ -77,26 +76,44 @@ typedef struct {
 	CList obj_lst;
 	CList user_tag_lst;
 
-	guint32 priority_val;
-	bool priority_present;
+	/* 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 {
@@ -165,19 +182,20 @@ _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);
 
 		if (rd_best) {
-			if (rd_best->priority_val > rules_data->priority_val)
+			if (rd_best->track_priority_val > rules_data->track_priority_val)
 				continue;
-			if (rd_best->priority_val == rules_data->priority_val) {
-				if (   rd_best->priority_present
-				    || !rules_data->priority_present)
+			if (rd_best->track_priority_val == rules_data->track_priority_val) {
+				if (   rd_best->track_priority_present
+				    || !rules_data->track_priority_present) {
+					/* if the priorities are identical, then "present" wins over
+					 * "!present" (absent). */
 					continue;
+				}
 			}
 		}
 
@@ -263,7 +281,7 @@ _rules_data_lookup (GHashTable *by_data,
 void
 nmp_rules_manager_track (NMPRulesManager *self,
                          const NMPlatformRoutingRule *routing_rule,
-                         gint32 priority,
+                         gint32 track_priority,
                          gconstpointer user_tag)
 {
 	NMPObject obj_stack;
@@ -272,13 +290,13 @@ nmp_rules_manager_track (NMPRulesManager *self,
 	RulesObjData *obj_data;
 	RulesUserTagData *user_tag_data;
 	gboolean changed = FALSE;
-	guint32 priority_val;
-	gboolean priority_present;
+	guint32 track_priority_val;
+	gboolean track_priority_present;
 
 	g_return_if_fail (NMP_IS_RULES_MANAGER (self));
 	g_return_if_fail (routing_rule);
 	g_return_if_fail (user_tag);
-	nm_assert (priority != G_MININT32);
+	nm_assert (track_priority != G_MININT32);
 
 	_rules_init (self);
 
@@ -286,12 +304,12 @@ nmp_rules_manager_track (NMPRulesManager *self,
 
 	nm_assert (nmp_object_is_visible (p_obj_stack));
 
-	if (priority >= 0) {
-		priority_val = priority;
-		priority_present = TRUE;
+	if (track_priority >= 0) {
+		track_priority_val = track_priority;
+		track_priority_present = TRUE;
 	} else {
-		priority_val = -priority;
-		priority_present = FALSE;
+		track_priority_val = -track_priority;
+		track_priority_present = FALSE;
 	}
 
 	rules_data = _rules_data_lookup (self->by_data, p_obj_stack, user_tag);
@@ -299,12 +317,12 @@ nmp_rules_manager_track (NMPRulesManager *self,
 	if (!rules_data) {
 		rules_data = g_slice_new (RulesData);
 		*rules_data = (RulesData) {
-			.obj              = nm_dedup_multi_index_obj_intern (nm_platform_get_multi_idx (self->platform),
-			                                                     p_obj_stack),
-			.user_tag         = user_tag,
-			.priority_val     = priority_val,
-			.priority_present = priority_present,
-			.dirty            = FALSE,
+			.obj                    = nm_dedup_multi_index_obj_intern (nm_platform_get_multi_idx (self->platform),
+			                                                           p_obj_stack),
+			.user_tag               = user_tag,
+			.track_priority_val     = track_priority_val,
+			.track_priority_present = track_priority_present,
+			.dirty                  = FALSE,
 		};
 		g_hash_table_add (self->by_data, rules_data);
 
@@ -314,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);
 		}
@@ -333,10 +351,10 @@ nmp_rules_manager_track (NMPRulesManager *self,
 		changed = TRUE;
 	} else {
 		rules_data->dirty = FALSE;
-		if (   rules_data->priority_val != priority_val
-		    || rules_data->priority_present != priority_present) {
-			rules_data->priority_val = priority_val;
-			rules_data->priority_present = priority_present;
+		if (   rules_data->track_priority_val != track_priority_val
+		    || rules_data->track_priority_present != track_priority_present) {
+			rules_data->track_priority_val = track_priority_val;
+			rules_data->track_priority_present = track_priority_present;
 			changed = TRUE;
 		}
 	}
@@ -344,10 +362,14 @@ 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->priority_present ? '+' : '-',
-		       (guint) rules_data->priority_val,
+		       ( 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));
 	}
 }
@@ -388,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);
 
@@ -481,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));
 
@@ -500,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)->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) {
@@ -518,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;
 		}
 	}
 
@@ -529,32 +556,76 @@ 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)->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));
 	}
 }
 
+void
+nmp_rules_manager_track_from_platform (NMPRulesManager *self,
+                                       NMPlatform *platform,
+                                       int addr_family,
+                                       gint32 tracking_priority,
+                                       gconstpointer user_tag)
+{
+	NMPLookup lookup;
+	const NMDedupMultiHeadEntry *head_entry;
+	NMDedupMultiIter iter;
+	const NMPObject *o;
+
+	g_return_if_fail (NMP_IS_RULES_MANAGER (self));
+
+	if (!platform)
+		platform = self->platform;
+	else
+		g_return_if_fail (NM_IS_PLATFORM (platform));
+
+	nm_assert (NM_IN_SET (addr_family, AF_UNSPEC, AF_INET, AF_INET6));
+
+	nmp_lookup_init_obj_type (&lookup, NMP_OBJECT_TYPE_ROUTING_RULE);
+	head_entry = nm_platform_lookup (platform, &lookup);
+	nmp_cache_iter_for_each (&iter, head_entry, &o) {
+		const NMPlatformRoutingRule *rr = NMP_OBJECT_CAST_ROUTING_RULE (o);
+
+		if (   addr_family != AF_UNSPEC
+		    && rr->addr_family != addr_family)
+			continue;
+
+		nmp_rules_manager_track (self, rr, tracking_priority, user_tag);
+	}
+}
+
 /*****************************************************************************/
 
 void
 nmp_rules_manager_track_default (NMPRulesManager *self,
                                  int addr_family,
-                                 int priority,
+                                 gint32 track_priority,
                                  gconstpointer user_tag)
 {
+	g_return_if_fail (NMP_IS_RULES_MANAGER (self));
+
+	nm_assert (NM_IN_SET (addr_family, AF_UNSPEC, AF_INET, AF_INET6));
+
 	/* track the default rules. See also `man ip-rule`. */
 
 	if (NM_IN_SET (addr_family, AF_UNSPEC, AF_INET)) {
@@ -566,7 +637,7 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .action      = FR_ACT_TO_TBL,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
-		                         priority,
+		                         track_priority,
 		                         user_tag);
 		nmp_rules_manager_track (self,
 		                         &((NMPlatformRoutingRule) {
@@ -576,7 +647,7 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .action      = FR_ACT_TO_TBL,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
-		                         priority,
+		                         track_priority,
 		                         user_tag);
 		nmp_rules_manager_track (self,
 		                         &((NMPlatformRoutingRule) {
@@ -586,7 +657,7 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .action      = FR_ACT_TO_TBL,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
-		                         priority,
+		                         track_priority,
 		                         user_tag);
 	}
 	if (NM_IN_SET (addr_family, AF_UNSPEC, AF_INET6)) {
@@ -598,7 +669,7 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .action      = FR_ACT_TO_TBL,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
-		                         priority,
+		                         track_priority,
 		                         user_tag);
 		nmp_rules_manager_track (self,
 		                         &((NMPlatformRoutingRule) {
@@ -608,7 +679,7 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .action      = FR_ACT_TO_TBL,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
-		                         priority,
+		                         track_priority,
 		                         user_tag);
 	}
 }
@@ -622,16 +693,12 @@ _rules_init (NMPRulesManager *self)
 	self->by_data      = g_hash_table_new_full (_rules_data_hash,      _rules_data_equal,      NULL, _rules_data_destroy);
 	self->by_obj       = g_hash_table_new_full (_rules_obj_hash,       _rules_obj_equal,       NULL, _rules_obj_destroy);
 	self->by_user_tag  = g_hash_table_new_full (_rules_user_tag_hash,  _rules_user_tag_equal,  NULL, _rules_user_tag_destroy);
-
-	if (self->track_default)
-		nmp_rules_manager_track_default (self, AF_UNSPEC, 0, &self->by_data);
 }
 
 /*****************************************************************************/
 
 NMPRulesManager *
-nmp_rules_manager_new (NMPlatform *platform,
-                       gboolean track_default)
+nmp_rules_manager_new (NMPlatform *platform)
 {
 	NMPRulesManager *self;
 
@@ -641,7 +708,6 @@ nmp_rules_manager_new (NMPlatform *platform,
 	*self = (NMPRulesManager) {
 		.ref_count     = 1,
 		.platform      = g_object_ref (platform),
-		.track_default = track_default,
 	};
 	return self;
 }
diff --git a/src/platform/nmp-rules-manager.h b/src/platform/nmp-rules-manager.h
index a1e5543548..310c7971f2 100644
--- a/src/platform/nmp-rules-manager.h
+++ b/src/platform/nmp-rules-manager.h
@@ -24,8 +24,7 @@
 
 typedef struct _NMPRulesManager NMPRulesManager;
 
-NMPRulesManager *nmp_rules_manager_new (NMPlatform *platform,
-                                        gboolean track_default);
+NMPRulesManager *nmp_rules_manager_new (NMPlatform *platform);
 
 void nmp_rules_manager_ref (NMPRulesManager *self);
 void nmp_rules_manager_unref (NMPRulesManager *self);
@@ -35,14 +34,20 @@ NM_AUTO_DEFINE_FCN0 (NMPRulesManager *, _nmp_rules_manager_unref, nmp_rules_mana
 
 void nmp_rules_manager_track (NMPRulesManager *self,
                               const NMPlatformRoutingRule *routing_rule,
-                              gint32 priority,
+                              gint32 track_priority,
                               gconstpointer user_tag);
 
 void nmp_rules_manager_track_default (NMPRulesManager *self,
                                       int addr_family,
-                                      int priority,
+                                      gint32 track_priority,
                                       gconstpointer user_tag);
 
+void nmp_rules_manager_track_from_platform (NMPRulesManager *self,
+                                            NMPlatform *platform,
+                                            int addr_family,
+                                            gint32 tracking_priority,
+                                            gconstpointer user_tag);
+
 void nmp_rules_manager_untrack (NMPRulesManager *self,
                                 const NMPlatformRoutingRule *routing_rule,
                                 gconstpointer user_tag);
diff --git a/src/platform/tests/test-route.c b/src/platform/tests/test-route.c
index ae3367779c..e6ffe1ae0c 100644
--- a/src/platform/tests/test-route.c
+++ b/src/platform/tests/test-route.c
@@ -1511,7 +1511,7 @@ again:
 
 	if (TEST_SYNC) {
 		gs_unref_hashtable GHashTable *unique_priorities = g_hash_table_new (NULL, NULL);
-		nm_auto_unref_rules_manager NMPRulesManager *rules_manager = nmp_rules_manager_new (platform, FALSE);
+		nm_auto_unref_rules_manager NMPRulesManager *rules_manager = nmp_rules_manager_new (platform);
 		gs_unref_ptrarray GPtrArray *objs_sync = NULL;
 		gconstpointer USER_TAG_1 = &platform;
 		gconstpointer USER_TAG_2 = &unique_priorities;