l3cfg: handle dynamic added routes tracking and deletionth/tmp9

Dynamic added routes i.e ECMP single-hop routes, are not managed by l3cd
as the other ones. Therefore, they need to be tracked properly and
marked as dirty when they are.

This way, NetworkManager makes sure there are no leftovers from merging
ECMP multi-hop routes.

Usually, routes from the combined NML3ConfigData and are resolved early
during a commit. For example, _obj_states_update_all() creates for each
such route an obj_state_hash entry. Let's call those static, or
"non-dynamic".

Later, we can receive additional routes. We get them back from NMNetns
during nm_netns_ip_route_ecmp_commit() (_commit_collect_routes()).
Let's call them "dynamic".

For those routes, we also must track an obj-state. Because we need to
keep track about whether the route was configured by NML3Cfg (and we
must delete it, when it no longer shall be configured). For that we need
the state.

Now we have two reasons why an obj-state is tracked. The "non-dynamic"
and the "dynamic" one.

Add two flags "os_dynamic" and "os_non_dynamic", respectively and adjust
the code for that tracking.

1 files changed, 225 insertions, 128 deletions

diff --git a/src/core/nm-l3cfg.c b/src/core/nm-l3cfg.c
index 31abee8cac..eb13968196 100644
--- a/src/core/nm-l3cfg.c
+++ b/src/core/nm-l3cfg.c
@@ -165,7 +165,18 @@ typedef struct {
 
     /* This flag is only used temporarily to do a bulk update and
      * clear all the ones that are no longer in used. */
-    bool os_dirty : 1;
+    bool os_non_dynamic_dirty : 1;
+
+    /* Indicates that we have a object in combined_l3cd_commited that keeps the
+     * object state alive. */
+    bool os_non_dynamic : 1;
+
+    /* Indicates that there is a dynamic route from _commit_collect_routes(), that keeps the
+      * object state alive. */
+    bool os_dynamic : 1;
+
+    /* Indicates that this dynamic obj-state is marked as dirty. */
+    bool os_dynamic_dirty : 1;
 } ObjStateData;
 
 G_STATIC_ASSERT(G_STRUCT_OFFSET(ObjStateData, obj) == 0);
@@ -772,59 +783,70 @@ _nm_n_acd_data_probe_new(NML3Cfg *self, in_addr_t addr, guint32 timeout_msec, gp
 
 /*****************************************************************************/
 
-#define nm_assert_obj_state(self, obj_state)                                                     \
-    G_STMT_START                                                                                 \
-    {                                                                                            \
-        if (NM_MORE_ASSERTS > 0) {                                                               \
-            const NML3Cfg      *_self      = (self);                                             \
-            const ObjStateData *_obj_state = (obj_state);                                        \
-                                                                                                 \
-            nm_assert(_obj_state);                                                               \
-            nm_assert(NM_IN_SET(NMP_OBJECT_GET_TYPE(_obj_state->obj),                            \
-                                NMP_OBJECT_TYPE_IP4_ADDRESS,                                     \
-                                NMP_OBJECT_TYPE_IP6_ADDRESS,                                     \
-                                NMP_OBJECT_TYPE_IP4_ROUTE,                                       \
-                                NMP_OBJECT_TYPE_IP6_ROUTE));                                     \
-            nm_assert(!_obj_state->os_plobj || _obj_state->os_was_in_platform);                  \
-            nm_assert(_obj_state->os_failedobj_expiry_msec != 0                                  \
-                      || _obj_state->os_failedobj_prioq_idx == NM_PRIOQ_IDX_NULL);               \
-            nm_assert(_obj_state->os_failedobj_expiry_msec == 0 || !_obj_state->os_plobj);       \
-            nm_assert(_obj_state->os_failedobj_expiry_msec == 0                                  \
-                      || c_list_is_empty(&_obj_state->os_zombie_lst));                           \
-            nm_assert(_obj_state->os_failedobj_expiry_msec == 0 || _obj_state->obj);             \
-            if (_self) {                                                                         \
-                if (c_list_is_empty(&_obj_state->os_zombie_lst)) {                               \
-                    nm_assert(_self->priv.p->combined_l3cd_commited);                            \
-                                                                                                 \
-                    if (NM_MORE_ASSERTS > 5) {                                                   \
+#define nm_assert_obj_state(self, obj_state)                                                  \
+    G_STMT_START                                                                              \
+    {                                                                                         \
+        if (NM_MORE_ASSERTS > 0) {                                                            \
+            const NML3Cfg      *_self      = (self);                                          \
+            const ObjStateData *_obj_state = (obj_state);                                     \
+                                                                                              \
+            nm_assert(_obj_state);                                                            \
+            nm_assert(NM_IN_SET(NMP_OBJECT_GET_TYPE(_obj_state->obj),                         \
+                                NMP_OBJECT_TYPE_IP4_ADDRESS,                                  \
+                                NMP_OBJECT_TYPE_IP6_ADDRESS,                                  \
+                                NMP_OBJECT_TYPE_IP4_ROUTE,                                    \
+                                NMP_OBJECT_TYPE_IP6_ROUTE));                                  \
+            nm_assert(!_obj_state->os_plobj || _obj_state->os_was_in_platform);               \
+            nm_assert(_obj_state->os_failedobj_expiry_msec != 0                               \
+                      || _obj_state->os_failedobj_prioq_idx == NM_PRIOQ_IDX_NULL);            \
+            nm_assert(_obj_state->os_failedobj_expiry_msec == 0 || !_obj_state->os_plobj);    \
+            nm_assert(_obj_state->os_failedobj_expiry_msec == 0                               \
+                      || c_list_is_empty(&_obj_state->os_zombie_lst));                        \
+            nm_assert(_obj_state->os_failedobj_expiry_msec == 0 || _obj_state->obj);          \
+            nm_assert(!_obj_state->os_plobj                                                   \
+                      || NMP_OBJECT_GET_TYPE(_obj_state->obj)                                 \
+                             == NMP_OBJECT_GET_TYPE(_obj_state->os_plobj));                   \
+            if (_self) {                                                                      \
+                if (c_list_is_empty(&_obj_state->os_zombie_lst)) {                            \
+                    nm_assert(_self->priv.p->combined_l3cd_commited);                         \
+                                                                                              \
+                    if (NM_MORE_ASSERTS > 5) {                                                \
                         /* metric-any must be resolved before adding the object. Otherwise,
                          * their real metric is not known, and they cannot be compared to objects
-                         * from NMPlatform cache. */      \
-                        nm_assert(!NM_IN_SET(NMP_OBJECT_GET_TYPE(_obj_state->obj),               \
-                                             NMP_OBJECT_TYPE_IP4_ROUTE,                          \
-                                             NMP_OBJECT_TYPE_IP6_ROUTE)                          \
-                                  || !NMP_OBJECT_CAST_IP_ROUTE(_obj_state->obj)->metric_any);    \
-                                                                                                 \
-                        nm_assert(c_list_contains(&_self->priv.p->obj_state_lst_head,            \
-                                                  &_obj_state->os_lst));                         \
-                        nm_assert(_obj_state->os_plobj                                           \
-                                  == nm_platform_lookup_obj(_self->priv.platform,                \
-                                                            NMP_CACHE_ID_TYPE_OBJECT_TYPE,       \
-                                                            _obj_state->obj));                   \
-                        nm_assert(                                                               \
-                            c_list_is_empty(&obj_state->os_zombie_lst)                           \
-                                ? (_obj_state->obj                                               \
-                                   == nm_dedup_multi_entry_get_obj(nm_l3_config_data_lookup_obj( \
-                                       _self->priv.p->combined_l3cd_commited,                    \
-                                       _obj_state->obj)))                                        \
-                                : (!nm_l3_config_data_lookup_obj(                                \
-                                    _self->priv.p->combined_l3cd_commited,                       \
-                                    _obj_state->obj)));                                          \
-                    }                                                                            \
-                }                                                                                \
-            }                                                                                    \
-        }                                                                                        \
-    }                                                                                            \
+                         * from NMPlatform cache. */   \
+                        nm_assert(!NM_IN_SET(NMP_OBJECT_GET_TYPE(_obj_state->obj),            \
+                                             NMP_OBJECT_TYPE_IP4_ROUTE,                       \
+                                             NMP_OBJECT_TYPE_IP6_ROUTE)                       \
+                                  || !NMP_OBJECT_CAST_IP_ROUTE(_obj_state->obj)->metric_any); \
+                                                                                              \
+                        nm_assert(c_list_contains(&_self->priv.p->obj_state_lst_head,         \
+                                                  &_obj_state->os_lst));                      \
+                        nm_assert(_obj_state->os_plobj                                        \
+                                  == nm_platform_lookup_obj(_self->priv.platform,             \
+                                                            NMP_CACHE_ID_TYPE_OBJECT_TYPE,    \
+                                                            _obj_state->obj));                \
+                        if (!c_list_is_empty(&obj_state->os_zombie_lst)) {                    \
+                            nm_assert(!obj_state->os_non_dynamic);                            \
+                            nm_assert(!obj_state->os_non_dynamic_dirty);                      \
+                            nm_assert(!obj_state->os_dynamic);                                \
+                            nm_assert(!obj_state->os_dynamic_dirty);                          \
+                        }                                                                     \
+                        if (obj_state->os_non_dynamic) {                                      \
+                            nm_assert(                                                        \
+                                _obj_state->obj                                               \
+                                == nm_dedup_multi_entry_get_obj(nm_l3_config_data_lookup_obj( \
+                                    _self->priv.p->combined_l3cd_commited,                    \
+                                    _obj_state->obj)));                                       \
+                        } else {                                                              \
+                            nm_assert(!nm_l3_config_data_lookup_obj(                          \
+                                _self->priv.p->combined_l3cd_commited,                        \
+                                _obj_state->obj));                                            \
+                        }                                                                     \
+                    }                                                                         \
+                }                                                                             \
+            }                                                                                 \
+        }                                                                                     \
+    }                                                                                         \
     G_STMT_END
 
 static ObjStateData *
@@ -838,7 +860,10 @@ _obj_state_data_new(const NMPObject *obj, const NMPObject *plobj)
         .os_plobj                 = nmp_object_ref(plobj),
         .os_was_in_platform       = !!plobj,
         .os_nm_configured         = FALSE,
-        .os_dirty                 = FALSE,
+        .os_non_dynamic_dirty     = FALSE,
+        .os_non_dynamic           = FALSE,
+        .os_dynamic               = FALSE,
+        .os_dynamic_dirty         = FALSE,
         .os_failedobj_expiry_msec = 0,
         .os_failedobj_prioq_idx   = NM_PRIOQ_IDX_NULL,
         .os_zombie_lst            = C_LIST_INIT(obj_state->os_zombie_lst),
@@ -907,34 +932,6 @@ _obj_state_data_to_string(const ObjStateData *obj_state, char *buf, gsize buf_si
     return buf0;
 }
 
-static gboolean
-_obj_state_data_update(ObjStateData *obj_state, const NMPObject *obj)
-{
-    gboolean changed = FALSE;
-
-    nm_assert_obj_state(NULL, obj_state);
-    nm_assert(obj);
-    nm_assert(nmp_object_id_equal(obj_state->obj, obj));
-
-    obj_state->os_dirty = FALSE;
-
-    if (obj_state->obj != obj) {
-        nm_auto_nmpobj const NMPObject *obj_old = NULL;
-
-        if (!nmp_object_equal(obj_state->obj, obj))
-            changed = TRUE;
-        obj_old        = g_steal_pointer(&obj_state->obj);
-        obj_state->obj = nmp_object_ref(obj);
-    }
-
-    if (!c_list_is_empty(&obj_state->os_zombie_lst)) {
-        c_list_unlink(&obj_state->os_zombie_lst);
-        changed = TRUE;
-    }
-
-    return changed;
-}
-
 /*****************************************************************************/
 
 static void
@@ -1009,7 +1006,7 @@ out:
 }
 
 static void
-_obj_states_track_new(NML3Cfg *self, const NMPObject *obj)
+_obj_states_track_new(NML3Cfg *self, const NMPObject *obj, gboolean dynamic)
 {
     char          sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE];
     ObjStateData *obj_state;
@@ -1017,12 +1014,142 @@ _obj_states_track_new(NML3Cfg *self, const NMPObject *obj)
     obj_state = _obj_state_data_new(
         obj,
         nm_platform_lookup_obj(self->priv.platform, NMP_CACHE_ID_TYPE_OBJECT_TYPE, obj));
+    obj_state->os_dynamic     = dynamic;
+    obj_state->os_non_dynamic = !dynamic;
     c_list_link_tail(&self->priv.p->obj_state_lst_head, &obj_state->os_lst);
     g_hash_table_add(self->priv.p->obj_state_hash, obj_state);
-    _LOGD("obj-state: track: %s", _obj_state_data_to_string(obj_state, sbuf, sizeof(sbuf)));
+    _LOGD("obj-state: track%s: %s",
+          dynamic ? " (dynamic)" : "",
+          _obj_state_data_to_string(obj_state, sbuf, sizeof(sbuf)));
     nm_assert_obj_state(self, obj_state);
 }
 
+static gboolean
+_obj_states_track_update(NML3Cfg         *self,
+                         ObjStateData    *obj_state,
+                         const NMPObject *obj,
+                         gboolean         dynamic)
+{
+    gboolean changed = FALSE;
+
+    nm_assert_obj_state(NULL, obj_state);
+    nm_assert(obj);
+    nm_assert(nmp_object_id_equal(obj_state->obj, obj));
+
+    if (dynamic) {
+        if (!obj_state->os_dynamic)
+            changed = TRUE;
+        obj_state->os_dynamic_dirty = FALSE;
+        obj_state->os_dynamic       = TRUE;
+    } else {
+        if (!obj_state->os_non_dynamic)
+            changed = TRUE;
+        obj_state->os_non_dynamic_dirty = FALSE;
+        obj_state->os_non_dynamic       = TRUE;
+    }
+
+    if (obj_state->obj != obj && (!dynamic || !obj_state->os_non_dynamic)) {
+        nm_auto_nmpobj const NMPObject *obj_old = NULL;
+
+        if (!nmp_object_equal(obj_state->obj, obj))
+            changed = TRUE;
+        obj_old        = g_steal_pointer(&obj_state->obj);
+        obj_state->obj = nmp_object_ref(obj);
+    }
+
+    if (!c_list_is_empty(&obj_state->os_zombie_lst)) {
+        c_list_unlink(&obj_state->os_zombie_lst);
+        changed = TRUE;
+    }
+
+    if (changed) {
+        char sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE];
+
+        _LOGD("obj-state: update: %s (static: %d, dynamic: %d)",
+              _obj_state_data_to_string(obj_state, sbuf, sizeof(sbuf)),
+              !!obj_state->os_non_dynamic,
+              !!obj_state->os_dynamic);
+    }
+
+    nm_assert_obj_state(self, obj_state);
+    return changed;
+}
+
+static gboolean
+_obj_states_track_mark_dirty(NML3Cfg *self, gboolean dynamic)
+{
+    ObjStateData *obj_state;
+    gboolean      any_dirty = FALSE;
+
+    c_list_for_each_entry (obj_state, &self->priv.p->obj_state_lst_head, os_lst) {
+        if (!c_list_is_empty(&obj_state->os_zombie_lst)) {
+            /* we can ignore zombies. */
+            continue;
+        }
+        if (dynamic) {
+            if (!obj_state->os_dynamic)
+                continue;
+            obj_state->os_dynamic_dirty = TRUE;
+        } else {
+            if (!obj_state->os_non_dynamic)
+                continue;
+            obj_state->os_non_dynamic_dirty = TRUE;
+        }
+        any_dirty = TRUE;
+    }
+
+    return any_dirty;
+}
+
+static void
+_obj_states_track_prune_dirty(NML3Cfg *self, gboolean also_dynamic)
+{
+    GHashTableIter h_iter;
+    ObjStateData  *obj_state;
+    char           sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE];
+
+    g_hash_table_iter_init(&h_iter, self->priv.p->obj_state_hash);
+    while (g_hash_table_iter_next(&h_iter, (gpointer *) &obj_state, NULL)) {
+        if (!c_list_is_empty(&obj_state->os_zombie_lst)) {
+            /* The object is half-dead already and only kept for cleanup. But
+             * it does not need to be untracked. */
+            continue;
+        }
+
+        /* Resolve the "dirty" flags. */
+        if (obj_state->os_non_dynamic_dirty) {
+            obj_state->os_non_dynamic       = FALSE;
+            obj_state->os_non_dynamic_dirty = FALSE;
+        }
+        if (also_dynamic) {
+            if (obj_state->os_dynamic_dirty) {
+                obj_state->os_dynamic       = FALSE;
+                obj_state->os_dynamic_dirty = FALSE;
+            }
+        }
+
+        if (obj_state->os_non_dynamic || obj_state->os_dynamic) {
+            /* This obj-state is still alive. Keep it. */
+            continue;
+        }
+
+        if (obj_state->os_plobj && obj_state->os_nm_configured) {
+            nm_assert(obj_state->os_failedobj_prioq_idx == NM_PRIOQ_IDX_NULL);
+            c_list_link_tail(&self->priv.p->obj_state_zombie_lst_head, &obj_state->os_zombie_lst);
+            obj_state->os_zombie_count = ZOMBIE_COUNT_START;
+            _LOGD("obj-state: now zombie: %s",
+                  _obj_state_data_to_string(obj_state, sbuf, sizeof(sbuf)));
+            continue;
+        }
+
+        _LOGD("obj-state: untrack: %s", _obj_state_data_to_string(obj_state, sbuf, sizeof(sbuf)));
+        nm_prioq_remove(&self->priv.p->failedobj_prioq,
+                        obj_state,
+                        &obj_state->os_failedobj_prioq_idx);
+        g_hash_table_iter_remove(&h_iter);
+    }
+}
+
 static void
 _obj_states_update_all(NML3Cfg *self)
 {
@@ -1032,21 +1159,13 @@ _obj_states_update_all(NML3Cfg *self)
         NMP_OBJECT_TYPE_IP4_ROUTE,
         NMP_OBJECT_TYPE_IP6_ROUTE,
     };
-    char          sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE];
     ObjStateData *obj_state;
     int           i;
     gboolean      any_dirty = FALSE;
 
     nm_assert(NM_IS_L3CFG(self));
 
-    c_list_for_each_entry (obj_state, &self->priv.p->obj_state_lst_head, os_lst) {
-        if (!c_list_is_empty(&obj_state->os_zombie_lst)) {
-            /* we can ignore zombies. */
-            continue;
-        }
-        any_dirty           = TRUE;
-        obj_state->os_dirty = TRUE;
-    }
+    any_dirty = _obj_states_track_mark_dirty(self, FALSE);
 
     for (i = 0; i < (int) G_N_ELEMENTS(obj_types); i++) {
         const NMPObjectType obj_type = obj_types[i];
@@ -1072,47 +1191,16 @@ _obj_states_update_all(NML3Cfg *self)
             }
             obj_state = g_hash_table_lookup(self->priv.p->obj_state_hash, &obj);
             if (!obj_state) {
-                _obj_states_track_new(self, obj);
+                _obj_states_track_new(self, obj, FALSE);
                 continue;
             }
 
-            if (_obj_state_data_update(obj_state, obj)) {
-                _LOGD("obj-state: update: %s",
-                      _obj_state_data_to_string(obj_state, sbuf, sizeof(sbuf)));
-            }
-
-            nm_assert_obj_state(self, obj_state);
+            _obj_states_track_update(self, obj_state, obj, FALSE);
         }
     }
 
-    if (any_dirty) {
-        GHashTableIter h_iter;
-
-        g_hash_table_iter_init(&h_iter, self->priv.p->obj_state_hash);
-        while (g_hash_table_iter_next(&h_iter, (gpointer *) &obj_state, NULL)) {
-            if (!c_list_is_empty(&obj_state->os_zombie_lst))
-                continue;
-            if (!obj_state->os_dirty)
-                continue;
-
-            if (obj_state->os_plobj && obj_state->os_nm_configured) {
-                nm_assert(obj_state->os_failedobj_prioq_idx == NM_PRIOQ_IDX_NULL);
-                c_list_link_tail(&self->priv.p->obj_state_zombie_lst_head,
-                                 &obj_state->os_zombie_lst);
-                obj_state->os_zombie_count = ZOMBIE_COUNT_START;
-                _LOGD("obj-state: now zombie: %s",
-                      _obj_state_data_to_string(obj_state, sbuf, sizeof(sbuf)));
-                continue;
-            }
-
-            _LOGD("obj-state: untrack: %s",
-                  _obj_state_data_to_string(obj_state, sbuf, sizeof(sbuf)));
-            nm_prioq_remove(&self->priv.p->failedobj_prioq,
-                            obj_state,
-                            &obj_state->os_failedobj_prioq_idx);
-            g_hash_table_iter_remove(&h_iter);
-        }
-    }
+    if (any_dirty)
+        _obj_states_track_prune_dirty(self, FALSE);
 }
 
 typedef struct {
@@ -1288,7 +1376,9 @@ loop_done:
 
                 obj_state = g_hash_table_lookup(self->priv.p->obj_state_hash, &obj);
                 if (!obj_state)
-                    _obj_states_track_new(self, obj);
+                    _obj_states_track_new(self, obj, TRUE);
+                else
+                    _obj_states_track_update(self, obj_state, obj, TRUE);
 
                 if (!_obj_states_sync_filter(self, obj, commit_type))
                     continue;
@@ -4865,6 +4955,7 @@ _l3_commit_one(NML3Cfg              *self,
     NMIPRouteTableSyncMode       route_table_sync;
     char                         sbuf_commit_type[50];
     guint                        i;
+    gboolean                     any_dirty = FALSE;
 
     nm_assert(NM_IS_L3CFG(self));
     nm_assert(NM_IN_SET(commit_type,
@@ -4877,6 +4968,9 @@ _l3_commit_one(NML3Cfg              *self,
           nm_utils_addr_family_to_char(addr_family),
           _l3_cfg_commit_type_to_string(commit_type, sbuf_commit_type, sizeof(sbuf_commit_type)));
 
+    if (IS_IPv4)
+        any_dirty = _obj_states_track_mark_dirty(self, TRUE);
+
     addresses = _commit_collect_addresses(self, addr_family, commit_type);
 
     _commit_collect_routes(self,
@@ -4901,6 +4995,9 @@ _l3_commit_one(NML3Cfg              *self,
     if (route_table_sync == NM_IP_ROUTE_TABLE_SYNC_MODE_NONE)
         route_table_sync = NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN;
 
+    if (any_dirty)
+        _obj_states_track_prune_dirty(self, TRUE);
+
     if (commit_type == NM_L3_CFG_COMMIT_TYPE_REAPPLY) {
         gs_unref_array GArray *ipv6_temp_addrs_keep = NULL;