platform: fix tracking similar objects in NMPGlobalTracker

NMPGlobalTracker allows to track objects for independent users/callers.
That is, callers that are not aware whether another caller tracks the
same/similar object. It thus groups all objects by their nmp_object_id_equal()
(as `TrackObjData` struct), while keeping a list of each individually tracked
object (as `TrackData` struct which honors the object and the user-tag parameter).

When the same caller (based on the user-tag) tracks the same object again, then
NMPGlobalTracker will only track it once and combine the objects. That is done by
also having a dictionary for the `TrackData` entries (`self->by_data`).

This latter dictionary lookup wrongly considered nmp_object_id_equal().
Instead, it needs to consider all minor differences of the objects, and
use nmp_object_equal().

For example, for NMPlatformMptcpAddress, only the "address" is part of
the ID. Other fields, like the MPTCP flags are not. Imagine a profile is
active with MPTCP endpoints configured with flags "subflow". During reapply,
the user can only update the MPTCP flags (e.g. to "signal"). When that happens,
the caller (NML3Cfg) would track a new NMPlatformMptcpAddress instance, that only
differs by MPTCP flags. In this case, we need to track the new address for the
differences that it has according to nmp_object_equal(), and not
nmp_object_id_equal().

Due to this bug, reapply might not work correctly. For other supported types (routing
rules and routes) this bug may have been harder to reproduce, because most attributes
of rules/routes are also part of the ID and because it's uncommon to reapply a minor
change to a rule/route.

https://bugzilla.redhat.com/show_bug.cgi?id=2120471

Fixes: b8398b9e7948 ('platform: add NMPRulesManager for syncing routing rules')

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1375
(cherry picked from commit d8aacba3b2d68d25ad20bf34197e617723022975)
(cherry picked from commit c456bfa7c44a069d21a0c297c9d8a7d16e32788a)

1 files changed, 18 insertions, 2 deletions

diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c
index 842301b1bd..883e037a21 100644
--- a/src/libnm-platform/nmp-route-manager.c
+++ b/src/libnm-platform/nmp-route-manager.c
@@ -157,7 +157,7 @@ _track_data_hash(gconstpointer data)
     _track_data_assert(track_data, FALSE);
 
     nm_hash_init(&h, 269297543u);
-    nmp_object_id_hash_update(track_data->obj, &h);
+    nmp_object_hash_update(track_data->obj, &h);
     nm_hash_update_val(&h, track_data->user_tag);
     return nm_hash_complete(&h);
 }
@@ -172,7 +172,7 @@ _track_data_equal(gconstpointer data_a, gconstpointer data_b)
     _track_data_assert(track_data_b, FALSE);
 
     return track_data_a->user_tag == track_data_b->user_tag
-           && nmp_object_id_equal(track_data_a->obj, track_data_b->obj);
+           && nmp_object_equal(track_data_a->obj, track_data_b->obj);
 }
 
 static void
@@ -212,6 +212,22 @@ _track_obj_data_get_best_data(TrackObjData *obj_data)
         td_best = track_data;
     }
 
+    if (!td_best)
+        return NULL;
+
+    /* Always copy the object from the best TrackData to the TrackObjData. It is
+     * a bit odd that this getter modifies TrackObjData. However, it gives the
+     * nice property that after calling _track_obj_data_get_best_data() you can
+     * use obj_data->obj (and get the same as td_best->obj).
+     *
+     * This is actually important, because the previous obj_data->obj will have
+     * the same ID, but it might have minor differences to td_best->obj.
+     *
+     * Note that at this point obj_data->obj also might be an object that is no longer
+     * tracked. Updating the reference will ensure that we don't have such old references
+     * around and update to use the most appropriate one. */
+    nmp_object_ref_set(&obj_data->obj, td_best->obj);
+
     return td_best;
 }