From d8aacba3b2d68d25ad20bf34197e617723022975 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Sep 2022 09:23:31 +0200 Subject: [PATCH] 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 --- src/libnm-platform/nmp-global-tracker.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/libnm-platform/nmp-global-tracker.c b/src/libnm-platform/nmp-global-tracker.c index b1231ace41..09f1e21711 100644 --- a/src/libnm-platform/nmp-global-tracker.c +++ b/src/libnm-platform/nmp-global-tracker.c @@ -198,7 +198,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); } @@ -213,7 +213,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 @@ -253,6 +253,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; }