From 982807ec4e28d434779396520359636c57ea8e2b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jan 2022 15:49:47 +0100 Subject: [PATCH 1/4] Revert "nm-device: prefer manually configured addresses to automatic" This commit does not seem correct. The enum was moved with the declared intent to make manual IP configuration preferred. But the code comment in L3ConfigDataType states that higher numbers are more important. Also, all the other values are intentionally ordered so that more important method have higher numbers. For example, LL_4 < DHCP_4 < SHARED_4 < DEV_4 (in increasing priority). While it's often not clear whether to prefer one method over the other, or what the actual effect of (LL_4 < DHCP_4) is, the numbers were chosen intentionally. Only moving MANUALIP first, counters the relative order of the other values. If there is the problem, that the code comment is wrong (and lower numbers mean more important), then we would have to reverse all enum values. The real problem is that NML3Cfg's _l3_config_datas_get_sorted_cmp() has the wrong order/understanding. So the real fix is there and will be done next. That is, unless we would agree that _l3_config_datas_get_sorted_cmp() is in the right, and prefer lower numbers -- in which case all values had to be reversed. This reverts commit af1903fe3f41acb79acfba6463615dff87abde01. --- src/core/devices/nm-device.c | 4 ++-- src/core/nm-l3cfg.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index bc23ef0198..2be894ac03 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -139,8 +139,6 @@ typedef enum { * their relative order matters: higher numbers in this enum means more * important (and during merge overwrites other settings). */ - L3_CONFIG_DATA_TYPE_MANUALIP, - L3_CONFIG_DATA_TYPE_LL_4, L3_CONFIG_DATA_TYPE_LL_6, @@ -183,6 +181,8 @@ typedef enum { _t; \ }) + L3_CONFIG_DATA_TYPE_MANUALIP, + _L3_CONFIG_DATA_TYPE_NUM, _L3_CONFIG_DATA_TYPE_NONE, _L3_CONFIG_DATA_TYPE_ACD_ONLY, diff --git a/src/core/nm-l3cfg.h b/src/core/nm-l3cfg.h index d994efde32..a7d89a2618 100644 --- a/src/core/nm-l3cfg.h +++ b/src/core/nm-l3cfg.h @@ -6,8 +6,8 @@ #include "libnm-platform/nmp-object.h" #include "nm-l3-config-data.h" -#define NM_L3CFG_CONFIG_PRIORITY_IPV4LL 1 -#define NM_L3CFG_CONFIG_PRIORITY_IPV6LL 1 +#define NM_L3CFG_CONFIG_PRIORITY_IPV4LL 0 +#define NM_L3CFG_CONFIG_PRIORITY_IPV6LL 0 #define NM_ACD_TIMEOUT_RFC5227_MSEC 9000u #define NM_TYPE_L3CFG (nm_l3cfg_get_type()) From 24896e636b78b354264383a10883911ad88823d8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 28 Jan 2022 13:32:32 +0100 Subject: [PATCH 2/4] l3cfg: fix order of NML3ConfigData priorities in NML3Cfg There was a disagreement whether higher priority numbers mean more/less important. NMDevice and callers of nm_l3cfg_add_config() assumed that higher numbers are more important, while _l3_config_datas_get_sorted_cmp() did the inverse. There is no obvious right or wrong. We only need to agree. It seems slightly more sensible to keep the caller's interpretation. --- src/core/devices/nm-device.c | 3 ++- src/core/nm-l3cfg.c | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 2be894ac03..fb0f8df175 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -137,7 +137,8 @@ typedef struct { typedef enum { /* The various NML3ConfigData types that we track explicitly. Note that * their relative order matters: higher numbers in this enum means more - * important (and during merge overwrites other settings). */ + * important (and during merge overwrites other settings). This is passed + * as priority to nm_l3cfg_add_config(). */ L3_CONFIG_DATA_TYPE_LL_4, L3_CONFIG_DATA_TYPE_LL_6, diff --git a/src/core/nm-l3cfg.c b/src/core/nm-l3cfg.c index f36d798d2c..b42ff8cb75 100644 --- a/src/core/nm-l3cfg.c +++ b/src/core/nm-l3cfg.c @@ -3055,9 +3055,9 @@ _l3_config_datas_get_sorted_cmp(gconstpointer p_a, gconstpointer p_b, gpointer u nm_assert(b); nm_assert(nm_l3_config_data_get_ifindex(a->l3cd) == nm_l3_config_data_get_ifindex(b->l3cd)); - /* we sort the entries with higher priority (more important, lower numerical value) + /* we sort the entries with higher priority (higher numerical value, more important) * first. */ - NM_CMP_FIELD(a, b, priority_confdata); + NM_CMP_FIELD(b, a, priority_confdata); /* if the priority is not unique, we sort them in the order they were added, * with the oldest first (lower numerical value). */ @@ -3105,6 +3105,14 @@ nm_l3cfg_mark_config_dirty(NML3Cfg *self, gconstpointer tag, gboolean dirty) } } +/* + * nm_l3cfg_add_config: + * @priority: all l3cd get merged/combined. This merging requires that some + * l3cd are more important than others. For example, coming from static IP + * configuration needs to take precedence over DHCP. The @priority determines + * the order in which l3cds get merged (and thus the outcome). Higher numbers + * mean more important!! + */ gboolean nm_l3cfg_add_config(NML3Cfg *self, gconstpointer tag, @@ -3470,6 +3478,20 @@ _l3cfg_update_combined_config(NML3Cfg *self, l3_config_datas_arr[i] = _l3_config_datas_at(self->priv.p->l3_config_datas, i); if (l3_config_datas_len > 1) { + /* We are about to merge the l3cds. The order in which we do that matters. + * + * Below, we iterate over the l3cds and merge them into a new one. nm_l3_config_data_merge() + * uses "NM_L3_CONFIG_ADD_FLAGS_EXCLUSIVE" flag, which means to keep the first entry. + * + * Consider for example addresses/routes, which have a set of ID attributes (based on + * which no duplicates can be accepted) and additional attributes. For example, trying + * to add the same address twice ("same" according to their ID), only one can be added. + * If they differ in their lifetimes, we need to make a choice. + * We could merge the attributes in a sensible way. Instead, NM_L3_CONFIG_ADD_FLAGS_EXCLUSIVE + * takes care to only take the first one. + * + * So we want to sort the more important entries *first*, and this is based on + * the priority_confdata. */ g_qsort_with_data(l3_config_datas_arr, l3_config_datas_len, sizeof(l3_config_datas_arr[0]), @@ -3499,6 +3521,14 @@ _l3cfg_update_combined_config(NML3Cfg *self, for (i = 0; i < l3_config_datas_len; i++) { const L3ConfigData *l3cd_data = l3_config_datas_arr[i]; + /* more important entries must be sorted *first*. */ + nm_assert( + i == 0 + || (l3_config_datas_arr[i - 1]->priority_confdata > l3cd_data->priority_confdata) + || (l3_config_datas_arr[i - 1]->priority_confdata == l3cd_data->priority_confdata + && l3_config_datas_arr[i - 1]->pseudo_timestamp_confdata + < l3cd_data->pseudo_timestamp_confdata)); + if (NM_FLAGS_HAS(l3cd_data->config_flags, NM_L3CFG_CONFIG_FLAGS_ONLY_FOR_ACD)) continue; From 5774af9cbd832a60f454a836fc2cd7e02355b1de Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 28 Jan 2022 13:46:40 +0100 Subject: [PATCH 3/4] device: adjust NM_L3CFG_CONFIG_PRIORITY_IPV6LL and L3_CONFIG_DATA_TYPE_LL_6 NM_L3CFG_CONFIG_PRIORITY_IPV6LL and L3_CONFIG_DATA_TYPE_LL_6 are somewhat related. They probably should be numerically identical. Now, L3_CONFIG_DATA_TYPE_LL_6 cannot be zero (because L3_CONFIG_DATA_TYPE_LL_4 is zero and L3ConfigDataType values must be distinct). Therefore, also bump NM_L3CFG_CONFIG_PRIORITY_IPV6LL to 1. Of course, NM_L3CFG_CONFIG_PRIORITY_IPV6LL is not obviously more important than NM_L3CFG_CONFIG_PRIORITY_IPV4LL. But to be consistent with L3_CONFIG_DATA_TYPE_LL_6 is probably more important here. --- src/core/devices/nm-device.c | 1 + src/core/nm-l3cfg.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index fb0f8df175..e361ff5d3a 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -190,6 +190,7 @@ typedef enum { } L3ConfigDataType; G_STATIC_ASSERT(NM_L3CFG_CONFIG_PRIORITY_IPV4LL == L3_CONFIG_DATA_TYPE_LL_4); +G_STATIC_ASSERT(NM_L3CFG_CONFIG_PRIORITY_IPV6LL == L3_CONFIG_DATA_TYPE_LL_6); typedef enum { HW_ADDR_TYPE_UNSET = 0, diff --git a/src/core/nm-l3cfg.h b/src/core/nm-l3cfg.h index a7d89a2618..cbc974efaa 100644 --- a/src/core/nm-l3cfg.h +++ b/src/core/nm-l3cfg.h @@ -7,7 +7,7 @@ #include "nm-l3-config-data.h" #define NM_L3CFG_CONFIG_PRIORITY_IPV4LL 0 -#define NM_L3CFG_CONFIG_PRIORITY_IPV6LL 0 +#define NM_L3CFG_CONFIG_PRIORITY_IPV6LL 1 #define NM_ACD_TIMEOUT_RFC5227_MSEC 9000u #define NM_TYPE_L3CFG (nm_l3cfg_get_type()) From 5cbf666279e244270e31bb641a49dcf12d3115fd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 28 Jan 2022 13:53:16 +0100 Subject: [PATCH 4/4] core: increase l3cfg merge priority for VPN config Now that higher priorities numbers really mean more important, it seems that the VPN configuration should be rather important. Bump the number, also in relation to NMDevice's L3ConfigDataType. It might not matter too much, because usually the VPN tunnel device does not have NDevice to add other l3cds and those from VPN might be alone. Except, maybe with routing VPN (libreswan) that is different. Dunno. --- src/core/devices/nm-device.c | 1 + src/core/nm-l3cfg.h | 1 + src/core/vpn/nm-vpn-connection.c | 5 +---- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index e361ff5d3a..a39aec16fb 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -191,6 +191,7 @@ typedef enum { G_STATIC_ASSERT(NM_L3CFG_CONFIG_PRIORITY_IPV4LL == L3_CONFIG_DATA_TYPE_LL_4); G_STATIC_ASSERT(NM_L3CFG_CONFIG_PRIORITY_IPV6LL == L3_CONFIG_DATA_TYPE_LL_6); +G_STATIC_ASSERT(NM_L3CFG_CONFIG_PRIORITY_VPN == L3_CONFIG_DATA_TYPE_DEVIP_6); typedef enum { HW_ADDR_TYPE_UNSET = 0, diff --git a/src/core/nm-l3cfg.h b/src/core/nm-l3cfg.h index cbc974efaa..6fd8f9de80 100644 --- a/src/core/nm-l3cfg.h +++ b/src/core/nm-l3cfg.h @@ -8,6 +8,7 @@ #define NM_L3CFG_CONFIG_PRIORITY_IPV4LL 0 #define NM_L3CFG_CONFIG_PRIORITY_IPV6LL 1 +#define NM_L3CFG_CONFIG_PRIORITY_VPN 9 #define NM_ACD_TIMEOUT_RFC5227_MSEC 9000u #define NM_TYPE_L3CFG (nm_l3cfg_get_type()) diff --git a/src/core/vpn/nm-vpn-connection.c b/src/core/vpn/nm-vpn-connection.c index 14ef1c4cac..bbb7355016 100644 --- a/src/core/vpn/nm-vpn-connection.c +++ b/src/core/vpn/nm-vpn-connection.c @@ -716,7 +716,6 @@ _l3cfg_l3cd_update(NMVpnConnection *self, L3CDType l3cd_type) { NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE(self); NML3Cfg *l3cfg; - int priority; const NML3ConfigData *const *p_l3cd; if (NM_IN_SET(l3cd_type, L3CD_TYPE_IP_4, L3CD_TYPE_IP_6, L3CD_TYPE_GENERIC, L3CD_TYPE_STATIC)) { @@ -743,13 +742,11 @@ _l3cfg_l3cd_update(NMVpnConnection *self, L3CDType l3cd_type) goto handle_changed; } - priority = 0; - if (!nm_l3cfg_add_config(l3cfg, p_l3cd, TRUE, *p_l3cd, - priority, + NM_L3CFG_CONFIG_PRIORITY_VPN, get_route_table(self, AF_INET, TRUE), get_route_table(self, AF_INET6, TRUE), nm_vpn_connection_get_ip_route_metric(self, AF_INET),