From 70b23c7979566f4be46d138dedfa59e215cec44b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 Jul 2019 18:24:24 +0200 Subject: [PATCH 1/3] libnm: accept special table names for policy-routing The tables "main", "local", and "default" have well known names. Accept them as aliases when parsing the string representation of the rule. Note that iproute2 also considers /etc/iproute2/rt_tables for table names. In particular, that allows a user to re-map the well-known names like "main" to a different table. We never honor that file, and "main" always means table 254. Note that this only affects how we parse the string representation for rules. As the representation is neither unique nor enforced to be normalized, being more graceful here is no problem. The point is of course that the user possibly has existing iproute2 scripts that use such keyword. This makes it simpler to copy & paste the rule. --- libnm-core/nm-setting-ip-config.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c index 51dbd12b98..8b044965ec 100644 --- a/libnm-core/nm-setting-ip-config.c +++ b/libnm-core/nm-setting-ip-config.c @@ -2992,7 +2992,8 @@ nm_ip_routing_rule_from_string (const char *str, * Of course, valid rules can be converted to string and read back the same (round-trip). * * - iproute2 in may regards is flexible about the command lines. For example - * - for tables it accepts table names from /etc/iproute2/rt_tables + * - for tables it accepts table names from /etc/iproute2/rt_tables. We only + * accept the special aliases "main", "local", and "default". * - key names like "preference" can be abbreviated to "prefe", we don't do that. * - the "preference"/"priority" may be unspecified, in which kernel automatically * chooses an unused priority (during `ip rule add`). We don't allow for that, the @@ -3072,8 +3073,16 @@ nm_ip_routing_rule_from_string (const char *str, if (i64_table != -1) goto next_fail_word0_duplicate_key; i64_table = _nm_utils_ascii_str_to_int64 (word1, 0, 1, G_MAXUINT32, -1); - if (i64_table == -1) - goto next_fail_word1_invalid_value; + if (i64_table == -1) { + if (nm_streq (word1, "main")) + i64_table = RT_TABLE_MAIN; + else if (nm_streq (word1, "local")) + i64_table = RT_TABLE_LOCAL; + else if (nm_streq (word1, "default")) + i64_table = RT_TABLE_DEFAULT; + else + goto next_fail_word1_invalid_value; + } goto next_words_consumed; } if (NM_IN_STRSET (word0, "tos", From 6ea56bc04c772b49b84b74396b5e71ba5ff1a089 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 Jul 2019 18:54:48 +0200 Subject: [PATCH 2/3] libnm,core: add support for "suppress_prefixlength" rule attribute WireGuard's wq-quick configures such rules to avoid routing loops. While we currently don't have an automatic solution for this, at least we should support it via explicit user configuration. One problem is that suppress_prefixlength is relatively new and kernel might not support this attribute. That can lead to odd results, because the NetworkManager is valid but it cannot be configured on the current kernel. But this is a general problem, and we would require a general solution. The solution cannot be to only support rule attributes that are supported by the oldest possible kernel. It's not clear how much of a problem there really is, or which general solution is required (if any). --- libnm-core/nm-core-internal.h | 39 +++--- libnm-core/nm-setting-ip-config.c | 195 +++++++++++++++++++++--------- libnm-core/nm-setting-ip-config.h | 5 + libnm/libnm.ver | 2 + src/NetworkManagerUtils.c | 43 +++---- 5 files changed, 187 insertions(+), 97 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index ad17a1d01d..912ce1b46e 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -661,25 +661,26 @@ gboolean nm_ip_routing_rule_get_xifname_bin (const NMIPRoutingRule *self, gboolean iif /* or else oif */, char out_xifname[static 16]); -#define NM_IP_ROUTING_RULE_ATTR_ACTION "action" -#define NM_IP_ROUTING_RULE_ATTR_DPORT_END "dport-end" -#define NM_IP_ROUTING_RULE_ATTR_DPORT_START "dport-start" -#define NM_IP_ROUTING_RULE_ATTR_FAMILY "family" -#define NM_IP_ROUTING_RULE_ATTR_FROM "from" -#define NM_IP_ROUTING_RULE_ATTR_FROM_LEN "from-len" -#define NM_IP_ROUTING_RULE_ATTR_FWMARK "fwmark" -#define NM_IP_ROUTING_RULE_ATTR_FWMASK "fwmask" -#define NM_IP_ROUTING_RULE_ATTR_IIFNAME "iifname" -#define NM_IP_ROUTING_RULE_ATTR_INVERT "invert" -#define NM_IP_ROUTING_RULE_ATTR_IPPROTO "ipproto" -#define NM_IP_ROUTING_RULE_ATTR_OIFNAME "oifname" -#define NM_IP_ROUTING_RULE_ATTR_PRIORITY "priority" -#define NM_IP_ROUTING_RULE_ATTR_SPORT_END "sport-end" -#define NM_IP_ROUTING_RULE_ATTR_SPORT_START "sport-start" -#define NM_IP_ROUTING_RULE_ATTR_TABLE "table" -#define NM_IP_ROUTING_RULE_ATTR_TO "to" -#define NM_IP_ROUTING_RULE_ATTR_TOS "tos" -#define NM_IP_ROUTING_RULE_ATTR_TO_LEN "to-len" +#define NM_IP_ROUTING_RULE_ATTR_ACTION "action" +#define NM_IP_ROUTING_RULE_ATTR_DPORT_END "dport-end" +#define NM_IP_ROUTING_RULE_ATTR_DPORT_START "dport-start" +#define NM_IP_ROUTING_RULE_ATTR_FAMILY "family" +#define NM_IP_ROUTING_RULE_ATTR_FROM "from" +#define NM_IP_ROUTING_RULE_ATTR_FROM_LEN "from-len" +#define NM_IP_ROUTING_RULE_ATTR_FWMARK "fwmark" +#define NM_IP_ROUTING_RULE_ATTR_FWMASK "fwmask" +#define NM_IP_ROUTING_RULE_ATTR_IIFNAME "iifname" +#define NM_IP_ROUTING_RULE_ATTR_INVERT "invert" +#define NM_IP_ROUTING_RULE_ATTR_IPPROTO "ipproto" +#define NM_IP_ROUTING_RULE_ATTR_OIFNAME "oifname" +#define NM_IP_ROUTING_RULE_ATTR_PRIORITY "priority" +#define NM_IP_ROUTING_RULE_ATTR_SPORT_END "sport-end" +#define NM_IP_ROUTING_RULE_ATTR_SPORT_START "sport-start" +#define NM_IP_ROUTING_RULE_ATTR_SUPPRESS_PREFIXLENGTH "suppress-prefixlength" +#define NM_IP_ROUTING_RULE_ATTR_TABLE "table" +#define NM_IP_ROUTING_RULE_ATTR_TO "to" +#define NM_IP_ROUTING_RULE_ATTR_TOS "tos" +#define NM_IP_ROUTING_RULE_ATTR_TO_LEN "to-len" NMIPRoutingRule *nm_ip_routing_rule_from_dbus (GVariant *variant, gboolean strict, diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c index 8b044965ec..6c1c96be1b 100644 --- a/libnm-core/nm-setting-ip-config.c +++ b/libnm-core/nm-setting-ip-config.c @@ -1392,6 +1392,7 @@ struct NMIPRoutingRule { guint ref_count; guint32 priority; guint32 table; + gint32 suppress_prefixlength; guint32 fwmark; guint32 fwmask; guint16 sport_start; @@ -1472,10 +1473,11 @@ nm_ip_routing_rule_new (int addr_family) self = g_slice_new (NMIPRoutingRule); *self = (NMIPRoutingRule) { - .ref_count = 1, - .is_v4 = (addr_family == AF_INET), - .action = FR_ACT_TO_TBL, - .table = RT_TABLE_MAIN, + .ref_count = 1, + .is_v4 = (addr_family == AF_INET), + .action = FR_ACT_TO_TBL, + .table = RT_TABLE_MAIN, + .suppress_prefixlength = -1, }; return self; } @@ -1499,50 +1501,52 @@ nm_ip_routing_rule_new_clone (const NMIPRoutingRule *rule) self = g_slice_new (NMIPRoutingRule); *self = (NMIPRoutingRule) { - .ref_count = 1, - .sealed = FALSE, - .is_v4 = rule->is_v4, + .ref_count = 1, + .sealed = FALSE, + .is_v4 = rule->is_v4, - .priority = rule->priority, - .priority_has = rule->priority_has, + .priority = rule->priority, + .priority_has = rule->priority_has, - .invert = rule->invert, + .invert = rule->invert, - .tos = rule->tos, + .tos = rule->tos, - .fwmark = rule->fwmark, - .fwmask = rule->fwmask, + .fwmark = rule->fwmark, + .fwmask = rule->fwmask, - .sport_start = rule->sport_start, - .sport_end = rule->sport_end, - .dport_start = rule->dport_start, - .dport_end = rule->dport_end, + .sport_start = rule->sport_start, + .sport_end = rule->sport_end, + .dport_start = rule->dport_start, + .dport_end = rule->dport_end, - .ipproto = rule->ipproto, + .ipproto = rule->ipproto, - .from_len = rule->from_len, - .from_bin = rule->from_bin, - .from_str = ( rule->from_has - && !rule->from_valid) - ? g_strdup (rule->from_str) - : NULL, - .from_has = rule->from_has, - .from_valid = rule->from_valid, + .from_len = rule->from_len, + .from_bin = rule->from_bin, + .from_str = ( rule->from_has + && !rule->from_valid) + ? g_strdup (rule->from_str) + : NULL, + .from_has = rule->from_has, + .from_valid = rule->from_valid, - .to_len = rule->to_len, - .to_bin = rule->to_bin, - .to_str = ( rule->to_has - && !rule->to_valid) - ? g_strdup (rule->to_str) - : NULL, - .to_has = rule->to_has, - .to_valid = rule->to_valid, + .to_len = rule->to_len, + .to_bin = rule->to_bin, + .to_str = ( rule->to_has + && !rule->to_valid) + ? g_strdup (rule->to_str) + : NULL, + .to_has = rule->to_has, + .to_valid = rule->to_valid, - .iifname = g_strdup (rule->iifname), - .oifname = g_strdup (rule->oifname), + .iifname = g_strdup (rule->iifname), + .oifname = g_strdup (rule->oifname), - .action = rule->action, - .table = rule->table, + .action = rule->action, + .table = rule->table, + + .suppress_prefixlength = rule->suppress_prefixlength, }; return self; } @@ -2325,6 +2329,38 @@ nm_ip_routing_rule_set_table (NMIPRoutingRule *self, guint32 table) self->table = table; } +/** + * nm_ip_routing_rule_get_suppress_prefixlength: + * @self: the #NMIPRoutingRule instance + * + * Returns: the suppress_prefixlength of the rule. -1 means that the value is unset. + * + * Since: 1.20 + */ +gint32 +nm_ip_routing_rule_get_suppress_prefixlength (const NMIPRoutingRule *self) +{ + g_return_val_if_fail (NM_IS_IP_ROUTING_RULE (self, TRUE), -1); + + return self->suppress_prefixlength; +} + +/** + * nm_ip_routing_rule_set_suppress_prefixlength: + * @self: the #NMIPRoutingRule instance + * @suppress_prefixlength: the suppress_prefixlength to set. The value -1 means + * unset. + * + * Since: 1.20 + */ +void +nm_ip_routing_rule_set_suppress_prefixlength (NMIPRoutingRule *self, gint32 suppress_prefixlength) +{ + g_return_if_fail (NM_IS_IP_ROUTING_RULE (self, FALSE)); + + self->suppress_prefixlength = suppress_prefixlength; +} + /** * nm_ip_routing_rule_cmp: * @rule: (allow-none): the #NMIPRoutingRule instance to compare @@ -2361,6 +2397,8 @@ nm_ip_routing_rule_cmp (const NMIPRoutingRule *rule, NM_CMP_FIELD (rule, other, table); + NM_CMP_FIELD (rule, other, suppress_prefixlength); + NM_CMP_FIELD (rule, other, sport_start); NM_CMP_FIELD (rule, other, sport_end); NM_CMP_FIELD (rule, other, dport_start); @@ -2571,6 +2609,20 @@ nm_ip_routing_rule_validate (const NMIPRoutingRule *self, return FALSE; } + if (self->suppress_prefixlength != -1) { + if ( self->suppress_prefixlength < -1 + || self->suppress_prefixlength > (self->is_v4 ? 32 : 128)) { + g_set_error_literal (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("suppress_prefixlength out of range")); + return FALSE; + } + if (self->action != FR_ACT_TO_TBL) { + g_set_error_literal (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("suppress_prefixlength is only allowed with the to-table action")); + return FALSE; + } + } + return TRUE; } @@ -2593,6 +2645,7 @@ typedef enum { RR_DBUS_ATTR_SPORT_END, RR_DBUS_ATTR_SPORT_START, RR_DBUS_ATTR_TABLE, + RR_DBUS_ATTR_SUPPRESS_PREFIXLENGTH, RR_DBUS_ATTR_TO, RR_DBUS_ATTR_TOS, RR_DBUS_ATTR_TO_LEN, @@ -2607,25 +2660,26 @@ typedef struct { static const RRDbusData rr_dbus_data[_RR_DBUS_ATTR_NUM] = { #define _D(attr, _name, type) [attr] = { .name = _name, .dbus_type = type, } - _D (RR_DBUS_ATTR_ACTION, NM_IP_ROUTING_RULE_ATTR_ACTION, G_VARIANT_TYPE_BYTE), - _D (RR_DBUS_ATTR_DPORT_END, NM_IP_ROUTING_RULE_ATTR_DPORT_END, G_VARIANT_TYPE_UINT16), - _D (RR_DBUS_ATTR_DPORT_START, NM_IP_ROUTING_RULE_ATTR_DPORT_START, G_VARIANT_TYPE_UINT16), - _D (RR_DBUS_ATTR_FAMILY, NM_IP_ROUTING_RULE_ATTR_FAMILY, G_VARIANT_TYPE_INT32), - _D (RR_DBUS_ATTR_FROM, NM_IP_ROUTING_RULE_ATTR_FROM, G_VARIANT_TYPE_STRING), - _D (RR_DBUS_ATTR_FROM_LEN, NM_IP_ROUTING_RULE_ATTR_FROM_LEN, G_VARIANT_TYPE_BYTE), - _D (RR_DBUS_ATTR_FWMARK, NM_IP_ROUTING_RULE_ATTR_FWMARK, G_VARIANT_TYPE_UINT32), - _D (RR_DBUS_ATTR_FWMASK, NM_IP_ROUTING_RULE_ATTR_FWMASK, G_VARIANT_TYPE_UINT32), - _D (RR_DBUS_ATTR_IIFNAME, NM_IP_ROUTING_RULE_ATTR_IIFNAME, G_VARIANT_TYPE_STRING), - _D (RR_DBUS_ATTR_INVERT, NM_IP_ROUTING_RULE_ATTR_INVERT, G_VARIANT_TYPE_BOOLEAN), - _D (RR_DBUS_ATTR_IPPROTO, NM_IP_ROUTING_RULE_ATTR_IPPROTO, G_VARIANT_TYPE_BYTE), - _D (RR_DBUS_ATTR_OIFNAME, NM_IP_ROUTING_RULE_ATTR_OIFNAME, G_VARIANT_TYPE_STRING), - _D (RR_DBUS_ATTR_PRIORITY, NM_IP_ROUTING_RULE_ATTR_PRIORITY, G_VARIANT_TYPE_UINT32), - _D (RR_DBUS_ATTR_SPORT_END, NM_IP_ROUTING_RULE_ATTR_SPORT_END, G_VARIANT_TYPE_UINT16), - _D (RR_DBUS_ATTR_SPORT_START, NM_IP_ROUTING_RULE_ATTR_SPORT_START, G_VARIANT_TYPE_UINT16), - _D (RR_DBUS_ATTR_TABLE, NM_IP_ROUTING_RULE_ATTR_TABLE, G_VARIANT_TYPE_UINT32), - _D (RR_DBUS_ATTR_TO, NM_IP_ROUTING_RULE_ATTR_TO, G_VARIANT_TYPE_STRING), - _D (RR_DBUS_ATTR_TOS, NM_IP_ROUTING_RULE_ATTR_TOS, G_VARIANT_TYPE_BYTE), - _D (RR_DBUS_ATTR_TO_LEN, NM_IP_ROUTING_RULE_ATTR_TO_LEN, G_VARIANT_TYPE_BYTE), + _D (RR_DBUS_ATTR_ACTION, NM_IP_ROUTING_RULE_ATTR_ACTION, G_VARIANT_TYPE_BYTE), + _D (RR_DBUS_ATTR_DPORT_END, NM_IP_ROUTING_RULE_ATTR_DPORT_END, G_VARIANT_TYPE_UINT16), + _D (RR_DBUS_ATTR_DPORT_START, NM_IP_ROUTING_RULE_ATTR_DPORT_START, G_VARIANT_TYPE_UINT16), + _D (RR_DBUS_ATTR_FAMILY, NM_IP_ROUTING_RULE_ATTR_FAMILY, G_VARIANT_TYPE_INT32), + _D (RR_DBUS_ATTR_FROM, NM_IP_ROUTING_RULE_ATTR_FROM, G_VARIANT_TYPE_STRING), + _D (RR_DBUS_ATTR_FROM_LEN, NM_IP_ROUTING_RULE_ATTR_FROM_LEN, G_VARIANT_TYPE_BYTE), + _D (RR_DBUS_ATTR_FWMARK, NM_IP_ROUTING_RULE_ATTR_FWMARK, G_VARIANT_TYPE_UINT32), + _D (RR_DBUS_ATTR_FWMASK, NM_IP_ROUTING_RULE_ATTR_FWMASK, G_VARIANT_TYPE_UINT32), + _D (RR_DBUS_ATTR_IIFNAME, NM_IP_ROUTING_RULE_ATTR_IIFNAME, G_VARIANT_TYPE_STRING), + _D (RR_DBUS_ATTR_INVERT, NM_IP_ROUTING_RULE_ATTR_INVERT, G_VARIANT_TYPE_BOOLEAN), + _D (RR_DBUS_ATTR_IPPROTO, NM_IP_ROUTING_RULE_ATTR_IPPROTO, G_VARIANT_TYPE_BYTE), + _D (RR_DBUS_ATTR_OIFNAME, NM_IP_ROUTING_RULE_ATTR_OIFNAME, G_VARIANT_TYPE_STRING), + _D (RR_DBUS_ATTR_PRIORITY, NM_IP_ROUTING_RULE_ATTR_PRIORITY, G_VARIANT_TYPE_UINT32), + _D (RR_DBUS_ATTR_SPORT_END, NM_IP_ROUTING_RULE_ATTR_SPORT_END, G_VARIANT_TYPE_UINT16), + _D (RR_DBUS_ATTR_SPORT_START, NM_IP_ROUTING_RULE_ATTR_SPORT_START, G_VARIANT_TYPE_UINT16), + _D (RR_DBUS_ATTR_SUPPRESS_PREFIXLENGTH, NM_IP_ROUTING_RULE_ATTR_SUPPRESS_PREFIXLENGTH, G_VARIANT_TYPE_INT32), + _D (RR_DBUS_ATTR_TABLE, NM_IP_ROUTING_RULE_ATTR_TABLE, G_VARIANT_TYPE_UINT32), + _D (RR_DBUS_ATTR_TO, NM_IP_ROUTING_RULE_ATTR_TO, G_VARIANT_TYPE_STRING), + _D (RR_DBUS_ATTR_TOS, NM_IP_ROUTING_RULE_ATTR_TOS, G_VARIANT_TYPE_BYTE), + _D (RR_DBUS_ATTR_TO_LEN, NM_IP_ROUTING_RULE_ATTR_TO_LEN, G_VARIANT_TYPE_BYTE), #undef _D }; @@ -2788,6 +2842,9 @@ nm_ip_routing_rule_from_dbus (GVariant *variant, if (variants[RR_DBUS_ATTR_TABLE]) nm_ip_routing_rule_set_table (self, g_variant_get_uint32 (variants[RR_DBUS_ATTR_TABLE])); + if (variants[RR_DBUS_ATTR_SUPPRESS_PREFIXLENGTH]) + nm_ip_routing_rule_set_suppress_prefixlength (self, g_variant_get_int32 (variants[RR_DBUS_ATTR_SUPPRESS_PREFIXLENGTH])); + if ( strict && !nm_ip_routing_rule_validate (self, error)) return NULL; @@ -2889,6 +2946,9 @@ nm_ip_routing_rule_to_dbus (const NMIPRoutingRule *self) if (self->table != 0) _rr_to_dbus_add (&builder, RR_DBUS_ATTR_TABLE, g_variant_new_uint32 (self->table)); + if (self->suppress_prefixlength != -1) + _rr_to_dbus_add (&builder, RR_DBUS_ATTR_SUPPRESS_PREFIXLENGTH, g_variant_new_int32 (self->suppress_prefixlength)); + return g_variant_builder_end (&builder);; } @@ -2965,6 +3025,7 @@ nm_ip_routing_rule_from_string (const char *str, gint64 i64_fwmask = -1; gint64 i64_sport_start = -1; gint64 i64_ipproto = -1; + gint64 i64_suppress_prefixlength = -1; guint16 sport_end = 0; gint64 i64_dport_start = -1; guint16 dport_end = 0; @@ -3160,6 +3221,17 @@ nm_ip_routing_rule_from_string (const char *str, word_oifname = word1; goto next_words_consumed; } + if (NM_IN_STRSET (word0, "suppress_prefixlength", + "sup_pl")) { + if (!word1) + continue; + if (i64_suppress_prefixlength != -1) + goto next_fail_word0_duplicate_key; + i64_suppress_prefixlength = _nm_utils_ascii_str_to_int64 (word1, 0, 0, G_MAXINT32, -1);; + if (i64_suppress_prefixlength == -1) + goto next_fail_word1_invalid_value; + goto next_words_consumed; + } /* also the action is still unsupported. For the moment, we only support * FR_ACT_TO_TBL, which is the default (by not expressing it on the command @@ -3253,6 +3325,9 @@ next_words_consumed: if (i64_dport_start != -1) nm_ip_routing_rule_set_destination_port (self, i64_dport_start, dport_end); + if (i64_suppress_prefixlength != -1) + nm_ip_routing_rule_set_suppress_prefixlength (self, i64_suppress_prefixlength); + if ( val_from_len > 0 || ( val_from_len == 0 && !nm_ip_addr_is_null (addr_family, &val_from))) { @@ -3489,6 +3564,12 @@ nm_ip_routing_rule_to_string (const NMIPRoutingRule *self, (guint) self->table); } + if (self->suppress_prefixlength != -1) { + g_string_append_printf (nm_gstring_add_space_delimiter (str), + "suppress_prefixlength %d", + (int) self->suppress_prefixlength); + } + return g_string_free (g_steal_pointer (&str), FALSE); } diff --git a/libnm-core/nm-setting-ip-config.h b/libnm-core/nm-setting-ip-config.h index 912beda160..4a5223673b 100644 --- a/libnm-core/nm-setting-ip-config.h +++ b/libnm-core/nm-setting-ip-config.h @@ -262,6 +262,11 @@ guint32 nm_ip_routing_rule_get_table (const NMIPRoutingRule *self); NM_AVAILABLE_IN_1_18 void nm_ip_routing_rule_set_table (NMIPRoutingRule *self, guint32 table); +NM_AVAILABLE_IN_1_20 +gint32 nm_ip_routing_rule_get_suppress_prefixlength (const NMIPRoutingRule *self); +NM_AVAILABLE_IN_1_20 +void nm_ip_routing_rule_set_suppress_prefixlength (NMIPRoutingRule *self, gint32 suppress_prefixlength); + NM_AVAILABLE_IN_1_18 int nm_ip_routing_rule_cmp (const NMIPRoutingRule *rule, const NMIPRoutingRule *other); diff --git a/libnm/libnm.ver b/libnm/libnm.ver index de73be0f69..52999f188a 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1614,6 +1614,8 @@ global: nm_device_modem_get_apn; nm_device_modem_get_device_id; nm_device_modem_get_operator_code; + nm_ip_routing_rule_get_suppress_prefixlength; + nm_ip_routing_rule_set_suppress_prefixlength; nm_setting_connection_get_wait_device_timeout; nm_setting_ethtool_get_optnames; nm_setting_ovs_dpdk_get_devargs; diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index 3618e32f51..1773405d5f 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -923,29 +923,30 @@ nm_ip_routing_rule_to_platform (const NMIPRoutingRule *rule, nm_assert (out_pl); *out_pl = (NMPlatformRoutingRule) { - .addr_family = nm_ip_routing_rule_get_addr_family (rule), - .flags = ( nm_ip_routing_rule_get_invert (rule) - ? FIB_RULE_INVERT - : 0), - .priority = nm_ip_routing_rule_get_priority (rule), - .tos = nm_ip_routing_rule_get_tos (rule), - .ip_proto = nm_ip_routing_rule_get_ipproto (rule), - .fwmark = nm_ip_routing_rule_get_fwmark (rule), - .fwmask = nm_ip_routing_rule_get_fwmask (rule), - .sport_range = { - .start = nm_ip_routing_rule_get_source_port_start (rule), - .end = nm_ip_routing_rule_get_source_port_end (rule), + .addr_family = nm_ip_routing_rule_get_addr_family (rule), + .flags = ( nm_ip_routing_rule_get_invert (rule) + ? FIB_RULE_INVERT + : 0), + .priority = nm_ip_routing_rule_get_priority (rule), + .tos = nm_ip_routing_rule_get_tos (rule), + .ip_proto = nm_ip_routing_rule_get_ipproto (rule), + .fwmark = nm_ip_routing_rule_get_fwmark (rule), + .fwmask = nm_ip_routing_rule_get_fwmask (rule), + .sport_range = { + .start = nm_ip_routing_rule_get_source_port_start (rule), + .end = nm_ip_routing_rule_get_source_port_end (rule), }, - .dport_range = { - .start = nm_ip_routing_rule_get_destination_port_start (rule), - .end = nm_ip_routing_rule_get_destination_port_end (rule), + .dport_range = { + .start = nm_ip_routing_rule_get_destination_port_start (rule), + .end = nm_ip_routing_rule_get_destination_port_end (rule), }, - .src = *(nm_ip_routing_rule_get_from_bin (rule) ?: &nm_ip_addr_zero), - .dst = *(nm_ip_routing_rule_get_to_bin (rule) ?: &nm_ip_addr_zero), - .src_len = nm_ip_routing_rule_get_from_len (rule), - .dst_len = nm_ip_routing_rule_get_to_len (rule), - .action = nm_ip_routing_rule_get_action (rule), - .table = nm_ip_routing_rule_get_table (rule), + .src = *(nm_ip_routing_rule_get_from_bin (rule) ?: &nm_ip_addr_zero), + .dst = *(nm_ip_routing_rule_get_to_bin (rule) ?: &nm_ip_addr_zero), + .src_len = nm_ip_routing_rule_get_from_len (rule), + .dst_len = nm_ip_routing_rule_get_to_len (rule), + .action = nm_ip_routing_rule_get_action (rule), + .table = nm_ip_routing_rule_get_table (rule), + .suppress_prefixlen_inverse = ~((guint32) nm_ip_routing_rule_get_suppress_prefixlength (rule)), }; nm_ip_routing_rule_get_xifname_bin (rule, TRUE, out_pl->iifname); From 15b1304477ed64a10f2be01760d6837ffaaba002 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 12 Jul 2019 11:19:43 +0200 Subject: [PATCH 3/3] policy-routing: take ownership of externally configured rules IP addresses, routes, TC and QDiscs are all tied to a certain interface. So when NetworkManager manages an interface, it can be confident that all related entires should be managed, deleted and modified by NetworkManager. Routing policy rules are global. For that we have NMPRulesManager which keeps track of whether NetworkManager owns a rule. This allows multiple connection profiles to specify the same rule, and NMPRulesManager can consolidate this information to know whether to add or remove the rule. NMPRulesManager would also support to explicitly block a rule by tracking it with negative priority. However that is still unused at the moment. All that devices do is to add rules (track with positive priority) and remove them (untrack) once the profile gets deactivated. As rules are not exclusively owned by NetworkManager, NetworkManager tries not to interfere with rules that it knows nothing about. That means in particular, when NetworkManager starts it will "weakly track" all rules that are present. "weakly track" is mostly interesting for two cases: - when NMPRulesManager had the same rule explicitly tracked (added) by a device, then deactivating the device will leave the rule in place. - when NMPRulesManager had the same rule explicitly blocked (tracked with negative priority), then it would restore the rule when that block gets removed (as said, currently nobody actually does this). Note that when restarting NetworkManager, then the device may stay and the rules kept. However after restart, NetworkManager no longer knows that it previously added this route, so it would weakly track it and never remove them again. That is a problem. Avoid that, by whenever explicitly tracking a rule we also make sure to no longer weakly track it. Most likely this rule was indeed previously managed by NetworkManager. If this was really a rule added by externally, then the user really should choose distinct rule priorities to avoid such conflicts altogether. --- src/devices/nm-device.c | 7 +- src/nm-netns.c | 16 ++++- src/platform/nmp-rules-manager.c | 116 ++++++++++++++++++++++++++----- src/platform/nmp-rules-manager.h | 5 +- src/platform/tests/test-route.c | 12 ++-- 5 files changed, 128 insertions(+), 28 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 6a27469d41..8175456b82 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -6618,10 +6618,15 @@ _routing_rules_sync (NMDevice *self, rule = nm_setting_ip_config_get_routing_rule (s_ip, i); nm_ip_routing_rule_to_platform (rule, &plrule); + + /* We track this rule, but we also make it explicitly not weakly-tracked + * (meaning to untrack NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG at + * the same time). */ nmp_rules_manager_track (rules_manager, &plrule, 10, - user_tag); + user_tag, + NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG); } } } diff --git a/src/nm-netns.c b/src/nm-netns.c index de822151af..c1ced15342 100644 --- a/src/nm-netns.c +++ b/src/nm-netns.c @@ -127,17 +127,27 @@ constructed (GObject *object) priv->rules_manager = nmp_rules_manager_new (priv->platform); - /* Weakly track the default rules and rules that were added - * outside of NetworkManager. */ + /* Weakly track the default rules with a dummy user-tag. These + * rules are always weekly tracked... */ nmp_rules_manager_track_default (priv->rules_manager, AF_UNSPEC, 0, nm_netns_parent_class /* static dummy user-tag */); + + /* Also weakly track all existing rules. These were added before NetworkManager + * starts, so they are probably none of NetworkManager's business. + * + * However note that during service restart, devices may stay up and rules kept. + * That means, after restart such rules may have been added by a previous run + * of NetworkManager, we just don't know. + * + * For that reason, whenever we will touch such rules later one, we make them + * fully owned and no longer weekly tracked. See %NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG. */ nmp_rules_manager_track_from_platform (priv->rules_manager, NULL, AF_UNSPEC, 0, - nm_netns_parent_class /* static dummy user-tag */); + NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG); G_OBJECT_CLASS (nm_netns_parent_class)->constructed (object); } diff --git a/src/platform/nmp-rules-manager.c b/src/platform/nmp-rules-manager.c index 970afcde50..53174e5759 100644 --- a/src/platform/nmp-rules-manager.c +++ b/src/platform/nmp-rules-manager.c @@ -99,6 +99,17 @@ typedef enum { CONFIG_STATE_NONE = 0, CONFIG_STATE_ADDED_BY_US = 1, CONFIG_STATE_REMOVED_BY_US = 2, + + /* ConfigState encodes whether the rule was touched by us at all (CONFIG_STATE_NONE). + * + * Maybe we would only need to track whether we touched the rule at all. But we + * track it more in detail what we did: did we add it (CONFIG_STATE_ADDED_BY_US) + * or did we remove it (CONFIG_STATE_REMOVED_BY_US)? + * Finally, we need CONFIG_STATE_OWNED_BY_US, which means that we didn't actively + * add/remove it, but whenever we are about to undo the add/remove, we need to do it. + * In that sense, CONFIG_STATE_OWNED_BY_US is really just a flag that we unconditionally + * force the state next time when necessary. */ + CONFIG_STATE_OWNED_BY_US = 3, } ConfigState; typedef struct { @@ -111,8 +122,10 @@ typedef struct { * This makes NMPRulesManager stateful (beyond the configuration that indicates * which rules are tracked). * After a restart, NetworkManager would no longer remember which rules were added - * by us. That would need to be fixed by persisting the state and reloading it after - * restart. */ + * by us. + * + * That is partially fixed by NetworkManager taking over the rules that it + * actively configures (see %NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG). */ ConfigState config_state; } RulesObjData; @@ -121,6 +134,15 @@ typedef struct { CList user_tag_lst_head; } RulesUserTagData; +/*****************************************************************************/ + +static void _rules_data_untrack (NMPRulesManager *self, + RulesData *rules_data, + gboolean remove_user_tag_data, + gboolean make_owned_by_us); + +/*****************************************************************************/ + static void _rules_data_assert (const RulesData *rules_data, gboolean linked) { @@ -278,11 +300,31 @@ _rules_data_lookup (GHashTable *by_data, return g_hash_table_lookup (by_data, &rules_data_needle); } +/** + * nmp_rules_manager_track: + * @self: the #NMPRulesManager instance + * @routing_rule: the #NMPlatformRoutingRule to track or untrack + * @track_priority: the priority for tracking the rule. Note that + * negative values indicate a forced absence of the rule. Priorities + * are compared with their absolute values (with higher absolute + * value being more important). For example, if you track the same + * rule twice, once with priority -5 and +10, then the rule is + * present (because the positive number is more important). + * The special value 0 indicates weakly-tracked rules. + * @user_tag: the tag associated with tracking this rule. The same tag + * must be used to untrack the rule later. + * @user_tag_untrack: if not %NULL, at the same time untrack this user-tag + * for the same rule. Note that this is different from a plain nmp_rules_manager_untrack(), + * because it enforces ownership of the now tracked rule. On the other hand, + * a plain nmp_rules_manager_untrack() merely forgets about the tracking. + * The purpose here is to set this to %NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG. + */ void nmp_rules_manager_track (NMPRulesManager *self, const NMPlatformRoutingRule *routing_rule, gint32 track_priority, - gconstpointer user_tag) + gconstpointer user_tag, + gconstpointer user_tag_untrack) { NMPObject obj_stack; const NMPObject *p_obj_stack; @@ -359,6 +401,17 @@ nmp_rules_manager_track (NMPRulesManager *self, } } + if (user_tag_untrack) { + if (user_tag != user_tag_untrack) { + RulesData *rules_data_untrack; + + rules_data_untrack = _rules_data_lookup (self->by_data, p_obj_stack, user_tag_untrack); + if (rules_data_untrack) + _rules_data_untrack (self, rules_data_untrack, FALSE, TRUE); + } else + nm_assert_not_reached (); + } + _rules_data_assert (rules_data, TRUE); if (changed) { @@ -377,7 +430,8 @@ nmp_rules_manager_track (NMPRulesManager *self, static void _rules_data_untrack (NMPRulesManager *self, RulesData *rules_data, - gboolean remove_user_tag_data) + gboolean remove_user_tag_data, + gboolean make_owned_by_us) { RulesObjData *obj_data; @@ -401,15 +455,22 @@ _rules_data_untrack (NMPRulesManager *self, #endif nm_assert (!c_list_is_empty (&rules_data->user_tag_lst)); - if ( remove_user_tag_data - && c_list_length_is (&rules_data->user_tag_lst, 1)) - g_hash_table_remove (self->by_user_tag, &rules_data->user_tag); obj_data = g_hash_table_lookup (self->by_obj, &rules_data->obj); nm_assert (obj_data); nm_assert (c_list_contains (&obj_data->obj_lst_head, &rules_data->obj_lst)); nm_assert (obj_data == g_hash_table_lookup (self->by_obj, &rules_data->obj)); + if (make_owned_by_us) { + if (obj_data->config_state == CONFIG_STATE_NONE) { + /* we need to mark this entry that it requires a touch on the next + * sync. */ + obj_data->config_state = CONFIG_STATE_OWNED_BY_US; + } + } else if ( remove_user_tag_data + && c_list_length_is (&rules_data->user_tag_lst, 1)) + g_hash_table_remove (self->by_user_tag, &rules_data->user_tag); + /* if obj_data is marked to be "added_by_us" or "removed_by_us", we need to keep this entry * around for the next sync -- so that we can undo what we did earlier. */ if ( obj_data->config_state == CONFIG_STATE_NONE @@ -440,7 +501,7 @@ nmp_rules_manager_untrack (NMPRulesManager *self, rules_data = _rules_data_lookup (self->by_data, p_obj_stack, user_tag); if (rules_data) - _rules_data_untrack (self, rules_data, TRUE); + _rules_data_untrack (self, rules_data, TRUE, FALSE); } void @@ -486,7 +547,7 @@ nmp_rules_manager_untrack_all (NMPRulesManager *self, c_list_for_each_entry_safe (rules_data, rules_data_safe, &user_tag_data->user_tag_lst_head, user_tag_lst) { if ( all || rules_data->dirty) - _rules_data_untrack (self, rules_data, FALSE); + _rules_data_untrack (self, rules_data, FALSE, FALSE); } if (c_list_is_empty (&user_tag_data->user_tag_lst_head)) g_hash_table_remove (self->by_user_tag, user_tag_data); @@ -525,11 +586,17 @@ nmp_rules_manager_sync (NMPRulesManager *self, rd_best = _rules_obj_get_best_data (obj_data); if (rd_best) { - if (rd_best->track_priority_present) + if (rd_best->track_priority_present) { + if (obj_data->config_state == CONFIG_STATE_OWNED_BY_US) + obj_data->config_state = CONFIG_STATE_ADDED_BY_US; continue; + } if (rd_best->track_priority_val == 0) { - if (obj_data->config_state != CONFIG_STATE_ADDED_BY_US) + if (!NM_IN_SET (obj_data->config_state, CONFIG_STATE_ADDED_BY_US, + CONFIG_STATE_OWNED_BY_US)) { + obj_data->config_state = CONFIG_STATE_NONE; continue; + } obj_data->config_state = CONFIG_STATE_NONE; } } @@ -563,11 +630,17 @@ nmp_rules_manager_sync (NMPRulesManager *self, continue; } - if (!rd_best->track_priority_present) + if (!rd_best->track_priority_present) { + if (obj_data->config_state == CONFIG_STATE_OWNED_BY_US) + obj_data->config_state = CONFIG_STATE_REMOVED_BY_US; continue; + } if (rd_best->track_priority_val == 0) { - if (obj_data->config_state != CONFIG_STATE_REMOVED_BY_US) + if (!NM_IN_SET (obj_data->config_state, CONFIG_STATE_REMOVED_BY_US, + CONFIG_STATE_OWNED_BY_US)) { + obj_data->config_state = CONFIG_STATE_NONE; continue; + } obj_data->config_state = CONFIG_STATE_NONE; } @@ -610,7 +683,7 @@ nmp_rules_manager_track_from_platform (NMPRulesManager *self, && rr->addr_family != addr_family) continue; - nmp_rules_manager_track (self, rr, tracking_priority, user_tag); + nmp_rules_manager_track (self, rr, tracking_priority, user_tag, NULL); } } @@ -638,7 +711,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self, .protocol = RTPROT_KERNEL, }), track_priority, - user_tag); + user_tag, + NULL); nmp_rules_manager_track (self, &((NMPlatformRoutingRule) { .addr_family = AF_INET, @@ -648,7 +722,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self, .protocol = RTPROT_KERNEL, }), track_priority, - user_tag); + user_tag, + NULL); nmp_rules_manager_track (self, &((NMPlatformRoutingRule) { .addr_family = AF_INET, @@ -658,7 +733,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self, .protocol = RTPROT_KERNEL, }), track_priority, - user_tag); + user_tag, + NULL); } if (NM_IN_SET (addr_family, AF_UNSPEC, AF_INET6)) { nmp_rules_manager_track (self, @@ -670,7 +746,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self, .protocol = RTPROT_KERNEL, }), track_priority, - user_tag); + user_tag, + NULL); nmp_rules_manager_track (self, &((NMPlatformRoutingRule) { .addr_family = AF_INET6, @@ -680,7 +757,8 @@ nmp_rules_manager_track_default (NMPRulesManager *self, .protocol = RTPROT_KERNEL, }), track_priority, - user_tag); + user_tag, + NULL); } } diff --git a/src/platform/nmp-rules-manager.h b/src/platform/nmp-rules-manager.h index 310c7971f2..645df5c289 100644 --- a/src/platform/nmp-rules-manager.h +++ b/src/platform/nmp-rules-manager.h @@ -22,6 +22,8 @@ /*****************************************************************************/ +#define NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG ((const void *) nmp_rules_manager_new) + typedef struct _NMPRulesManager NMPRulesManager; NMPRulesManager *nmp_rules_manager_new (NMPlatform *platform); @@ -35,7 +37,8 @@ NM_AUTO_DEFINE_FCN0 (NMPRulesManager *, _nmp_rules_manager_unref, nmp_rules_mana void nmp_rules_manager_track (NMPRulesManager *self, const NMPlatformRoutingRule *routing_rule, gint32 track_priority, - gconstpointer user_tag); + gconstpointer user_tag, + gconstpointer user_tag_untrack); void nmp_rules_manager_track_default (NMPRulesManager *self, int addr_family, diff --git a/src/platform/tests/test-route.c b/src/platform/tests/test-route.c index 0d7fdcf338..44bfbc583b 100644 --- a/src/platform/tests/test-route.c +++ b/src/platform/tests/test-route.c @@ -1534,14 +1534,16 @@ again: nmp_rules_manager_track (rules_manager, NMP_OBJECT_CAST_ROUTING_RULE (objs_sync->pdata[i]), 1, - USER_TAG_1); + USER_TAG_1, + NULL); if (nmtst_get_rand_bool ()) { /* this has no effect, because a negative priority (of same absolute value) * has lower priority than the positive priority above. */ nmp_rules_manager_track (rules_manager, NMP_OBJECT_CAST_ROUTING_RULE (objs_sync->pdata[i]), -1, - USER_TAG_2); + USER_TAG_2, + NULL); } if (nmtst_get_rand_uint32 () % objs_sync->len == 0) { nmp_rules_manager_sync (rules_manager, FALSE); @@ -1566,13 +1568,15 @@ again: nmp_rules_manager_track (rules_manager, NMP_OBJECT_CAST_ROUTING_RULE (objs_sync->pdata[i]), -1, - USER_TAG_1); + USER_TAG_1, + NULL); break; case 2: nmp_rules_manager_track (rules_manager, NMP_OBJECT_CAST_ROUTING_RULE (objs_sync->pdata[i]), -2, - USER_TAG_2); + USER_TAG_2, + NULL); break; } if (nmtst_get_rand_uint32 () % objs_sync->len == 0) {