From 5f882e8e8f7795900d4923e8304611b7f9bcd669 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 14:56:51 +0200 Subject: [PATCH 01/16] libnm: reject colon in nm_utils_is_valid_iface_name() Since kernel commit a4176a9391868bfa87705bcd2e3b49e9b9dd2996 (net: reject creation of netdev names with colons), kernel rejects any colons in the interface name. Since kernel could get away with tightening up the check, we can too. The user anyway can not choose arbitrary interface names, like "all", "default", "bonding_masters" are all going to fail one way or another. --- libnm-core/nm-utils.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 7e9618434..e700b714e 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -32,6 +32,7 @@ #include #include #include +#include #if WITH_JANSSON #include @@ -3675,36 +3676,41 @@ _nm_utils_generate_mac_address_mask_parse (const char *value, gboolean nm_utils_is_valid_iface_name (const char *name, GError **error) { - g_return_val_if_fail (name != NULL, FALSE); + int i; - if (*name == '\0') { + g_return_val_if_fail (name, FALSE); + + if (name[0] == '\0') { g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, _("interface name is too short")); return FALSE; } - if (strlen (name) >= 16) { - g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - _("interface name is longer than 15 characters")); - return FALSE; - } - - if (!strcmp (name, ".") || !strcmp (name, "..")) { + if ( name[0] == '.' + && ( name[1] == '\0' + || ( name[1] == '.' + && name[2] == '\0'))) { g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, _("interface name is reserved")); return FALSE; } - while (*name) { - if (*name == '/' || g_ascii_isspace (*name)) { + for (i = 0; i < IFNAMSIZ; i++) { + char ch = name[i]; + + if (ch == '\0') + return TRUE; + if ( NM_IN_SET (ch, '/', ':') + || g_ascii_isspace (ch)) { g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, _("interface name contains an invalid character")); return FALSE; } - name++; } - return TRUE; + g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + _("interface name is longer than 15 characters")); + return FALSE; } /** From 8daa61dae354ea3c5138222e2de2ab15e23f6d3a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 16:59:10 +0200 Subject: [PATCH 02/16] platform: fix return value for nm_platform_sysctl_set() When comparing an unsigned and a signed integer, the signed integer is promoted to unsigned, resulting in a very large number. See the checks "nwrote < len - 1", where nwrote might be -1 to indicate failure. The condition would not be TRUE due to promoting -1 to the max int value. Hence, sysctl_set() was rather wrong. --- src/platform/nm-linux-platform.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 440d2760c..d304bdfbf 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -2934,7 +2934,7 @@ sysctl_set (NMPlatform *platform, const char *pathid, int dirfd, const char *pat nm_auto_pop_netns NMPNetns *netns = NULL; int fd, tries; gssize nwrote; - gsize len; + gssize len; char *actual; gs_free char *actual_free = NULL; int errsv; @@ -2989,6 +2989,7 @@ sysctl_set (NMPlatform *platform, const char *pathid, int dirfd, const char *pat * about to write. */ len = strlen (value) + 1; + nm_assert (len > 0); if (len > 512) actual = actual_free = g_malloc (len + 1); else From 05c4497bddffbd1ab024d2f249b3e2400373d325 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 13 Oct 2017 13:11:11 +0200 Subject: [PATCH 03/16] device: set MTU property of device in new _set_mtu() function --- src/devices/nm-device.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index e7e076ec2..f3ae934e2 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -550,6 +550,7 @@ static void realize_start_setup (NMDevice *self, const char *assume_state_connection_uuid, gboolean set_nm_owned, NMUnmanFlagOp unmanaged_user_explicit); +static void _set_mtu (NMDevice *self, guint32 mtu); static void _commit_mtu (NMDevice *self, const NMIP4Config *config); static void dhcp_schedule_restart (NMDevice *self, int addr_family, const char *reason); static void _cancel_activation (NMDevice *self); @@ -2672,10 +2673,7 @@ device_link_changed (NMDevice *self) _notify (self, PROP_DRIVER); } - if (priv->mtu != info.mtu) { - priv->mtu = info.mtu; - _notify (self, PROP_MTU); - } + _set_mtu (self, info.mtu); if (ifindex == nm_device_get_ip_ifindex (self)) _stats_update_counters_from_pllink (self, &info); @@ -3242,7 +3240,6 @@ realize_start_setup (NMDevice *self, NMDeviceCapabilities capabilities = 0; NMConfig *config; guint real_rate; - guint32 mtu; /* plink is a NMPlatformLink type, however, we require it to come from the platform * cache (where else would it come from?). */ @@ -3271,10 +3268,7 @@ realize_start_setup (NMDevice *self, priv->mtu_initial = 0; priv->ip6_mtu_initial = 0; priv->ip6_mtu = 0; - if (priv->mtu) { - priv->mtu = 0; - _notify (self, PROP_MTU); - } + _set_mtu (self, 0); _assume_state_set (self, assume_state_guess_assume, assume_state_connection_uuid); @@ -3295,11 +3289,9 @@ realize_start_setup (NMDevice *self, if (nm_platform_link_is_software (nm_device_get_platform (self), priv->ifindex)) capabilities |= NM_DEVICE_CAP_IS_SOFTWARE; - mtu = nm_platform_link_get_mtu (nm_device_get_platform (self), priv->ifindex); - if (priv->mtu != mtu) { - priv->mtu = mtu; - _notify (self, PROP_MTU); - } + _set_mtu (self, + nm_platform_link_get_mtu (nm_device_get_platform (self), + priv->ifindex)); nm_platform_link_get_driver_info (nm_device_get_platform (self), priv->ifindex, @@ -3524,10 +3516,7 @@ nm_device_unrealize (NMDevice *self, gboolean remove_resources, GError **error) if (nm_clear_g_free (&priv->ip_iface)) _notify (self, PROP_IP_IFACE); - if (priv->mtu != 0) { - priv->mtu = 0; - _notify (self, PROP_MTU); - } + _set_mtu (self, 0); if (priv->driver_version) { g_clear_pointer (&priv->driver_version, g_free); @@ -7211,6 +7200,18 @@ nm_device_get_configured_mtu_for_wired (NMDevice *self, gboolean *out_is_user_co /*****************************************************************************/ +static void +_set_mtu (NMDevice *self, guint32 mtu) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + + if (priv->mtu == mtu) + return; + + priv->mtu = mtu; + _notify (self, PROP_MTU); +} + static void _commit_mtu (NMDevice *self, const NMIP4Config *config) { From 667aed8aeb3ebd7f3abbf34fd6acf16d9dcd4dff Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Oct 2017 19:36:09 +0200 Subject: [PATCH 04/16] device: reset MTU when VLAN's parent device changes MTU Kernel does not allow setting the MTU of a VLAN larger then the MTU of the underlying device. Hence, we might initially fail to set a large MTU of the VLAN, but we have to retry when the MTU of the parent changes. https://bugzilla.redhat.com/show_bug.cgi?id=1414901 --- src/devices/nm-device-private.h | 2 ++ src/devices/nm-device-vlan.c | 21 ++++++++++++++++++++- src/devices/nm-device.c | 19 +++++++++++++++++-- 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/devices/nm-device-private.h b/src/devices/nm-device-private.h index 416ae845c..f1486c549 100644 --- a/src/devices/nm-device-private.h +++ b/src/devices/nm-device-private.h @@ -118,6 +118,8 @@ gint64 nm_device_get_configured_mtu_from_connection_default (NMDevice *self, guint32 nm_device_get_configured_mtu_for_wired (NMDevice *self, gboolean *out_is_user_config); +void nm_device_commit_mtu (NMDevice *self); + /*****************************************************************************/ #define NM_DEVICE_CLASS_DECLARE_TYPES(klass, conn_type, ...) \ diff --git a/src/devices/nm-device-vlan.c b/src/devices/nm-device-vlan.c index 06f19c408..e30dae744 100644 --- a/src/devices/nm-device-vlan.c +++ b/src/devices/nm-device-vlan.c @@ -51,6 +51,7 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMDeviceVlan, typedef struct { gulong parent_state_id; gulong parent_hwaddr_id; + gulong parent_mtu_id; guint vlan_id; } NMDeviceVlanPrivate; @@ -85,6 +86,17 @@ parent_state_changed (NMDevice *parent, nm_device_set_unmanaged_by_flags (NM_DEVICE (self), NM_UNMANAGED_PARENT, !nm_device_get_managed (parent, FALSE), reason); } +static void +parent_mtu_maybe_changed (NMDevice *parent, + GParamSpec *pspec, + gpointer user_data) +{ + /* the MTU of a VLAN device is limited by the parent's MTU. + * + * When the parent's MTU changes, try to re-set the MTU. */ + nm_device_commit_mtu (user_data); +} + static void parent_hwaddr_maybe_changed (NMDevice *parent, GParamSpec *pspec, @@ -143,6 +155,7 @@ parent_changed_notify (NMDevice *device, * parent_changed_notify(). */ nm_clear_g_signal_handler (old_parent, &priv->parent_state_id); nm_clear_g_signal_handler (old_parent, &priv->parent_hwaddr_id); + nm_clear_g_signal_handler (old_parent, &priv->parent_mtu_id); if (new_parent) { priv->parent_state_id = g_signal_connect (new_parent, @@ -154,6 +167,10 @@ parent_changed_notify (NMDevice *device, G_CALLBACK (parent_hwaddr_maybe_changed), device); parent_hwaddr_maybe_changed (new_parent, NULL, self); + priv->parent_mtu_id = g_signal_connect (new_parent, "notify::" NM_DEVICE_MTU, + G_CALLBACK (parent_mtu_maybe_changed), device); + parent_mtu_maybe_changed (new_parent, NULL, self); + /* Set parent-dependent unmanaged flag */ nm_device_set_unmanaged_by_flags (device, NM_UNMANAGED_PARENT, @@ -482,8 +499,10 @@ act_stage1_prepare (NMDevice *device, NMDeviceStateReason *out_failure_reason) /* Change MAC address to parent's one if needed */ parent_device = nm_device_parent_get_device (device); - if (parent_device) + if (parent_device) { parent_hwaddr_maybe_changed (parent_device, NULL, device); + parent_mtu_maybe_changed (parent_device, NULL, device); + } s_vlan = (NMSettingVlan *) nm_device_get_applied_setting (device, NM_TYPE_SETTING_VLAN); if (s_vlan) { diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index f3ae934e2..9a5a40136 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7231,8 +7231,7 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config) return; if (nm_device_sys_iface_state_is_external_or_assume (self)) { - /* for assumed connections we don't tamper with the MTU. This is - * a bug and supposed to be fixed by the unmanaged/assumed rework. */ + /* for assumed connections we don't tamper with the MTU. */ return; } @@ -7354,6 +7353,22 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config) #undef _IP6_MTU_SYS } +void +nm_device_commit_mtu (NMDevice *self) +{ + NMDeviceState state; + + g_return_if_fail (NM_IS_DEVICE (self)); + + state = nm_device_get_state (self); + if ( state >= NM_DEVICE_STATE_CONFIG + && state < NM_DEVICE_STATE_DEACTIVATING) { + _LOGT (LOGD_DEVICE, "mtu: commit-mtu..."); + _commit_mtu (self, NM_DEVICE_GET_PRIVATE (self)->ip4_config); + } else + _LOGT (LOGD_DEVICE, "mtu: commit-mtu... skip due to state %s", nm_device_state_to_str (state)); +} + static void ndisc_config_changed (NMNDisc *ndisc, const NMNDiscData *rdata, guint changed_int, NMDevice *self) { From 09ee0c92050593b4d1122260af722d6d84d7082c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 18 Oct 2017 19:01:50 +0200 Subject: [PATCH 05/16] device: reset MTU when slave's MTU changes --- src/devices/nm-device.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 9a5a40136..9e2e67fba 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7210,6 +7210,14 @@ _set_mtu (NMDevice *self, guint32 mtu) priv->mtu = mtu; _notify (self, PROP_MTU); + + if (priv->master) { + /* changing the MTU of a slave, might require the master to reset + * it's MTU. Note that the master usually cannot set a MTU larger + * then the slave's. Hence, when the slave increases the MTU, + * master might want to retry setting the MTU. */ + nm_device_commit_mtu (priv->master); + } } static void From 42cfcf6f23414492b07f2839f93b2e947ba71bc9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 12:48:49 +0200 Subject: [PATCH 06/16] platform: downgrade warning about failure to set MTU Setting the MTU failes under regular conditions, for example when setting the MTU of a master larger then the MTU of the slaves. Logging a warning it too alarming. --- src/platform/nm-linux-platform.c | 31 +++++++++++++++++++++---------- src/platform/nm-platform.c | 1 + src/platform/nm-platform.h | 1 + 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index d304bdfbf..315c8f63d 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -206,6 +206,11 @@ typedef enum { INFINIBAND_ACTION_DELETE_CHILD, } InfinibandAction; +typedef enum { + CHANGE_LINK_TYPE_UNSPEC, + CHANGE_LINK_TYPE_SET_MTU, +} ChangeLinkType; + enum { DELAYED_ACTION_IDX_REFRESH_ALL_LINKS, DELAYED_ACTION_IDX_REFRESH_ALL_IP4_ADDRESSES, @@ -4450,6 +4455,7 @@ retry: static NMPlatformError do_change_link_result (NMPlatform *platform, + ChangeLinkType change_link_type, int ifindex, WaitForNlResponseResult seq_result) { @@ -4465,6 +4471,10 @@ do_change_link_result (NMPlatform *platform, } else if (NM_IN_SET (-((int) seq_result), ESRCH, ENOENT)) { log_detail = ", firmware not found"; result = NM_PLATFORM_ERROR_NO_FIRMWARE; + } else if ( NM_IN_SET (-((int) seq_result), ERANGE) + && change_link_type == CHANGE_LINK_TYPE_SET_MTU) { + log_detail = ", setting MTU to requested size is not possible"; + result = NM_PLATFORM_ERROR_CANT_SET_MTU; } else if (NM_IN_SET (-((int) seq_result), ENODEV)) { log_level = LOGL_DEBUG; result = NM_PLATFORM_ERROR_NOT_FOUND; @@ -4484,13 +4494,14 @@ do_change_link_result (NMPlatform *platform, static NMPlatformError do_change_link (NMPlatform *platform, + ChangeLinkType change_link_type, int ifindex, struct nl_msg *nlmsg) { WaitForNlResponseResult seq_result; seq_result = do_change_link_request (platform, ifindex, nlmsg); - return do_change_link_result (platform, ifindex, seq_result); + return do_change_link_result (platform, change_link_type, ifindex, seq_result); } static gboolean @@ -4584,7 +4595,7 @@ link_set_netns (NMPlatform *platform, return FALSE; NLA_PUT (nlmsg, IFLA_NET_NS_FD, 4, &netns_fd); - return do_change_link (platform, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; nla_put_failure: g_return_val_if_reached (FALSE); @@ -4614,7 +4625,7 @@ link_change_flags (NMPlatform *platform, flags_set); if (!nlmsg) return NM_PLATFORM_ERROR_UNSPECIFIED; - return do_change_link (platform, ifindex, nlmsg); + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg); } static gboolean @@ -4683,7 +4694,7 @@ link_set_user_ipv6ll_enabled (NMPlatform *platform, int ifindex, gboolean enable || !_nl_msg_new_link_set_afspec (nlmsg, mode, NULL)) g_return_val_if_reached (NM_PLATFORM_ERROR_BUG); - return do_change_link (platform, ifindex, nlmsg); + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg); } static gboolean @@ -4698,7 +4709,7 @@ link_set_token (NMPlatform *platform, int ifindex, NMUtilsIPv6IfaceId iid) if (!nlmsg || !_nl_msg_new_link_set_afspec (nlmsg, -1, &iid)) g_return_val_if_reached (FALSE); - return do_change_link (platform, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; } static gboolean @@ -4806,7 +4817,7 @@ link_set_address (NMPlatform *platform, int ifindex, gconstpointer address, size } } - return do_change_link_result (platform, ifindex, seq_result); + return do_change_link_result (platform, ifindex, CHANGE_LINK_TYPE_UNSPEC, seq_result); nla_put_failure: g_return_val_if_reached (NM_PLATFORM_ERROR_UNSPECIFIED); @@ -4830,7 +4841,7 @@ link_set_name (NMPlatform *platform, int ifindex, const char *name) NLA_PUT (nlmsg, IFLA_IFNAME, strlen (name) + 1, name); - return do_change_link (platform, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; nla_put_failure: g_return_val_if_reached (FALSE); } @@ -4867,7 +4878,7 @@ link_set_mtu (NMPlatform *platform, int ifindex, guint32 mtu) NLA_PUT_U32 (nlmsg, IFLA_MTU, mtu); - return do_change_link (platform, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_SET_MTU, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; nla_put_failure: g_return_val_if_reached (FALSE); } @@ -5575,7 +5586,7 @@ link_vlan_change (NMPlatform *platform, new_n_egress_map)) g_return_val_if_reached (FALSE); - return do_change_link (platform, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; } static int @@ -5655,7 +5666,7 @@ link_enslave (NMPlatform *platform, int master, int slave) NLA_PUT_U32 (nlmsg, IFLA_MASTER, master); - return do_change_link (platform, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; nla_put_failure: g_return_val_if_reached (FALSE); } diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 1773b15ad..90ea2fc72 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -240,6 +240,7 @@ NM_UTILS_LOOKUP_STR_DEFINE_STATIC (_nm_platform_error_to_string, NMPlatformError NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_NO_FIRMWARE, "no-firmware"), NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_OPNOTSUPP, "not-supported"), NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_NETLINK, "netlink"), + NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_CANT_SET_MTU, "cant-set-mtu"), NM_UTILS_LOOKUP_ITEM_IGNORE (_NM_PLATFORM_ERROR_MININT), ); diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index bdf5dbc2c..0d40ee129 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -166,6 +166,7 @@ typedef enum { /*< skip >*/ NM_PLATFORM_ERROR_NO_FIRMWARE, NM_PLATFORM_ERROR_OPNOTSUPP, NM_PLATFORM_ERROR_NETLINK, + NM_PLATFORM_ERROR_CANT_SET_MTU, } NMPlatformError; #define NM_PLATFORM_LINK_OTHER_NETNS (-1) From c0c23911da6929caed2e2161ce547cf24d87cdd5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 13:17:21 +0200 Subject: [PATCH 07/16] platform: move evaluating the result of set_address to do_change_link_result() Move all evaluations of the result at one place. --- src/platform/nm-linux-platform.c | 78 +++++++++++++++++--------------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 315c8f63d..53e605ae4 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -209,8 +209,18 @@ typedef enum { typedef enum { CHANGE_LINK_TYPE_UNSPEC, CHANGE_LINK_TYPE_SET_MTU, + CHANGE_LINK_TYPE_SET_ADDRESS, } ChangeLinkType; +typedef struct { + union { + struct { + gconstpointer address; + gsize length; + } set_address; + }; +} ChangeLinkData; + enum { DELAYED_ACTION_IDX_REFRESH_ALL_LINKS, DELAYED_ACTION_IDX_REFRESH_ALL_IP4_ADDRESSES, @@ -4457,12 +4467,14 @@ static NMPlatformError do_change_link_result (NMPlatform *platform, ChangeLinkType change_link_type, int ifindex, - WaitForNlResponseResult seq_result) + WaitForNlResponseResult seq_result, + const ChangeLinkData *data) { char s_buf[256]; NMPlatformError result = NM_PLATFORM_ERROR_SUCCESS; NMLogLevel log_level = LOGL_DEBUG; const char *log_result = "failure", *log_detail = ""; + const NMPObject *obj_cache; if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK) { log_result = "success"; @@ -4475,6 +4487,16 @@ do_change_link_result (NMPlatform *platform, && change_link_type == CHANGE_LINK_TYPE_SET_MTU) { log_detail = ", setting MTU to requested size is not possible"; result = NM_PLATFORM_ERROR_CANT_SET_MTU; + } else if ( NM_IN_SET (-((int) seq_result), ENFILE) + && change_link_type == CHANGE_LINK_TYPE_SET_ADDRESS + && (obj_cache = nmp_cache_lookup_link (nm_platform_get_cache (platform), ifindex)) + && obj_cache->link.addr.len == data->set_address.length + && memcmp (obj_cache->link.addr.data, data->set_address.address, data->set_address.length) == 0) { + /* workaround ENFILE which may be wrongly returned (bgo #770456). + * If the MAC address is as expected, assume success? */ + log_result = "success"; + log_detail = " (assume success changing address)"; + result = NM_PLATFORM_ERROR_SUCCESS; } else if (NM_IN_SET (-((int) seq_result), ENODEV)) { log_level = LOGL_DEBUG; result = NM_PLATFORM_ERROR_NOT_FOUND; @@ -4496,12 +4518,13 @@ static NMPlatformError do_change_link (NMPlatform *platform, ChangeLinkType change_link_type, int ifindex, - struct nl_msg *nlmsg) + struct nl_msg *nlmsg, + const ChangeLinkData *data) { WaitForNlResponseResult seq_result; seq_result = do_change_link_request (platform, ifindex, nlmsg); - return do_change_link_result (platform, change_link_type, ifindex, seq_result); + return do_change_link_result (platform, change_link_type, ifindex, seq_result, data); } static gboolean @@ -4595,7 +4618,7 @@ link_set_netns (NMPlatform *platform, return FALSE; NLA_PUT (nlmsg, IFLA_NET_NS_FD, 4, &netns_fd); - return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg, NULL) == NM_PLATFORM_ERROR_SUCCESS; nla_put_failure: g_return_val_if_reached (FALSE); @@ -4625,7 +4648,7 @@ link_change_flags (NMPlatform *platform, flags_set); if (!nlmsg) return NM_PLATFORM_ERROR_UNSPECIFIED; - return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg); + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg, NULL); } static gboolean @@ -4694,7 +4717,7 @@ link_set_user_ipv6ll_enabled (NMPlatform *platform, int ifindex, gboolean enable || !_nl_msg_new_link_set_afspec (nlmsg, mode, NULL)) g_return_val_if_reached (NM_PLATFORM_ERROR_BUG); - return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg); + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg, NULL); } static gboolean @@ -4709,7 +4732,7 @@ link_set_token (NMPlatform *platform, int ifindex, NMUtilsIPv6IfaceId iid) if (!nlmsg || !_nl_msg_new_link_set_afspec (nlmsg, -1, &iid)) g_return_val_if_reached (FALSE); - return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg, NULL) == NM_PLATFORM_ERROR_SUCCESS; } static gboolean @@ -4774,8 +4797,12 @@ link_set_address (NMPlatform *platform, int ifindex, gconstpointer address, size { nm_auto_nlmsg struct nl_msg *nlmsg = NULL; gs_free char *mac = NULL; - WaitForNlResponseResult seq_result; - char s_buf[256]; + const ChangeLinkData d = { + .set_address = { + .address = address, + .length = length, + }, + }; if (!address || !length) g_return_val_if_reached (NM_PLATFORM_ERROR_BUG); @@ -4795,30 +4822,7 @@ link_set_address (NMPlatform *platform, int ifindex, gconstpointer address, size NLA_PUT (nlmsg, IFLA_ADDRESS, length, address); - seq_result = do_change_link_request (platform, ifindex, nlmsg); - - if (NM_IN_SET (-((int) seq_result), ENFILE)) { - const NMPObject *obj_cache; - - /* workaround ENFILE which may be wrongly returned (bgo #770456). - * If the MAC address is as expected, assume success? */ - - obj_cache = nmp_cache_lookup_link (nm_platform_get_cache (platform), ifindex); - if ( obj_cache - && obj_cache->link.addr.len == length - && memcmp (obj_cache->link.addr.data, address, length) == 0) { - _NMLOG (LOGL_DEBUG, - "do-change-link[%d]: %s changing link: %s%s", - ifindex, - "success", - wait_for_nl_response_to_string (seq_result, s_buf, sizeof (s_buf)), - " (assume success changing address)"); - return NM_PLATFORM_ERROR_SUCCESS; - } - } - - return do_change_link_result (platform, ifindex, CHANGE_LINK_TYPE_UNSPEC, seq_result); - + return do_change_link (platform, CHANGE_LINK_TYPE_SET_ADDRESS, ifindex, nlmsg, &d); nla_put_failure: g_return_val_if_reached (NM_PLATFORM_ERROR_UNSPECIFIED); } @@ -4841,7 +4845,7 @@ link_set_name (NMPlatform *platform, int ifindex, const char *name) NLA_PUT (nlmsg, IFLA_IFNAME, strlen (name) + 1, name); - return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg, NULL) == NM_PLATFORM_ERROR_SUCCESS; nla_put_failure: g_return_val_if_reached (FALSE); } @@ -4878,7 +4882,7 @@ link_set_mtu (NMPlatform *platform, int ifindex, guint32 mtu) NLA_PUT_U32 (nlmsg, IFLA_MTU, mtu); - return do_change_link (platform, CHANGE_LINK_TYPE_SET_MTU, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_SET_MTU, ifindex, nlmsg, NULL) == NM_PLATFORM_ERROR_SUCCESS; nla_put_failure: g_return_val_if_reached (FALSE); } @@ -5586,7 +5590,7 @@ link_vlan_change (NMPlatform *platform, new_n_egress_map)) g_return_val_if_reached (FALSE); - return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg, NULL) == NM_PLATFORM_ERROR_SUCCESS; } static int @@ -5666,7 +5670,7 @@ link_enslave (NMPlatform *platform, int master, int slave) NLA_PUT_U32 (nlmsg, IFLA_MASTER, master); - return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_UNSPEC, ifindex, nlmsg, NULL) == NM_PLATFORM_ERROR_SUCCESS; nla_put_failure: g_return_val_if_reached (FALSE); } From a37532a69457af8bea8ee9bb10c3f9e1146815e5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 13:25:54 +0200 Subject: [PATCH 08/16] platform: merge do_change_link_result() into do_change_link() There is only one caller left. --- src/platform/nm-linux-platform.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 53e605ae4..2872484a6 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -4464,18 +4464,21 @@ retry: } static NMPlatformError -do_change_link_result (NMPlatform *platform, - ChangeLinkType change_link_type, - int ifindex, - WaitForNlResponseResult seq_result, - const ChangeLinkData *data) +do_change_link (NMPlatform *platform, + ChangeLinkType change_link_type, + int ifindex, + struct nl_msg *nlmsg, + const ChangeLinkData *data) { + WaitForNlResponseResult seq_result; char s_buf[256]; NMPlatformError result = NM_PLATFORM_ERROR_SUCCESS; NMLogLevel log_level = LOGL_DEBUG; const char *log_result = "failure", *log_detail = ""; const NMPObject *obj_cache; + seq_result = do_change_link_request (platform, ifindex, nlmsg); + if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK) { log_result = "success"; } else if (NM_IN_SET (-((int) seq_result), EEXIST, EADDRINUSE)) { @@ -4514,19 +4517,6 @@ do_change_link_result (NMPlatform *platform, return result; } -static NMPlatformError -do_change_link (NMPlatform *platform, - ChangeLinkType change_link_type, - int ifindex, - struct nl_msg *nlmsg, - const ChangeLinkData *data) -{ - WaitForNlResponseResult seq_result; - - seq_result = do_change_link_request (platform, ifindex, nlmsg); - return do_change_link_result (platform, change_link_type, ifindex, seq_result, data); -} - static gboolean link_add (NMPlatform *platform, const char *name, From b27a10bde8f0e28b16cc52e2c2f7de39d44cf0e6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 13:26:14 +0200 Subject: [PATCH 09/16] platform: merge do_change_link_request() into do_change_link() There is only one caller left. --- src/platform/nm-linux-platform.c | 56 +++++++++++++++----------------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 2872484a6..7b2ac003e 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -4426,25 +4426,38 @@ do_delete_object (NMPlatform *platform, const NMPObject *obj_id, struct nl_msg * return success; } -static WaitForNlResponseResult -do_change_link_request (NMPlatform *platform, - int ifindex, - struct nl_msg *nlmsg) +static NMPlatformError +do_change_link (NMPlatform *platform, + ChangeLinkType change_link_type, + int ifindex, + struct nl_msg *nlmsg, + const ChangeLinkData *data) { nm_auto_pop_netns NMPNetns *netns = NULL; - WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; int nle; + WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; + char s_buf[256]; + NMPlatformError result = NM_PLATFORM_ERROR_SUCCESS; + NMLogLevel log_level = LOGL_DEBUG; + const char *log_result = "failure"; + const char *log_detail = ""; + gs_free char *log_detail_free = NULL; + const NMPObject *obj_cache; - if (!nm_platform_netns_push (platform, &netns)) - return WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; + if (!nm_platform_netns_push (platform, &netns)) { + log_level = LOGL_ERR; + log_detail = ", failure to change network namespace"; + goto out; + } retry: nle = _nl_send_nlmsg (platform, nlmsg, &seq_result, DELAYED_ACTION_RESPONSE_TYPE_VOID, NULL); if (nle < 0) { - _LOGE ("do-change-link[%d]: failure sending netlink request \"%s\" (%d)", - ifindex, - nl_geterror (nle), -nle); - return WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; + log_level = LOGL_ERR; + log_detail_free = g_strdup_printf (", failure sending netlink request: %s (%d)", + nl_geterror (nle), -nle); + log_detail = log_detail_free; + goto out; } /* always refetch the link after changing it. There seems to be issues @@ -4460,24 +4473,6 @@ retry: nlmsg_hdr (nlmsg)->nlmsg_type = RTM_SETLINK; goto retry; } - return seq_result; -} - -static NMPlatformError -do_change_link (NMPlatform *platform, - ChangeLinkType change_link_type, - int ifindex, - struct nl_msg *nlmsg, - const ChangeLinkData *data) -{ - WaitForNlResponseResult seq_result; - char s_buf[256]; - NMPlatformError result = NM_PLATFORM_ERROR_SUCCESS; - NMLogLevel log_level = LOGL_DEBUG; - const char *log_result = "failure", *log_detail = ""; - const NMPObject *obj_cache; - - seq_result = do_change_link_request (platform, ifindex, nlmsg); if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK) { log_result = "success"; @@ -4507,13 +4502,14 @@ do_change_link (NMPlatform *platform, log_level = LOGL_WARN; result = NM_PLATFORM_ERROR_UNSPECIFIED; } + +out: _NMLOG (log_level, "do-change-link[%d]: %s changing link: %s%s", ifindex, log_result, wait_for_nl_response_to_string (seq_result, s_buf, sizeof (s_buf)), log_detail); - return result; } From 6e01238a407ac546bf20fd23998b28cde672aeba Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 13:55:54 +0200 Subject: [PATCH 10/16] core: don't use static buffer for nm_utils_ip4_property_path() and nm_utils_ip6_property_path(). The API with static buffers looks a bit nicer. But I think they are dangerous, because we tend to pass the buffer down several layers of the stack, and it's not immediately clear, that we don't overwrite the static buffer again (which we probably did not, but it's hard to verify that there is no bug there). --- src/devices/nm-device.c | 25 ++++++++++++++++++------- src/ndisc/nm-lndp-ndisc.c | 4 +++- src/nm-core-utils.c | 38 +++++++++++++++++++++++--------------- src/nm-core-utils.h | 7 +++++-- src/nm-iface-helper.c | 14 ++++++++------ src/platform/nm-platform.c | 3 ++- 6 files changed, 59 insertions(+), 32 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 9e2e67fba..cceda7ae3 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -852,26 +852,29 @@ nm_device_ipv4_sysctl_set (NMDevice *self, const char *property, const char *val NMPlatform *platform = nm_device_get_platform (self); gs_free char *value_to_free = NULL; const char *value_to_set; + char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; if (value) { value_to_set = value; } else { /* Set to a default value when we've got a NULL @value. */ value_to_free = nm_platform_sysctl_get (platform, - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path ("default", property))); + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (buf, "default", property))); value_to_set = value_to_free; } return nm_platform_sysctl_set (platform, - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (nm_device_get_ip_iface (self), property)), + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (buf, nm_device_get_ip_iface (self), property)), value_to_set); } static guint32 nm_device_ipv4_sysctl_get_uint32 (NMDevice *self, const char *property, guint32 fallback) { + char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + return nm_platform_sysctl_get_int_checked (nm_device_get_platform (self), - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (nm_device_get_ip_iface (self), property)), + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (buf, nm_device_get_ip_iface (self), property)), 10, 0, G_MAXUINT32, @@ -881,14 +884,18 @@ nm_device_ipv4_sysctl_get_uint32 (NMDevice *self, const char *property, guint32 gboolean nm_device_ipv6_sysctl_set (NMDevice *self, const char *property, const char *value) { - return nm_platform_sysctl_set (nm_device_get_platform (self), NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (nm_device_get_ip_iface (self), property)), value); + char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + + return nm_platform_sysctl_set (nm_device_get_platform (self), NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, nm_device_get_ip_iface (self), property)), value); } static guint32 nm_device_ipv6_sysctl_get_uint32 (NMDevice *self, const char *property, guint32 fallback) { + char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + return nm_platform_sysctl_get_int_checked (nm_device_get_platform (self), - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (nm_device_get_ip_iface (self), property)), + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, nm_device_get_ip_iface (self), property)), 10, 0, G_MAXUINT32, @@ -7678,7 +7685,9 @@ save_ip6_properties (NMDevice *self) g_hash_table_remove_all (priv->ip6_saved_properties); for (i = 0; i < G_N_ELEMENTS (ip6_properties_to_save); i++) { - value = nm_platform_sysctl_get (nm_device_get_platform (self), NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (ifname, ip6_properties_to_save[i]))); + char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + + value = nm_platform_sysctl_get (nm_device_get_platform (self), NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, ifname, ip6_properties_to_save[i]))); if (value) { g_hash_table_insert (priv->ip6_saved_properties, (char *) ip6_properties_to_save[i], @@ -7738,9 +7747,11 @@ set_nm_ipv6ll (NMDevice *self, gboolean enable) } if (enable) { + char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + /* Bounce IPv6 to ensure the kernel stops IPv6LL address generation */ value = nm_platform_sysctl_get (nm_device_get_platform (self), - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (nm_device_get_ip_iface (self), "disable_ipv6"))); + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, nm_device_get_ip_iface (self), "disable_ipv6"))); if (g_strcmp0 (value, "0") == 0) nm_device_ipv6_sysctl_set (self, "disable_ipv6", "1"); g_free (value); diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index b27d7f8bf..e155402e1 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -531,8 +531,10 @@ start (NMNDisc *ndisc) static inline int ipv6_sysctl_get (NMPlatform *platform, const char *ifname, const char *property, int min, int max, int defval) { + char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + return (int) nm_platform_sysctl_get_int_checked (platform, - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (ifname, property)), + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, ifname, property)), 10, min, max, diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 138d87171..2325283e6 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -2525,55 +2525,63 @@ nm_utils_monotonic_timestamp_as_boottime (gint64 timestamp, gint64 timestamp_ns_ #define IPV6_PROPERTY_DIR "/proc/sys/net/ipv6/conf/" #define IPV4_PROPERTY_DIR "/proc/sys/net/ipv4/conf/" G_STATIC_ASSERT (sizeof (IPV4_PROPERTY_DIR) == sizeof (IPV6_PROPERTY_DIR)); +G_STATIC_ASSERT (NM_STRLEN (IPV6_PROPERTY_DIR) + IFNAMSIZ + 60 == NM_UTILS_IP_PROPERTY_PATH_BUFSIZE); static const char * -_get_property_path (const char *ifname, +_get_property_path (char *buf, + const char *ifname, const char *property, gboolean ipv6) { - static char path[sizeof (IPV6_PROPERTY_DIR) + IFNAMSIZ + 32]; int len; + nm_assert (buf); + ifname = NM_ASSERT_VALID_PATH_COMPONENT (ifname); property = NM_ASSERT_VALID_PATH_COMPONENT (property); - len = g_snprintf (path, - sizeof (path), + len = g_snprintf (buf, + NM_UTILS_IP_PROPERTY_PATH_BUFSIZE, "%s%s/%s", ipv6 ? IPV6_PROPERTY_DIR : IPV4_PROPERTY_DIR, ifname, property); - g_assert (len < sizeof (path) - 1); - - return path; + g_assert (len < NM_UTILS_IP_PROPERTY_PATH_BUFSIZE - 1); + return buf; } /** * nm_utils_ip6_property_path: + * @buf: the output buffer where to write the path. It + * must be at least NM_UTILS_IP_PROPERTY_PATH_BUFSIZE bytes + * long. * @ifname: an interface name * @property: a property name * - * Returns the path to IPv6 property @property on @ifname. Note that - * this uses a static buffer. + * Returns: the path to IPv6 property @property on @ifname. Note that + * this returns the input argument @buf. */ const char * -nm_utils_ip6_property_path (const char *ifname, const char *property) +nm_utils_ip6_property_path (char *buf, const char *ifname, const char *property) { - return _get_property_path (ifname, property, TRUE); + return _get_property_path (buf, ifname, property, TRUE); } /** * nm_utils_ip4_property_path: + * @buf: the output buffer where to write the path. It + * must be at least NM_UTILS_IP_PROPERTY_PATH_BUFSIZE bytes + * long. * @ifname: an interface name * @property: a property name * - * Returns the path to IPv4 property @property on @ifname. Note that - * this uses a static buffer. + * Returns: the path to IPv6 property @property on @ifname. Note that + * this returns the input argument @buf. */ const char * -nm_utils_ip4_property_path (const char *ifname, const char *property) +nm_utils_ip4_property_path (char *buf, const char *ifname, const char *property) { - return _get_property_path (ifname, property, FALSE); + return _get_property_path (buf, ifname, property, FALSE); } gboolean diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 288319988..e08903614 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -248,8 +248,11 @@ gint64 nm_utils_monotonic_timestamp_as_boottime (gint64 timestamp, gint64 timest gboolean nm_utils_is_valid_path_component (const char *name); const char *NM_ASSERT_VALID_PATH_COMPONENT (const char *name); -const char *nm_utils_ip6_property_path (const char *ifname, const char *property); -const char *nm_utils_ip4_property_path (const char *ifname, const char *property); + +#define NM_UTILS_IP_PROPERTY_PATH_BUFSIZE 100 + +const char *nm_utils_ip6_property_path (char *buf, const char *ifname, const char *property); +const char *nm_utils_ip4_property_path (char *buf, const char *ifname, const char *property); gboolean nm_utils_is_specific_hostname (const char *name); diff --git a/src/nm-iface-helper.c b/src/nm-iface-helper.c index 38a8a8afe..658665d1d 100644 --- a/src/nm-iface-helper.c +++ b/src/nm-iface-helper.c @@ -222,9 +222,10 @@ ndisc_config_changed (NMNDisc *ndisc, const NMNDiscData *rdata, guint changed_in if (changed & NM_NDISC_CONFIG_MTU) { char val[16]; + char sysctl_path_buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; g_snprintf (val, sizeof (val), "%d", rdata->mtu); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (global_opt.ifname, "mtu")), val); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "mtu")), val); } nm_ip6_config_merge (existing, ndisc_config, NM_IP_CONFIG_MERGE_DEFAULT, 0); @@ -344,6 +345,7 @@ main (int argc, char *argv[]) gconstpointer tmp; gs_free NMUtilsIPv6IfaceId *iid = NULL; guint sd_id; + char sysctl_path_buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; nm_g_type_init (); @@ -448,7 +450,7 @@ main (int argc, char *argv[]) } if (global_opt.dhcp4_address) { - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (global_opt.ifname, "promote_secondaries")), "1"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (sysctl_path_buf, global_opt.ifname, "promote_secondaries")), "1"); dhcp4_client = nm_dhcp_manager_start_ip4 (nm_dhcp_manager_get (), nm_platform_get_multi_idx (NM_PLATFORM_GET), @@ -497,10 +499,10 @@ main (int argc, char *argv[]) if (iid) nm_ndisc_set_iid (ndisc, *iid); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (global_opt.ifname, "accept_ra")), "1"); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (global_opt.ifname, "accept_ra_defrtr")), "0"); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (global_opt.ifname, "accept_ra_pinfo")), "0"); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (global_opt.ifname, "accept_ra_rtr_pref")), "0"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "accept_ra")), "1"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "accept_ra_defrtr")), "0"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "accept_ra_pinfo")), "0"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "accept_ra_rtr_pref")), "0"); g_signal_connect (NM_PLATFORM_GET, NM_PLATFORM_SIGNAL_IP6_ADDRESS_CHANGED, diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 90ea2fc72..e1625d16f 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -414,6 +414,7 @@ nm_platform_sysctl_set_ip6_hop_limit_safe (NMPlatform *self, const char *iface, { const char *path; gint64 cur; + char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; _CHECK_SELF (self, klass, FALSE); @@ -425,7 +426,7 @@ nm_platform_sysctl_set_ip6_hop_limit_safe (NMPlatform *self, const char *iface, if (value < 10) return FALSE; - path = nm_utils_ip6_property_path (iface, "hop_limit"); + path = nm_utils_ip6_property_path (buf, iface, "hop_limit"); cur = nm_platform_sysctl_get_int_checked (self, NMP_SYSCTL_PATHID_ABSOLUTE (path), 10, 1, G_MAXINT32, -1); /* only allow increasing the hop-limit to avoid DOS by an attacker From 32b3eb11814e522e18800d0d402d84d170a6f456 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 13:55:54 +0200 Subject: [PATCH 11/16] core: merge IPv4 and IPv6 implementation of nm_utils_ip4_property_path() and nm_utils_ip6_property_path(). Also, rename to nm_utils_sysctl_ip_conf_path(). --- src/devices/nm-device.c | 26 ++++++++-------- src/ndisc/nm-lndp-ndisc.c | 4 +-- src/nm-core-utils.c | 62 ++++++++++++-------------------------- src/nm-core-utils.h | 5 ++- src/nm-iface-helper.c | 16 +++++----- src/platform/nm-platform.c | 4 +-- 6 files changed, 46 insertions(+), 71 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index cceda7ae3..18789ba88 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -852,29 +852,29 @@ nm_device_ipv4_sysctl_set (NMDevice *self, const char *property, const char *val NMPlatform *platform = nm_device_get_platform (self); gs_free char *value_to_free = NULL; const char *value_to_set; - char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; if (value) { value_to_set = value; } else { /* Set to a default value when we've got a NULL @value. */ value_to_free = nm_platform_sysctl_get (platform, - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (buf, "default", property))); + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET, buf, "default", property))); value_to_set = value_to_free; } return nm_platform_sysctl_set (platform, - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (buf, nm_device_get_ip_iface (self), property)), + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET, buf, nm_device_get_ip_iface (self), property)), value_to_set); } static guint32 nm_device_ipv4_sysctl_get_uint32 (NMDevice *self, const char *property, guint32 fallback) { - char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; return nm_platform_sysctl_get_int_checked (nm_device_get_platform (self), - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (buf, nm_device_get_ip_iface (self), property)), + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET, buf, nm_device_get_ip_iface (self), property)), 10, 0, G_MAXUINT32, @@ -884,18 +884,18 @@ nm_device_ipv4_sysctl_get_uint32 (NMDevice *self, const char *property, guint32 gboolean nm_device_ipv6_sysctl_set (NMDevice *self, const char *property, const char *value) { - char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; - return nm_platform_sysctl_set (nm_device_get_platform (self), NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, nm_device_get_ip_iface (self), property)), value); + return nm_platform_sysctl_set (nm_device_get_platform (self), NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, buf, nm_device_get_ip_iface (self), property)), value); } static guint32 nm_device_ipv6_sysctl_get_uint32 (NMDevice *self, const char *property, guint32 fallback) { - char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; return nm_platform_sysctl_get_int_checked (nm_device_get_platform (self), - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, nm_device_get_ip_iface (self), property)), + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, buf, nm_device_get_ip_iface (self), property)), 10, 0, G_MAXUINT32, @@ -7685,9 +7685,9 @@ save_ip6_properties (NMDevice *self) g_hash_table_remove_all (priv->ip6_saved_properties); for (i = 0; i < G_N_ELEMENTS (ip6_properties_to_save); i++) { - char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; - value = nm_platform_sysctl_get (nm_device_get_platform (self), NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, ifname, ip6_properties_to_save[i]))); + value = nm_platform_sysctl_get (nm_device_get_platform (self), NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, buf, ifname, ip6_properties_to_save[i]))); if (value) { g_hash_table_insert (priv->ip6_saved_properties, (char *) ip6_properties_to_save[i], @@ -7747,11 +7747,11 @@ set_nm_ipv6ll (NMDevice *self, gboolean enable) } if (enable) { - char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; /* Bounce IPv6 to ensure the kernel stops IPv6LL address generation */ value = nm_platform_sysctl_get (nm_device_get_platform (self), - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, nm_device_get_ip_iface (self), "disable_ipv6"))); + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, buf, nm_device_get_ip_iface (self), "disable_ipv6"))); if (g_strcmp0 (value, "0") == 0) nm_device_ipv6_sysctl_set (self, "disable_ipv6", "1"); g_free (value); diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index e155402e1..70200ed34 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -531,10 +531,10 @@ start (NMNDisc *ndisc) static inline int ipv6_sysctl_get (NMPlatform *platform, const char *ifname, const char *property, int min, int max, int defval) { - char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; return (int) nm_platform_sysctl_get_int_checked (platform, - NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (buf, ifname, property)), + NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, buf, ifname, property)), 10, min, max, diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 2325283e6..bf8d50e18 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -2525,65 +2525,41 @@ nm_utils_monotonic_timestamp_as_boottime (gint64 timestamp, gint64 timestamp_ns_ #define IPV6_PROPERTY_DIR "/proc/sys/net/ipv6/conf/" #define IPV4_PROPERTY_DIR "/proc/sys/net/ipv4/conf/" G_STATIC_ASSERT (sizeof (IPV4_PROPERTY_DIR) == sizeof (IPV6_PROPERTY_DIR)); -G_STATIC_ASSERT (NM_STRLEN (IPV6_PROPERTY_DIR) + IFNAMSIZ + 60 == NM_UTILS_IP_PROPERTY_PATH_BUFSIZE); +G_STATIC_ASSERT (NM_STRLEN (IPV6_PROPERTY_DIR) + IFNAMSIZ + 60 == NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE); -static const char * -_get_property_path (char *buf, - const char *ifname, - const char *property, - gboolean ipv6) +/** + * nm_utils_sysctl_ip_conf_path: + * @addr_family: either AF_INET or AF_INET6. + * @buf: the output buffer where to write the path. It + * must be at least NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE bytes + * long. + * @ifname: an interface name + * @property: a property name + * + * Returns: the path to IPv6 property @property on @ifname. Note that + * this returns the input argument @buf. + */ +const char * +nm_utils_sysctl_ip_conf_path (int addr_family, char *buf, const char *ifname, const char *property) { int len; nm_assert (buf); + nm_assert_addr_family (addr_family); ifname = NM_ASSERT_VALID_PATH_COMPONENT (ifname); property = NM_ASSERT_VALID_PATH_COMPONENT (property); len = g_snprintf (buf, - NM_UTILS_IP_PROPERTY_PATH_BUFSIZE, + NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE, "%s%s/%s", - ipv6 ? IPV6_PROPERTY_DIR : IPV4_PROPERTY_DIR, + addr_family == AF_INET6 ? IPV6_PROPERTY_DIR : IPV4_PROPERTY_DIR, ifname, property); - g_assert (len < NM_UTILS_IP_PROPERTY_PATH_BUFSIZE - 1); + g_assert (len < NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE - 1); return buf; } -/** - * nm_utils_ip6_property_path: - * @buf: the output buffer where to write the path. It - * must be at least NM_UTILS_IP_PROPERTY_PATH_BUFSIZE bytes - * long. - * @ifname: an interface name - * @property: a property name - * - * Returns: the path to IPv6 property @property on @ifname. Note that - * this returns the input argument @buf. - */ -const char * -nm_utils_ip6_property_path (char *buf, const char *ifname, const char *property) -{ - return _get_property_path (buf, ifname, property, TRUE); -} - -/** - * nm_utils_ip4_property_path: - * @buf: the output buffer where to write the path. It - * must be at least NM_UTILS_IP_PROPERTY_PATH_BUFSIZE bytes - * long. - * @ifname: an interface name - * @property: a property name - * - * Returns: the path to IPv6 property @property on @ifname. Note that - * this returns the input argument @buf. - */ -const char * -nm_utils_ip4_property_path (char *buf, const char *ifname, const char *property) -{ - return _get_property_path (buf, ifname, property, FALSE); -} - gboolean nm_utils_is_valid_path_component (const char *name) { diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index e08903614..3b3944e93 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -249,10 +249,9 @@ gint64 nm_utils_monotonic_timestamp_as_boottime (gint64 timestamp, gint64 timest gboolean nm_utils_is_valid_path_component (const char *name); const char *NM_ASSERT_VALID_PATH_COMPONENT (const char *name); -#define NM_UTILS_IP_PROPERTY_PATH_BUFSIZE 100 +#define NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE 100 -const char *nm_utils_ip6_property_path (char *buf, const char *ifname, const char *property); -const char *nm_utils_ip4_property_path (char *buf, const char *ifname, const char *property); +const char *nm_utils_sysctl_ip_conf_path (int addr_family, char *buf, const char *ifname, const char *property); gboolean nm_utils_is_specific_hostname (const char *name); diff --git a/src/nm-iface-helper.c b/src/nm-iface-helper.c index 658665d1d..5a68713a5 100644 --- a/src/nm-iface-helper.c +++ b/src/nm-iface-helper.c @@ -222,10 +222,10 @@ ndisc_config_changed (NMNDisc *ndisc, const NMNDiscData *rdata, guint changed_in if (changed & NM_NDISC_CONFIG_MTU) { char val[16]; - char sysctl_path_buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char sysctl_path_buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; g_snprintf (val, sizeof (val), "%d", rdata->mtu); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "mtu")), val); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, sysctl_path_buf, global_opt.ifname, "mtu")), val); } nm_ip6_config_merge (existing, ndisc_config, NM_IP_CONFIG_MERGE_DEFAULT, 0); @@ -345,7 +345,7 @@ main (int argc, char *argv[]) gconstpointer tmp; gs_free NMUtilsIPv6IfaceId *iid = NULL; guint sd_id; - char sysctl_path_buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char sysctl_path_buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; nm_g_type_init (); @@ -450,7 +450,7 @@ main (int argc, char *argv[]) } if (global_opt.dhcp4_address) { - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip4_property_path (sysctl_path_buf, global_opt.ifname, "promote_secondaries")), "1"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET, sysctl_path_buf, global_opt.ifname, "promote_secondaries")), "1"); dhcp4_client = nm_dhcp_manager_start_ip4 (nm_dhcp_manager_get (), nm_platform_get_multi_idx (NM_PLATFORM_GET), @@ -499,10 +499,10 @@ main (int argc, char *argv[]) if (iid) nm_ndisc_set_iid (ndisc, *iid); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "accept_ra")), "1"); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "accept_ra_defrtr")), "0"); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "accept_ra_pinfo")), "0"); - nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_ip6_property_path (sysctl_path_buf, global_opt.ifname, "accept_ra_rtr_pref")), "0"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, sysctl_path_buf, global_opt.ifname, "accept_ra")), "1"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, sysctl_path_buf, global_opt.ifname, "accept_ra_defrtr")), "0"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, sysctl_path_buf, global_opt.ifname, "accept_ra_pinfo")), "0"); + nm_platform_sysctl_set (NM_PLATFORM_GET, NMP_SYSCTL_PATHID_ABSOLUTE (nm_utils_sysctl_ip_conf_path (AF_INET6, sysctl_path_buf, global_opt.ifname, "accept_ra_rtr_pref")), "0"); g_signal_connect (NM_PLATFORM_GET, NM_PLATFORM_SIGNAL_IP6_ADDRESS_CHANGED, diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index e1625d16f..1697bc03e 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -414,7 +414,7 @@ nm_platform_sysctl_set_ip6_hop_limit_safe (NMPlatform *self, const char *iface, { const char *path; gint64 cur; - char buf[NM_UTILS_IP_PROPERTY_PATH_BUFSIZE]; + char buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; _CHECK_SELF (self, klass, FALSE); @@ -426,7 +426,7 @@ nm_platform_sysctl_set_ip6_hop_limit_safe (NMPlatform *self, const char *iface, if (value < 10) return FALSE; - path = nm_utils_ip6_property_path (buf, iface, "hop_limit"); + path = nm_utils_sysctl_ip_conf_path (AF_INET6, buf, iface, "hop_limit"); cur = nm_platform_sysctl_get_int_checked (self, NMP_SYSCTL_PATHID_ABSOLUTE (path), 10, 1, G_MAXINT32, -1); /* only allow increasing the hop-limit to avoid DOS by an attacker From cd271d5cb1b4d1e87ecec36956531431c4f3cb3f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 14:24:28 +0200 Subject: [PATCH 12/16] core: add nm_utils_sysctl_ip_conf_is_path() util --- src/nm-core-utils.c | 51 +++++++++++++++++++++++++++++++++++++++- src/nm-core-utils.h | 2 ++ src/tests/test-general.c | 28 ++++++++++++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index bf8d50e18..4239c8982 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -2547,7 +2547,7 @@ nm_utils_sysctl_ip_conf_path (int addr_family, char *buf, const char *ifname, co nm_assert (buf); nm_assert_addr_family (addr_family); - ifname = NM_ASSERT_VALID_PATH_COMPONENT (ifname); + g_assert (nm_utils_is_valid_iface_name (ifname, NULL)); property = NM_ASSERT_VALID_PATH_COMPONENT (property); len = g_snprintf (buf, @@ -2560,6 +2560,55 @@ nm_utils_sysctl_ip_conf_path (int addr_family, char *buf, const char *ifname, co return buf; } +gboolean +nm_utils_sysctl_ip_conf_is_path (int addr_family, const char *path, const char *ifname, const char *property) +{ + g_return_val_if_fail (path, FALSE); + NM_ASSERT_VALID_PATH_COMPONENT (property); + g_assert (!ifname || nm_utils_is_valid_iface_name (ifname, NULL)); + + if (addr_family == AF_INET) { + if (!g_str_has_prefix (path, IPV4_PROPERTY_DIR)) + return FALSE; + path += NM_STRLEN (IPV4_PROPERTY_DIR); + } else if (addr_family == AF_INET6) { + if (!g_str_has_prefix (path, IPV6_PROPERTY_DIR)) + return FALSE; + path += NM_STRLEN (IPV6_PROPERTY_DIR); + } else + g_return_val_if_reached (FALSE); + + if (ifname) { + if (!g_str_has_prefix (path, ifname)) + return FALSE; + path += strlen (ifname); + if (path[0] != '/') + return FALSE; + path++; + } else { + const char *slash; + char buf[IFNAMSIZ]; + gsize l; + + slash = strchr (path, '/'); + if (!slash) + return FALSE; + l = slash - path; + if (l >= IFNAMSIZ) + return FALSE; + memcpy (buf, path, l); + buf[l] = '\0'; + if (!nm_utils_is_valid_iface_name (buf, NULL)) + return FALSE; + path = slash + 1; + } + + if (!nm_streq (path, property)) + return FALSE; + + return TRUE; +} + gboolean nm_utils_is_valid_path_component (const char *name) { diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 3b3944e93..cc7847244 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -253,6 +253,8 @@ const char *NM_ASSERT_VALID_PATH_COMPONENT (const char *name); const char *nm_utils_sysctl_ip_conf_path (int addr_family, char *buf, const char *ifname, const char *property); +gboolean nm_utils_sysctl_ip_conf_is_path (int addr_family, const char *path, const char *ifname, const char *property); + gboolean nm_utils_is_specific_hostname (const char *name); int nm_utils_fd_get_contents (int fd, diff --git a/src/tests/test-general.c b/src/tests/test-general.c index 37bbc5ca9..d36a26ef4 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -272,6 +272,32 @@ test_nm_utils_log_connection_diff (void) /*****************************************************************************/ +static void +do_test_sysctl_ip_conf (int addr_family, + const char *iface, + const char *property) +{ + char path[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE]; + const char *pp; + + pp = nm_utils_sysctl_ip_conf_path (addr_family, path, iface, property); + g_assert (pp == path); + g_assert (path[0] == '/'); + + g_assert (nm_utils_sysctl_ip_conf_is_path (addr_family, path, iface, property)); + g_assert (nm_utils_sysctl_ip_conf_is_path (addr_family, path, NULL, property)); +} + +static void +test_nm_utils_sysctl_ip_conf_path (void) +{ + do_test_sysctl_ip_conf (AF_INET6, "a", "mtu"); + do_test_sysctl_ip_conf (AF_INET6, "eth0", "mtu"); + do_test_sysctl_ip_conf (AF_INET6, "e23456789012345", "mtu"); +} + +/*****************************************************************************/ + static NMConnection * _match_connection_new (void) { @@ -1716,6 +1742,8 @@ main (int argc, char **argv) g_test_add_func ("/general/nm_utils_ip6_address_same_prefix", test_nm_utils_ip6_address_same_prefix); g_test_add_func ("/general/nm_utils_log_connection_diff", test_nm_utils_log_connection_diff); + g_test_add_func ("/general/nm_utils_sysctl_ip_conf_path", test_nm_utils_sysctl_ip_conf_path); + g_test_add_func ("/general/exp10", test_nm_utils_exp10); g_test_add_func ("/general/connection-match/basic", test_connection_match_basic); From a53f45c15e65e8f6e1e61e37a710d240ad123753 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 14:27:08 +0200 Subject: [PATCH 13/16] platform: suppress logging error on failure to set MTU --- src/platform/nm-linux-platform.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 7b2ac003e..6071826ab 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -3027,8 +3027,17 @@ sysctl_set (NMPlatform *platform, const char *pathid, int dirfd, const char *pat } } if (nwrote == -1 && errsv != EEXIST) { - _LOGE ("sysctl: failed to set '%s' to '%s': (%d) %s", - path, value, errsv, strerror (errsv)); + NMLogLevel level = LOGL_ERR; + + if ( errsv == EINVAL + && nm_utils_sysctl_ip_conf_is_path (AF_INET6, path, NULL, "mtu")) { + /* setting the MTU can fail under regular conditions. Suppress + * logging a warning. */ + level = LOGL_DEBUG; + } + + _NMLOG (level, "sysctl: failed to set '%s' to '%s': (%d) %s", + path, value, errsv, strerror (errsv)); } else if (nwrote < len - 1) { _LOGE ("sysctl: failed to set '%s' to '%s' after three attempts", path, value); From 54cbb321e59fe3ed73bb2f8eb814c1944025c31a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 15:16:50 +0200 Subject: [PATCH 14/16] platform: return platform error code from nm_platform_link_set_mtu() --- src/platform/nm-fake-platform.c | 6 +++--- src/platform/nm-linux-platform.c | 4 ++-- src/platform/nm-platform.c | 2 +- src/platform/nm-platform.h | 4 ++-- src/platform/tests/test-link.c | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/platform/nm-fake-platform.c b/src/platform/nm-fake-platform.c index de02ebe09..c199c5ed3 100644 --- a/src/platform/nm-fake-platform.c +++ b/src/platform/nm-fake-platform.c @@ -585,7 +585,7 @@ link_set_address (NMPlatform *platform, int ifindex, gconstpointer addr, size_t return NM_PLATFORM_ERROR_SUCCESS; } -static gboolean +static NMPlatformError link_set_mtu (NMPlatform *platform, int ifindex, guint32 mtu) { NMFakePlatformLink *device = link_get (platform, ifindex); @@ -593,13 +593,13 @@ link_set_mtu (NMPlatform *platform, int ifindex, guint32 mtu) if (!device) { _LOGE ("failure changing link: netlink error (No such device)"); - return FALSE; + return NM_PLATFORM_ERROR_EXISTS; } obj_tmp = nmp_object_clone (device->obj, FALSE); obj_tmp->link.mtu = mtu; link_set_obj (platform, device, obj_tmp); - return TRUE; + return NM_PLATFORM_ERROR_SUCCESS; } static gboolean diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 6071826ab..cc8be1ad7 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -4859,7 +4859,7 @@ link_get_permanent_address (NMPlatform *platform, return nmp_utils_ethtool_get_permanent_address (ifindex, buf, length); } -static gboolean +static NMPlatformError link_set_mtu (NMPlatform *platform, int ifindex, guint32 mtu) { nm_auto_nlmsg struct nl_msg *nlmsg = NULL; @@ -4877,7 +4877,7 @@ link_set_mtu (NMPlatform *platform, int ifindex, guint32 mtu) NLA_PUT_U32 (nlmsg, IFLA_MTU, mtu); - return do_change_link (platform, CHANGE_LINK_TYPE_SET_MTU, ifindex, nlmsg, NULL) == NM_PLATFORM_ERROR_SUCCESS; + return do_change_link (platform, CHANGE_LINK_TYPE_SET_MTU, ifindex, nlmsg, NULL); nla_put_failure: g_return_val_if_reached (FALSE); } diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 1697bc03e..ffc4b3956 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -1500,7 +1500,7 @@ nm_platform_link_set_noarp (NMPlatform *self, int ifindex) * * Set interface MTU. */ -gboolean +NMPlatformError nm_platform_link_set_mtu (NMPlatform *self, int ifindex, guint32 mtu) { _CHECK_SELF (self, klass, FALSE); diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 0d40ee129..d155c1092 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -719,7 +719,7 @@ typedef struct { guint8 *buf, size_t *length); NMPlatformError (*link_set_address) (NMPlatform *, int ifindex, gconstpointer address, size_t length); - gboolean (*link_set_mtu) (NMPlatform *, int ifindex, guint32 mtu); + NMPlatformError (*link_set_mtu) (NMPlatform *, int ifindex, guint32 mtu); gboolean (*link_set_name) (NMPlatform *, int ifindex, const char *name); gboolean (*link_set_sriov_num_vfs) (NMPlatform *, int ifindex, guint num_vfs); @@ -1061,7 +1061,7 @@ gboolean nm_platform_link_set_ipv6_token (NMPlatform *self, int ifindex, NMUtils gboolean nm_platform_link_get_permanent_address (NMPlatform *self, int ifindex, guint8 *buf, size_t *length); NMPlatformError nm_platform_link_set_address (NMPlatform *self, int ifindex, const void *address, size_t length); -gboolean nm_platform_link_set_mtu (NMPlatform *self, int ifindex, guint32 mtu); +NMPlatformError nm_platform_link_set_mtu (NMPlatform *self, int ifindex, guint32 mtu); gboolean nm_platform_link_set_name (NMPlatform *self, int ifindex, const char *name); gboolean nm_platform_link_set_sriov_num_vfs (NMPlatform *self, int ifindex, guint num_vfs); diff --git a/src/platform/tests/test-link.c b/src/platform/tests/test-link.c index ccbada32d..0a4ab1a92 100644 --- a/src/platform/tests/test-link.c +++ b/src/platform/tests/test-link.c @@ -76,7 +76,7 @@ test_bogus(void) g_assert (!addrlen); g_assert (!nm_platform_link_get_address (NM_PLATFORM_GET, BOGUS_IFINDEX, NULL)); - g_assert (!nm_platform_link_set_mtu (NM_PLATFORM_GET, BOGUS_IFINDEX, MTU)); + g_assert (nm_platform_link_set_mtu (NM_PLATFORM_GET, BOGUS_IFINDEX, MTU) != NM_PLATFORM_ERROR_SUCCESS); g_assert (!nm_platform_link_get_mtu (NM_PLATFORM_GET, BOGUS_IFINDEX)); @@ -603,7 +603,7 @@ test_internal (void) accept_signal (link_changed); /* Set MTU */ - g_assert (nm_platform_link_set_mtu (NM_PLATFORM_GET, ifindex, MTU)); + g_assert (nm_platform_link_set_mtu (NM_PLATFORM_GET, ifindex, MTU) == NM_PLATFORM_ERROR_SUCCESS); g_assert_cmpint (nm_platform_link_get_mtu (NM_PLATFORM_GET, ifindex), ==, MTU); accept_signal (link_changed); From 8a6c4fca3df71a047b8e784412a715164df7dc2a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 17:36:20 +0200 Subject: [PATCH 15/16] platform: log result also for EEXIST in sysctl_set() --- src/platform/nm-linux-platform.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index cc8be1ad7..79a0eb0b6 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -3026,11 +3026,13 @@ sysctl_set (NMPlatform *platform, const char *pathid, int dirfd, const char *pat break; } } - if (nwrote == -1 && errsv != EEXIST) { + if (nwrote == -1) { NMLogLevel level = LOGL_ERR; - if ( errsv == EINVAL - && nm_utils_sysctl_ip_conf_is_path (AF_INET6, path, NULL, "mtu")) { + if (errsv == EEXIST) { + level = LOGL_DEBUG; + } else if ( errsv == EINVAL + && nm_utils_sysctl_ip_conf_is_path (AF_INET6, path, NULL, "mtu")) { /* setting the MTU can fail under regular conditions. Suppress * logging a warning. */ level = LOGL_DEBUG; From d732ac7d31b07c9cc2969f17624ca3ba8f25cf0c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Oct 2017 15:41:07 +0200 Subject: [PATCH 16/16] device: show better logging message when setting MTU fails Setting the MTU might fail when the underlying device's MTU is not set. Detect that case, and log a better warning message. Unfortunately, it's tricky to detect whether this is a complete failure, or whether we will later try again to change the MTU. So, we log a failure, altough later we might fix it. It would be better not to warn about non-errors. --- src/devices/nm-device.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 18789ba88..3060c6095 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -72,6 +72,7 @@ #include "nm-arping-manager.h" #include "nm-connectivity.h" #include "nm-dbus-interface.h" +#include "nm-device-vlan.h" #include "nm-device-logging.h" _LOG_DECLARE_SELF (NMDevice); @@ -7346,6 +7347,7 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config) }) if ( (mtu_desired && mtu_desired != mtu_plat) || (ip6_mtu && ip6_mtu != _IP6_MTU_SYS ())) { + gboolean anticipated_failure = FALSE; if (!priv->mtu_initial && !priv->ip6_mtu_initial) { /* before touching any of the MTU paramters, record the @@ -7355,13 +7357,30 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config) } if (mtu_desired && mtu_desired != mtu_plat) { - nm_platform_link_set_mtu (nm_device_get_platform (self), ifindex, mtu_desired); + if (nm_platform_link_set_mtu (nm_device_get_platform (self), ifindex, mtu_desired) == NM_PLATFORM_ERROR_CANT_SET_MTU) { + anticipated_failure = TRUE; + _LOGW (LOGD_DEVICE, "mtu: failure to set MTU. %s", + NM_IS_DEVICE_VLAN (self) + ? "Is the parent's MTU size large enough?" + : (!c_list_is_empty (&priv->slaves) + ? "Are the MTU sizes of the slaves large enough?" + : "Did you configure the MTU correctly?")); + } priv->carrier_wait_until_ms = nm_utils_get_monotonic_timestamp_ms () + CARRIER_WAIT_TIME_AFTER_MTU_MS; } if (ip6_mtu && ip6_mtu != _IP6_MTU_SYS ()) { - nm_device_ipv6_sysctl_set (self, "mtu", - nm_sprintf_buf (sbuf, "%u", (unsigned) ip6_mtu)); + if (!nm_device_ipv6_sysctl_set (self, "mtu", + nm_sprintf_buf (sbuf, "%u", (unsigned) ip6_mtu))) { + int errsv = errno; + + _NMLOG (anticipated_failure && errsv == EINVAL ? LOGL_DEBUG : LOGL_WARN, + LOGD_DEVICE, + "mtu: failure to set IPv6 MTU%s", + anticipated_failure && errsv == EINVAL + ? ": Is the underlying MTU value successfully set?" + : ""); + } priv->carrier_wait_until_ms = nm_utils_get_monotonic_timestamp_ms () + CARRIER_WAIT_TIME_AFTER_MTU_MS; } }