l3cfg: handle dynamic added routes tracking and deletionnm-1-44

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.

Without this, the state of those ECMP single hop routes is not properly
tracked, and they are for example not removed by NML3Cfg when they
should.

Usually, routes to be configured originate 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.

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"
to the ObjStateData and consider the flags at the necessary places.

Co-authored-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
(cherry picked from commit 5d26452da5cb151723561cb8db5466bb8df4dec5)

1 files changed, 235 insertions, 128 deletions

diff --git a/src/core/nm-l3cfg.c b/src/core/nm-l3cfg.c
index 8cc55dfb38..0aac4832ef 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];
@@ -1064,50 +1183,24 @@ _obj_states_update_all(NML3Cfg *self)
                 /* this is a nodev route. We don't track an obj-state for this. */
                 continue;
             }
-
+            if (obj_type == NMP_OBJECT_TYPE_IP4_ROUTE
+                && NMP_OBJECT_CAST_IP4_ROUTE(obj)->weight > 0) {
+                /* this route weight is bigger than 0, that means we don't know
+                 * which kind of route this will be. It can only be determined during commit. */
+                continue;
+            }
             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 {
@@ -1279,6 +1372,13 @@ loop_done:
         if (singlehop_routes) {
             for (i = 0; i < singlehop_routes->len; i++) {
                 const NMPObject *obj = singlehop_routes->pdata[i];
+                ObjStateData    *obj_state;
+
+                obj_state = g_hash_table_lookup(self->priv.p->obj_state_hash, &obj);
+                if (!obj_state)
+                    _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;
@@ -4855,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,
@@ -4867,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,
@@ -4891,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;