libnm: make ref counting of immutable types thread safe

The types NMBridgeVlan, NMIPRoutingRule, NMRange, NMWireGuardPeer
are immutable (or immutable, after the seal() function is called).

Immutable types are great, as it means a reference to them can be shared
without doing a full clone. Hence the G_DEFINE_BOXED_TYPE() for these
types prefers to take a reference instead of cloning the objects. Except
for sealable types, where it will prefer to clone unsealed values.
Likewise, nm_simple_connection_new_clone() probably will just take
another reference to the value, instead of doing a deep clone.

libnm is not a thread-safe library in the sense that you could pass a
NMConnection or NMClient instance to multiple threads and access them
without your own synchronization. However, it should be possible that
multiple threads access (seemingly) distinct objects.

As the copy function of these boxed types (and nm_simple_connection_new_clone()
and similar) prefers to share the references to immutable types, it is important
that the ref function is thread-safe too. Otherwise you cannot just clone a
NMConnection on thread1, hand the clone to thread2 and operate on the
clone and the original independently. If you do before this patch, you would
hit a subtle race condition.

Avoid that. While atomic operations have a runtime overhead, being safe
is more important. Also, we already save a full malloc()/free() by
having immutable, ref-counted types. We just need to make it safe to use
in order to fully benefit from it.

5 files changed, 32 insertions, 20 deletions

diff --git a/src/libnm-client-impl/nm-device.c b/src/libnm-client-impl/nm-device.c
index ec246afc1a..d5ecee48c1 100644
--- a/src/libnm-client-impl/nm-device.c
+++ b/src/libnm-client-impl/nm-device.c
@@ -2982,7 +2982,8 @@ NM_IS_LLDP_NEIGHBOR(const NMLldpNeighbor *self)
  * Note that #NMLldpNeighbor has no public API for mutating
  * an instance. Also, libnm will not internally mutate a
  * once exposed object. They are guaranteed to be immutable.
- * Since 1.32, ref-counting is thread-safe.
+ *
+ * Since 1.32, ref-counting of #NMLldpNeighbor is thread-safe.
  *
  * This function is not useful, as there is no public API to
  * actually modify the (empty) instance.
diff --git a/src/libnm-core-impl/nm-setting-bridge.c b/src/libnm-core-impl/nm-setting-bridge.c
index 1da740a1ce..c1a2621db1 100644
--- a/src/libnm-core-impl/nm-setting-bridge.c
+++ b/src/libnm-core-impl/nm-setting-bridge.c
@@ -109,7 +109,7 @@ G_DEFINE_TYPE(NMSettingBridge, nm_setting_bridge, NM_TYPE_SETTING)
 G_DEFINE_BOXED_TYPE(NMBridgeVlan, nm_bridge_vlan, _nm_bridge_vlan_dup, nm_bridge_vlan_unref)
 
 struct _NMBridgeVlan {
-    guint   refcount;
+    int     refcount;
     guint16 vid_start;
     guint16 vid_end;
     bool    untagged : 1;
@@ -132,6 +132,8 @@ NM_IS_BRIDGE_VLAN(const NMBridgeVlan *self, gboolean also_sealed)
  * Setting @vid_end to 0 is equivalent to setting it to @vid_start
  * and creates a single-id VLAN.
  *
+ * Since 1.42, ref-counting of #NMBridgeVlan is thread-safe.
+ *
  * Returns: (transfer full): the new #NMBridgeVlan object.
  *
  * Since: 1.18
@@ -165,6 +167,8 @@ nm_bridge_vlan_new(guint16 vid_start, guint16 vid_end)
  *
  * Returns: the input argument @vlan object.
  *
+ * Since 1.42, ref-counting of #NMBridgeVlan is thread-safe.
+ *
  * Since: 1.18
  **/
 NMBridgeVlan *
@@ -172,9 +176,9 @@ nm_bridge_vlan_ref(NMBridgeVlan *vlan)
 {
     g_return_val_if_fail(NM_IS_BRIDGE_VLAN(vlan, TRUE), NULL);
 
-    nm_assert(vlan->refcount < G_MAXUINT);
+    nm_assert(vlan->refcount < G_MAXINT);
 
-    vlan->refcount++;
+    g_atomic_int_inc(&vlan->refcount);
     return vlan;
 }
 
@@ -185,6 +189,8 @@ nm_bridge_vlan_ref(NMBridgeVlan *vlan)
  * Decreases the reference count of the object.  If the reference count
  * reaches zero the object will be destroyed.
  *
+ * Since 1.42, ref-counting of #NMBridgeVlan is thread-safe.
+ *
  * Since: 1.18
  **/
 void
@@ -192,7 +198,7 @@ nm_bridge_vlan_unref(NMBridgeVlan *vlan)
 {
     g_return_if_fail(NM_IS_BRIDGE_VLAN(vlan, TRUE));
 
-    if (--vlan->refcount == 0)
+    if (g_atomic_int_dec_and_test(&vlan->refcount))
         g_slice_free(NMBridgeVlan, vlan);
 }
 
diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c
index b4fdd040d4..496e7297e8 100644
--- a/src/libnm-core-impl/nm-setting-ip-config.c
+++ b/src/libnm-core-impl/nm-setting-ip-config.c
@@ -1537,7 +1537,7 @@ struct NMIPRoutingRule {
     char    *to_str;
     char    *iifname;
     char    *oifname;
-    guint    ref_count;
+    int      ref_count;
     guint32  priority;
     guint32  table;
     gint32   suppress_prefixlength;
@@ -1636,9 +1636,11 @@ nm_ip_routing_rule_new(int addr_family)
  * nm_ip_routing_rule_new_clone:
  * @rule: the #NMIPRoutingRule to clone.
  *
+ * Since 1.42, ref-counting of #NMIPRoutingRule is thread-safe.
+ *
  * Returns: (transfer full): a newly created rule instance with
  *   the same settings as @rule. Note that the instance will
- *   always be unsealred.
+ *   always be unsealed.
  *
  * Since: 1.18
  */
@@ -1704,11 +1706,12 @@ nm_ip_routing_rule_new_clone(const NMIPRoutingRule *rule)
  * @self: (allow-none): the #NMIPRoutingRule instance
  *
  * Increases the reference count of the instance.
- * This is not thread-safe.
  *
  * Returns: (transfer full): the @self argument with incremented
  *  reference count.
  *
+ * Since 1.42, ref-counting of #NMIPRoutingRule is thread-safe.
+ *
  * Since: 1.18
  */
 NMIPRoutingRule *
@@ -1719,8 +1722,9 @@ nm_ip_routing_rule_ref(NMIPRoutingRule *self)
 
     g_return_val_if_fail(NM_IS_IP_ROUTING_RULE(self, TRUE), NULL);
 
-    nm_assert(self->ref_count < G_MAXUINT);
-    self->ref_count++;
+    nm_assert(self->ref_count < G_MAXINT);
+
+    g_atomic_int_inc(&self->ref_count);
     return self;
 }
 
@@ -1730,7 +1734,8 @@ nm_ip_routing_rule_ref(NMIPRoutingRule *self)
  *
  * Decreases the reference count of the instance and destroys
  * the instance if the reference count reaches zero.
- * This is not thread-safe.
+ *
+ * Since 1.42, ref-counting of #NMIPRoutingRule is thread-safe.
  *
  * Since: 1.18
  */
@@ -1742,7 +1747,7 @@ nm_ip_routing_rule_unref(NMIPRoutingRule *self)
 
     g_return_if_fail(NM_IS_IP_ROUTING_RULE(self, TRUE));
 
-    if (--self->ref_count > 0)
+    if (!g_atomic_int_dec_and_test(&self->ref_count))
         return;
 
     g_free(self->from_str);
diff --git a/src/libnm-core-impl/nm-setting-private.h b/src/libnm-core-impl/nm-setting-private.h
index e7a26c5d3b..a0dcf777fd 100644
--- a/src/libnm-core-impl/nm-setting-private.h
+++ b/src/libnm-core-impl/nm-setting-private.h
@@ -303,7 +303,7 @@ typedef struct {
 /*****************************************************************************/
 
 struct _NMRange {
-    guint   refcount;
+    int     refcount;
     guint64 start;
     guint64 end;
 };
diff --git a/src/libnm-core-impl/nm-setting-wireguard.c b/src/libnm-core-impl/nm-setting-wireguard.c
index ec28f03a5f..a981187f10 100644
--- a/src/libnm-core-impl/nm-setting-wireguard.c
+++ b/src/libnm-core-impl/nm-setting-wireguard.c
@@ -50,7 +50,7 @@ struct _NMWireGuardPeer {
     char                *public_key;
     char                *preshared_key;
     GPtrArray           *allowed_ips;
-    guint                refcount;
+    int                  refcount;
     NMSettingSecretFlags preshared_key_flags;
     guint16              persistent_keepalive;
     bool                 public_key_valid : 1;
@@ -127,11 +127,11 @@ nm_wireguard_peer_new_clone(const NMWireGuardPeer *self, gboolean with_secrets)
  * nm_wireguard_peer_ref:
  * @self: (allow-none): the #NMWireGuardPeer instance
  *
- * This is not thread-safe.
- *
  * Returns: returns the input argument @self after incrementing
  *   the reference count.
  *
+ * Since 1.42, ref-counting of #NMWireGuardPeer is thread-safe.
+ *
  * Since: 1.16
  */
 NMWireGuardPeer *
@@ -142,9 +142,9 @@ nm_wireguard_peer_ref(NMWireGuardPeer *self)
 
     g_return_val_if_fail(NM_IS_WIREGUARD_PEER(self, TRUE), NULL);
 
-    nm_assert(self->refcount < G_MAXUINT);
+    nm_assert(self->refcount < G_MAXINT);
 
-    self->refcount++;
+    g_atomic_int_inc(&self->refcount);
     return self;
 }
 
@@ -155,7 +155,7 @@ nm_wireguard_peer_ref(NMWireGuardPeer *self)
  * Drop a reference to @self. If the last reference is dropped,
  * the instance is freed and all associate data released.
  *
- * This is not thread-safe.
+ * Since 1.42, ref-counting of #NMWireGuardPeer is thread-safe.
  *
  * Since: 1.16
  */
@@ -167,7 +167,7 @@ nm_wireguard_peer_unref(NMWireGuardPeer *self)
 
     g_return_if_fail(NM_IS_WIREGUARD_PEER(self, TRUE));
 
-    if (--self->refcount > 0)
+    if (!g_atomic_int_dec_and_test(&self->refcount))
         return;
 
     nm_sock_addr_endpoint_unref(self->endpoint);