policy-routing: take ownership of externally configured rulesth/routing-rule-suppress-prefixlength

IP addresses, routes, TC and QDiscs are all tied to a certain interface.
So when NetworkManager manages an interface, it can be confident that
the entires should be managed, deleted and modified by NetworkManager.

Routing policy rules are global. For that we have NMPRulesManager which
keeps track of who owns a rule. This allows multiple connection profiles
specify the same rule, and NMPRulesManager can consolidate this
information to know whether to add or remove the rule.

NMPRulesManager would also support to explicitly block a rule, but that
is currently not used. All that devices do is to add rules (track) and
remove them (untrack) once the profile gets deactivated.

As rules are not exclusively owned by NetworkManager, NetworkManager
tries not to interfere with rules that it knows nothing about. That
means in particular, when NetworkManager starts it will "weakly track"
all rules that are present. "weakly track" is mostly interesting for two
cases:

  - when NMPRulesManager had the same rule explicitly tracked (added) by a
    device, then deactivating the device will leave the rule in place.

  - when NMPRulesManager had the same rule explicitly blocked (tracked
    with negative priority), then it would restore the rule when that
    block gets removed (as said, currently nobody actually does this).

Note that when restarting NetworkManager, then the device may stay and
the rules kept. However after restart, NetworkManager no longer knows
that it previously added this route, so it would weakly track it and
never remove them again.

That is a problem. Avoid that, by whenever explicitly tracking a rule we
also make sure to no longer weakly track it. Most likely this rule was
indeed previously managed by NetworkManager. If this was really a rule
added by externally, then the user really should choose distinct
rule priority to avoid such conflict altogether.

5 files changed, 109 insertions, 28 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 6a27469d41..8175456b82 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6618,10 +6618,15 @@ _routing_rules_sync (NMDevice *self,
 
 				rule = nm_setting_ip_config_get_routing_rule (s_ip, i);
 				nm_ip_routing_rule_to_platform (rule, &plrule);
+
+				/* We track this rule, but we also make it explicitly not weakly-tracked
+				 * (meaning to untrack NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG at
+				 * the same time). */
 				nmp_rules_manager_track (rules_manager,
 				                         &plrule,
 				                         10,
-				                         user_tag);
+				                         user_tag,
+				                         NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG);
 			}
 		}
 	}
diff --git a/src/nm-netns.c b/src/nm-netns.c
index de822151af..f5669c6617 100644
--- a/src/nm-netns.c
+++ b/src/nm-netns.c
@@ -127,17 +127,27 @@ constructed (GObject *object)
 
 	priv->rules_manager = nmp_rules_manager_new (priv->platform);
 
-	/* Weakly track the default rules and rules that were added
-	 * outside of NetworkManager. */
+	/* Weakly track the default rules with a dummy user-tag. These
+	 * rules are always weekly tracked... */
 	nmp_rules_manager_track_default (priv->rules_manager,
 	                                 AF_UNSPEC,
 	                                 0,
 	                                 nm_netns_parent_class /* static dummy user-tag */);
+
+	/* Also weakly track all existing route. These were added before NetworkManager
+	 * starts, so it's probably none of NetworkManager's business.
+	 *
+	 * However note that during service restart, devices may stay up and rules kept.
+	 * That means, after restart such rules may have been added by a previous run
+	 * of NetworkManager, we just don't know.
+	 *
+	 * For that reason, whenever we will touch such rules later one, we make them
+	 * fully owned and no longer weekly tracked. See %NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG. */
 	nmp_rules_manager_track_from_platform (priv->rules_manager,
 	                                       NULL,
 	                                       AF_UNSPEC,
 	                                       0,
-	                                       nm_netns_parent_class /* static dummy user-tag */);
+	                                       NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_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 970afcde50..a097b6dfb6 100644
--- a/src/platform/nmp-rules-manager.c
+++ b/src/platform/nmp-rules-manager.c
@@ -99,6 +99,17 @@ typedef enum {
 	CONFIG_STATE_NONE          = 0,
 	CONFIG_STATE_ADDED_BY_US   = 1,
 	CONFIG_STATE_REMOVED_BY_US = 2,
+
+	/* ConfigState encodes whether the rule was touched by us at all (CONFIG_STATE_NONE).
+	 *
+	 * Maybe we would only need to track whether we touched the rule at all. But we
+	 * track it more in detail what we did: did we add it (CONFIG_STATE_ADDED_BY_US)
+	 * or did we remove it (CONFIG_STATE_REMOVED_BY_US)?
+	 * Finally, we need CONFIG_STATE_OWNED_BY_US, which means that we didn't actively
+	 * add/remove it, but whenever we are about to undo the add/remove, we need to do it.
+	 * In that sense, CONFIG_STATE_OWNED_BY_US is really just a flag that we unconditionally
+	 * force the state next time when necessary. */
+	CONFIG_STATE_OWNED_BY_US   = 3,
 } ConfigState;
 
 typedef struct {
@@ -111,8 +122,10 @@ typedef struct {
 	 * 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. */
+	 * by us.
+	 *
+	 * That is partially fixed by NetworkManager taking over the rules that it
+	 * actively configures (see NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG). */
 	ConfigState config_state;
 } RulesObjData;
 
@@ -121,6 +134,15 @@ typedef struct {
 	CList user_tag_lst_head;
 } RulesUserTagData;
 
+/*****************************************************************************/
+
+static void _rules_data_untrack (NMPRulesManager *self,
+                                 RulesData *rules_data,
+                                 gboolean remove_user_tag_data,
+                                 gboolean make_owned_by_us);
+
+/*****************************************************************************/
+
 static void
 _rules_data_assert (const RulesData *rules_data, gboolean linked)
 {
@@ -282,7 +304,8 @@ void
 nmp_rules_manager_track (NMPRulesManager *self,
                          const NMPlatformRoutingRule *routing_rule,
                          gint32 track_priority,
-                         gconstpointer user_tag)
+                         gconstpointer user_tag,
+                         gconstpointer user_tag_untrack)
 {
 	NMPObject obj_stack;
 	const NMPObject *p_obj_stack;
@@ -359,6 +382,17 @@ nmp_rules_manager_track (NMPRulesManager *self,
 		}
 	}
 
+	if (user_tag_untrack) {
+		if (user_tag != user_tag_untrack) {
+			RulesData *rules_data_untrack;
+
+			rules_data_untrack = _rules_data_lookup (self->by_data, p_obj_stack, user_tag_untrack);
+			if (rules_data_untrack)
+				_rules_data_untrack (self, rules_data_untrack, FALSE, TRUE);
+		} else
+			nm_assert_not_reached ();
+	}
+
 	_rules_data_assert (rules_data, TRUE);
 
 	if (changed) {
@@ -377,7 +411,8 @@ nmp_rules_manager_track (NMPRulesManager *self,
 static void
 _rules_data_untrack (NMPRulesManager *self,
                      RulesData *rules_data,
-                     gboolean remove_user_tag_data)
+                     gboolean remove_user_tag_data,
+                     gboolean make_owned_by_us)
 {
 	RulesObjData *obj_data;
 
@@ -401,15 +436,22 @@ _rules_data_untrack (NMPRulesManager *self,
 #endif
 
 	nm_assert (!c_list_is_empty (&rules_data->user_tag_lst));
-	if (   remove_user_tag_data
-	    && c_list_length_is (&rules_data->user_tag_lst, 1))
-		g_hash_table_remove (self->by_user_tag, &rules_data->user_tag);
 
 	obj_data = g_hash_table_lookup (self->by_obj, &rules_data->obj);
 	nm_assert (obj_data);
 	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 (make_owned_by_us) {
+		if (obj_data->config_state == CONFIG_STATE_NONE) {
+			/* we need to mark this entry that it requires a touch on the next
+			 * sync. */
+			obj_data->config_state = CONFIG_STATE_OWNED_BY_US;
+		}
+	} else if (   remove_user_tag_data
+	           && c_list_length_is (&rules_data->user_tag_lst, 1))
+		g_hash_table_remove (self->by_user_tag, &rules_data->user_tag);
+
 	/* 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
@@ -440,7 +482,7 @@ nmp_rules_manager_untrack (NMPRulesManager *self,
 
 	rules_data = _rules_data_lookup (self->by_data, p_obj_stack, user_tag);
 	if (rules_data)
-		_rules_data_untrack (self, rules_data, TRUE);
+		_rules_data_untrack (self, rules_data, TRUE, FALSE);
 }
 
 void
@@ -486,7 +528,7 @@ nmp_rules_manager_untrack_all (NMPRulesManager *self,
 	c_list_for_each_entry_safe (rules_data, rules_data_safe, &user_tag_data->user_tag_lst_head, user_tag_lst) {
 		if (   all
 		    || rules_data->dirty)
-			_rules_data_untrack (self, rules_data, FALSE);
+			_rules_data_untrack (self, rules_data, FALSE, FALSE);
 	}
 	if (c_list_is_empty (&user_tag_data->user_tag_lst_head))
 		g_hash_table_remove (self->by_user_tag, user_tag_data);
@@ -525,11 +567,17 @@ nmp_rules_manager_sync (NMPRulesManager *self,
 
 			rd_best = _rules_obj_get_best_data (obj_data);
 			if (rd_best) {
-				if (rd_best->track_priority_present)
+				if (rd_best->track_priority_present) {
+					if (obj_data->config_state == CONFIG_STATE_OWNED_BY_US)
+						obj_data->config_state = CONFIG_STATE_ADDED_BY_US;
 					continue;
+				}
 				if (rd_best->track_priority_val == 0) {
-					if (obj_data->config_state != CONFIG_STATE_ADDED_BY_US)
+					if (!NM_IN_SET (obj_data->config_state, CONFIG_STATE_ADDED_BY_US,
+					                                        CONFIG_STATE_OWNED_BY_US)) {
+						obj_data->config_state = CONFIG_STATE_NONE;
 						continue;
+					}
 					obj_data->config_state = CONFIG_STATE_NONE;
 				}
 			}
@@ -563,11 +611,17 @@ nmp_rules_manager_sync (NMPRulesManager *self,
 			continue;
 		}
 
-		if (!rd_best->track_priority_present)
+		if (!rd_best->track_priority_present) {
+			if (obj_data->config_state == CONFIG_STATE_OWNED_BY_US)
+				obj_data->config_state = CONFIG_STATE_REMOVED_BY_US;
 			continue;
+		}
 		if (rd_best->track_priority_val == 0) {
-			if (obj_data->config_state != CONFIG_STATE_REMOVED_BY_US)
+			if (!NM_IN_SET (obj_data->config_state, CONFIG_STATE_REMOVED_BY_US,
+			                                        CONFIG_STATE_OWNED_BY_US)) {
+				obj_data->config_state = CONFIG_STATE_NONE;
 				continue;
+			}
 			obj_data->config_state = CONFIG_STATE_NONE;
 		}
 
@@ -610,7 +664,7 @@ nmp_rules_manager_track_from_platform (NMPRulesManager *self,
 		    && rr->addr_family != addr_family)
 			continue;
 
-		nmp_rules_manager_track (self, rr, tracking_priority, user_tag);
+		nmp_rules_manager_track (self, rr, tracking_priority, user_tag, NULL);
 	}
 }
 
@@ -638,7 +692,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
 		                         track_priority,
-		                         user_tag);
+		                         user_tag,
+		                         NULL);
 		nmp_rules_manager_track (self,
 		                         &((NMPlatformRoutingRule) {
 		                             .addr_family = AF_INET,
@@ -648,7 +703,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
 		                         track_priority,
-		                         user_tag);
+		                         user_tag,
+		                         NULL);
 		nmp_rules_manager_track (self,
 		                         &((NMPlatformRoutingRule) {
 		                             .addr_family = AF_INET,
@@ -658,7 +714,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
 		                         track_priority,
-		                         user_tag);
+		                         user_tag,
+		                         NULL);
 	}
 	if (NM_IN_SET (addr_family, AF_UNSPEC, AF_INET6)) {
 		nmp_rules_manager_track (self,
@@ -670,7 +727,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
 		                         track_priority,
-		                         user_tag);
+		                         user_tag,
+		                         NULL);
 		nmp_rules_manager_track (self,
 		                         &((NMPlatformRoutingRule) {
 		                             .addr_family = AF_INET6,
@@ -680,7 +738,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self,
 		                             .protocol    = RTPROT_KERNEL,
 		                         }),
 		                         track_priority,
-		                         user_tag);
+		                         user_tag,
+		                         NULL);
 	}
 }
 
diff --git a/src/platform/nmp-rules-manager.h b/src/platform/nmp-rules-manager.h
index 310c7971f2..645df5c289 100644
--- a/src/platform/nmp-rules-manager.h
+++ b/src/platform/nmp-rules-manager.h
@@ -22,6 +22,8 @@
 
 /*****************************************************************************/
 
+#define NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG ((const void *) nmp_rules_manager_new)
+
 typedef struct _NMPRulesManager NMPRulesManager;
 
 NMPRulesManager *nmp_rules_manager_new (NMPlatform *platform);
@@ -35,7 +37,8 @@ NM_AUTO_DEFINE_FCN0 (NMPRulesManager *, _nmp_rules_manager_unref, nmp_rules_mana
 void nmp_rules_manager_track (NMPRulesManager *self,
                               const NMPlatformRoutingRule *routing_rule,
                               gint32 track_priority,
-                              gconstpointer user_tag);
+                              gconstpointer user_tag,
+                              gconstpointer user_tag_untrack);
 
 void nmp_rules_manager_track_default (NMPRulesManager *self,
                                       int addr_family,
diff --git a/src/platform/tests/test-route.c b/src/platform/tests/test-route.c
index 0d7fdcf338..44bfbc583b 100644
--- a/src/platform/tests/test-route.c
+++ b/src/platform/tests/test-route.c
@@ -1534,14 +1534,16 @@ again:
 			nmp_rules_manager_track (rules_manager,
 			                         NMP_OBJECT_CAST_ROUTING_RULE (objs_sync->pdata[i]),
 			                         1,
-			                         USER_TAG_1);
+			                         USER_TAG_1,
+			                         NULL);
 			if (nmtst_get_rand_bool ()) {
 				/* this has no effect, because a negative priority (of same absolute value)
 				 * has lower priority than the positive priority above. */
 				nmp_rules_manager_track (rules_manager,
 				                         NMP_OBJECT_CAST_ROUTING_RULE (objs_sync->pdata[i]),
 				                         -1,
-				                         USER_TAG_2);
+				                         USER_TAG_2,
+				                         NULL);
 			}
 			if (nmtst_get_rand_uint32 () % objs_sync->len == 0) {
 				nmp_rules_manager_sync (rules_manager, FALSE);
@@ -1566,13 +1568,15 @@ again:
 				nmp_rules_manager_track (rules_manager,
 				                         NMP_OBJECT_CAST_ROUTING_RULE (objs_sync->pdata[i]),
 				                         -1,
-				                         USER_TAG_1);
+				                         USER_TAG_1,
+				                         NULL);
 				break;
 			case 2:
 				nmp_rules_manager_track (rules_manager,
 				                         NMP_OBJECT_CAST_ROUTING_RULE (objs_sync->pdata[i]),
 				                         -2,
-				                         USER_TAG_2);
+				                         USER_TAG_2,
+				                         NULL);
 				break;
 			}
 			if (nmtst_get_rand_uint32 () % objs_sync->len == 0) {