From 3fcfb53c4b8d9c8c84008a01d0cc8e7cd7eff2e8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 14 Sep 2020 15:07:18 +0200 Subject: [PATCH 1/2] core: add nm_netns_shared_ip_reserve() API Add a better way of tracking the shared IP addresses that are in use. This will replace NMDevice's usage of a global hash table. For one, the API is more formalized with reserve() and release() functions. Also, it ties the used IP addresses to the netns, which would be more correct (in the future when we may support more netns). --- src/nm-netns.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++ src/nm-netns.h | 12 ++++++ 2 files changed, 120 insertions(+) diff --git a/src/nm-netns.c b/src/nm-netns.c index cf0e5f0b6b..a9596eaf00 100644 --- a/src/nm-netns.c +++ b/src/nm-netns.c @@ -28,6 +28,7 @@ typedef struct { NMPNetns *platform_netns; NMPRulesManager *rules_manager; GHashTable *l3cfgs; + GHashTable *shared_ips; CList l3cfg_signal_pending_lst_head; guint signal_pending_idle_id; } NMNetnsPrivate; @@ -218,6 +219,112 @@ _platform_signal_cb (NMPlatform *platform, /*****************************************************************************/ +NMNetnsSharedIPHandle * +nm_netns_shared_ip_reserve (NMNetns *self) +{ + NMNetnsPrivate *priv; + NMNetnsSharedIPHandle *handle; + const in_addr_t addr_start = ntohl (0x0a2a0001u); /* 10.42.0.1 */ + in_addr_t addr; + char sbuf_addr[NM_UTILS_INET_ADDRSTRLEN]; + + /* Find an unused address in the 10.42.x.x range */ + + g_return_val_if_fail (NM_IS_NETNS (self), NULL); + + priv = NM_NETNS_GET_PRIVATE (self); + + if (!priv->shared_ips) { + addr = addr_start; + priv->shared_ips = g_hash_table_new (nm_puint32_hash, nm_puint32_equals); + g_object_ref (self); + } else { + guint32 count; + + nm_assert (g_hash_table_size (priv->shared_ips) > 0); + + count = 0u; + for (;;) { + addr = addr_start + htonl (count << 8u); + + handle = g_hash_table_lookup (priv->shared_ips, &addr); + if (!handle) + break; + + count++; + + if (count > 0xFFu) { + if (handle->_ref_count == 1) { + _LOGE ("shared-ip4: ran out of shared IP addresses. Reuse %s/24", + _nm_utils_inet4_ntop (handle->addr, sbuf_addr)); + } else { + _LOGD ("shared-ip4: reserved IP address range %s/24 (duplicate)", + _nm_utils_inet4_ntop (handle->addr, sbuf_addr)); + } + handle->_ref_count++; + return handle; + } + } + } + + handle = g_slice_new (NMNetnsSharedIPHandle); + *handle = (NMNetnsSharedIPHandle) { + .addr = addr, + ._ref_count = 1, + ._self = self, + }; + + g_hash_table_add (priv->shared_ips, handle); + + _LOGD ("shared-ip4: reserved IP address range %s/24", + _nm_utils_inet4_ntop (handle->addr, sbuf_addr)); + return handle; +} + +void +nm_netns_shared_ip_release (NMNetnsSharedIPHandle *handle) +{ + NMNetns *self; + NMNetnsPrivate *priv; + char sbuf_addr[NM_UTILS_INET_ADDRSTRLEN]; + + g_return_if_fail (handle); + + self = handle->_self; + + g_return_if_fail (NM_IS_NETNS (self)); + + priv = NM_NETNS_GET_PRIVATE (self); + + nm_assert (handle->_ref_count > 0); + nm_assert (handle == nm_g_hash_table_lookup (priv->shared_ips, handle)); + + if (handle->_ref_count > 1) { + nm_assert (handle->addr == ntohl (0x0A2AFF01u)); /* 10.42.255.1 */ + handle->_ref_count--; + _LOGD ("shared-ip4: release IP address range %s/24 (%d more references held)", + _nm_utils_inet4_ntop (handle->addr, sbuf_addr), + handle->_ref_count); + return; + } + + if (!g_hash_table_remove (priv->shared_ips, handle)) + nm_assert_not_reached (); + + if (g_hash_table_size (priv->shared_ips) == 0) { + nm_clear_pointer (&priv->shared_ips, g_hash_table_unref); + g_object_unref (self); + } + + _LOGD ("shared-ip4: release IP address range %s/24", + _nm_utils_inet4_ntop (handle->addr, sbuf_addr)); + + handle->_self = NULL; + nm_g_slice_free (handle); +} + +/*****************************************************************************/ + static void set_property (GObject *object, guint prop_id, const GValue *value, GParamSpec *pspec) @@ -312,6 +419,7 @@ dispose (GObject *object) nm_assert (nm_g_hash_table_size (priv->l3cfgs) == 0); nm_assert (c_list_is_empty (&priv->l3cfg_signal_pending_lst_head)); + nm_assert (!priv->shared_ips); nm_clear_g_source (&priv->signal_pending_idle_id); diff --git a/src/nm-netns.h b/src/nm-netns.h index 2af0242b8d..080caa30e3 100644 --- a/src/nm-netns.h +++ b/src/nm-netns.h @@ -34,4 +34,16 @@ struct _NMDedupMultiIndex *nm_netns_get_multi_idx (NMNetns *self); NML3Cfg *nm_netns_access_l3cfg (NMNetns *netns, int ifindex); +/*****************************************************************************/ + +typedef struct { + in_addr_t addr; + int _ref_count; + NMNetns *_self; +} NMNetnsSharedIPHandle; + +NMNetnsSharedIPHandle *nm_netns_shared_ip_reserve (NMNetns *self); + +void nm_netns_shared_ip_release (NMNetnsSharedIPHandle *handle); + #endif /* __NM_NETNS_H__ */ From 3338ea0530bc05b403049df6df8f246cb455b597 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 14 Sep 2020 15:40:08 +0200 Subject: [PATCH 2/2] device: track used shared-ips via NMNetns Note that when NetworkManager tries to allocate more than 256 networks, then previously the allocation would fail. We no longer fail, but log an error and reuse the last address (10.42.255.1/24). It's simpler to have code that cannot fail, because it's often hard to handle failure properly. Also, if the user would configure two shared profiles that explicitly use the same subnet, we also wouldn't fail. Why not? Is that not a problem as well? If it is not, there is no need to fail in this case. If it is a problem, then it would be much more important to handle this case otherwise -- since it's more likely to activate two profiles that accidentally use the same subnet than activating 257+ shared profiles. --- src/devices/nm-device.c | 50 +++++++++++------------------------------ 1 file changed, 13 insertions(+), 37 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index c1a2bab12b..f0fe5a0385 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -327,6 +327,8 @@ typedef struct _NMDevicePrivate { const NML3ConfigData *l3cds[_L3_CONFIG_DATA_TYPE_NUM]; + NMNetnsSharedIPHandle *shared_ip_handle; + int parent_ifindex; int auth_retries; @@ -9649,21 +9651,11 @@ nm_device_dhcp4_renew (NMDevice *self, gboolean release) /*****************************************************************************/ -static GHashTable *shared_ips = NULL; - -static void -shared_ip_release (gpointer data) -{ - g_hash_table_remove (shared_ips, data); - if (!g_hash_table_size (shared_ips)) - nm_clear_pointer (&shared_ips, g_hash_table_unref); -} - static NMIP4Config * shared4_new_config (NMDevice *self, NMConnection *connection) { - NMIP4Config *config = NULL; - gboolean is_generated = FALSE; + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + NMIP4Config *config; NMSettingIPConfig *s_ip4; NMPlatformIP4Address address = { .addr_source = NM_IP_CONFIG_SOURCE_SHARED, @@ -9673,42 +9665,24 @@ shared4_new_config (NMDevice *self, NMConnection *connection) g_return_val_if_fail (connection, NULL); s_ip4 = nm_connection_get_setting_ip4_config (connection); - if (s_ip4 && nm_setting_ip_config_get_num_addresses (s_ip4)) { + if ( s_ip4 + && nm_setting_ip_config_get_num_addresses (s_ip4) > 0) { /* Use the first user-supplied address */ NMIPAddress *user = nm_setting_ip_config_get_address (s_ip4, 0); in_addr_t a; nm_ip_address_get_address_binary (user, &a); nm_platform_ip4_address_set_addr (&address, a, nm_ip_address_get_prefix (user)); + nm_clear_pointer (&priv->shared_ip_handle, nm_netns_shared_ip_release); } else { - /* Find an unused address in the 10.42.x.x range */ - guint32 start = (guint32) ntohl (0x0a2a0001); /* 10.42.0.1 */ - guint32 count = 0; - - if (G_UNLIKELY (!shared_ips)) - shared_ips = g_hash_table_new (nm_direct_hash, NULL); - else { - while (g_hash_table_lookup (shared_ips, GUINT_TO_POINTER (start + count))) { - count += ntohl (0x100); - if (count > ntohl (0xFE00)) { - _LOGE (LOGD_SHARING, "ran out of shared IP addresses!"); - return FALSE; - } - } - } - nm_platform_ip4_address_set_addr (&address, start + count, 24); - g_hash_table_add (shared_ips, GUINT_TO_POINTER (address.address)); - is_generated = TRUE; + if (!priv->shared_ip_handle) + priv->shared_ip_handle = nm_netns_shared_ip_reserve (nm_device_get_netns (self)); + nm_platform_ip4_address_set_addr (&address, priv->shared_ip_handle->addr, 24); } config = nm_device_ip4_config_new (self); nm_ip4_config_add_address (config, &address); - if (is_generated) { - /* Remove the address lock when the object gets disposed */ - g_object_set_qdata_full (G_OBJECT (config), NM_CACHED_QUARK ("shared-ip"), - GUINT_TO_POINTER (address.address), - shared_ip_release); - } + return config; } @@ -15936,6 +15910,8 @@ _cleanup_generic_pre (NMDevice *self, CleanupType cleanup_type) queued_state_clear (self); + nm_clear_pointer (&priv->shared_ip_handle, nm_netns_shared_ip_release); + _cleanup_ip_pre (self, AF_INET, cleanup_type); _cleanup_ip_pre (self, AF_INET6, cleanup_type); }