platform/ethtool: add SocketHandle to reuse socket for ethtool requests

Previously, each call to ethtool_get() would resolve the ifindex and create a new socket for the ethtool request. This is partly done, because ethtool only supports making requests by name. Since interfaces can be renamed, this is inherrently racy. So, we want to fetch the latest name shortly before making the request. Some functions like nmp_utils_ethtool_supports_vlans() require multiple ioctls. And next, we will introduce more ethtool functions, that make an even larger number of individual requests. Add a simple SocketHandle struct, to create the socket once and reuse it for multiple requests. This is still entirely internal API in "nm-platform-utils.c".
2018-07-16 13:08:38 +02:00
parent 29266e0086
commit bdd9f7482c
1 changed files with 133 additions and 85 deletions
--- a/src/platform/nm-platform-utils.c
+++ b/src/platform/nm-platform-utils.c
@@ -63,6 +63,42 @@ nmp_utils_if_nametoindex (const char *ifname)
 	return if_nametoindex (ifname);
 }

+/*****************************************************************************/
+
+typedef struct {
+	int fd;
+	int ifindex;
+	char ifname[IFNAMSIZ];
+} SocketHandle;
+
+static int
+socket_handle_init (SocketHandle *shandle, int ifindex)
+{
+	if (!nmp_utils_if_indextoname (ifindex, shandle->ifname)) {
+		shandle->ifindex = 0;
+		return -ENODEV;
+	}
+
+	shandle->fd = socket (PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+	if (shandle->fd < 0) {
+		shandle->ifindex = 0;
+		return -errno;
+	}
+
+	shandle->ifindex = ifindex;
+	return 0;
+}
+
+static void
+socket_handle_destroy (SocketHandle *shandle)
+{
+	if (shandle->ifindex) {
+		shandle->ifindex = 0;
+		nm_close (shandle->fd);
+	}
+}
+#define nm_auto_socket_handle nm_auto(socket_handle_destroy)
+
 /******************************************************************************
 * ethtool
 *****************************************************************************/
@@ -87,6 +123,8 @@ _ethtool_data_to_string (gconstpointer edata, char *buf, gsize len)
 	return _ethtool_cmd_to_string (*((guint32 *) edata), buf, len);
 }

+/*****************************************************************************/
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,27)
 #define ethtool_cmd_speed(pedata) ((pedata)->speed)

@@ -94,73 +132,63 @@ _ethtool_data_to_string (gconstpointer edata, char *buf, gsize len)
 	G_STMT_START { (pedata)->speed = (guint16) (speed); } G_STMT_END
 #endif

-static gboolean
-ethtool_get (int ifindex, gpointer edata)
+static int
+ethtool_call_handle (SocketHandle *shandle, gpointer edata)
 {
-	char ifname[IFNAMSIZ];
-	char sbuf[50];
-
-	nm_assert (ifindex > 0);
-
-	/* ethtool ioctl API uses the ifname to refer to an interface. That is racy
-	 * as interfaces can be renamed *sigh*.
-	 *
-	 * Note that we anyway have to verify whether the interface exists, before
-	 * calling ioctl for a non-existing ifname. This is to prevent autoloading
-	 * of kernel modules *sigh*.
-	 * Thus, as we anyway verify the existence of ifname before doing the call,
-	 * go one step further and lookup the ifname everytime anew.
-	 *
-	 * This does not solve the renaming race, but it minimizes the time for
-	 * the race to happen as much as possible. */
-
-	if (!nmp_utils_if_indextoname (ifindex, ifname)) {
-		nm_log_trace (LOGD_PLATFORM, "ethtool[%d]: %s: request fails resolving ifindex: %s",
-		              ifindex,
-		              _ethtool_data_to_string (edata, sbuf, sizeof (sbuf)),
-		              g_strerror (errno));
-		return FALSE;
-	}
-
-	{
-		nm_auto_close int fd = -1;
 	struct ifreq ifr = {
 		.ifr_data = edata,
 	};
+	char sbuf[50];
+	int errsv;

-		memcpy (ifr.ifr_name, ifname, sizeof (ifname));
+	nm_assert (shandle);
+	nm_assert (shandle->ifindex);
+	nm_assert (shandle->ifname[0]);
+	nm_assert (strlen (shandle->ifname) < IFNAMSIZ);
+	nm_assert (edata);

-		fd = socket (PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
-		if (fd < 0) {
-			nm_log_trace (LOGD_PLATFORM, "ethtool[%d]: %s, %s: failed creating socket for ioctl: %s",
-			              ifindex,
-			              _ethtool_data_to_string (edata, sbuf, sizeof (sbuf)),
-			              ifname,
-			              g_strerror (errno));
-			return FALSE;
-		}
-
-		if (ioctl (fd, SIOCETHTOOL, &ifr) < 0) {
+	memcpy (ifr.ifr_name, shandle->ifname, IFNAMSIZ);
+	if (ioctl (shandle->fd, SIOCETHTOOL, &ifr) < 0) {
+		errsv = errno;
 		nm_log_trace (LOGD_PLATFORM, "ethtool[%d]: %s, %s: failed: %s",
-			              ifindex,
+		              shandle->ifindex,
 		              _ethtool_data_to_string (edata, sbuf, sizeof (sbuf)),
-			              ifname,
-			              strerror (errno));
-			return FALSE;
+		              shandle->ifname,
+		              strerror (errsv));
+		return -errsv;
 	}

 	nm_log_trace (LOGD_PLATFORM, "ethtool[%d]: %s, %s: success",
+	              shandle->ifindex,
+	              _ethtool_data_to_string (edata, sbuf, sizeof (sbuf)),
+	              shandle->ifname);
+	return 0;
+}
+
+static int
+ethtool_call_ifindex (int ifindex, gpointer edata)
+{
+	nm_auto_socket_handle SocketHandle shandle = { };
+	int r;
+	char sbuf[50];
+
+	nm_assert (edata);
+
+	if ((r = socket_handle_init (&shandle, ifindex)) < 0) {
+		nm_log_trace (LOGD_PLATFORM, "ethtool[%d]: %s: failed creating ethtool socket: %s",
 		              ifindex,
 		              _ethtool_data_to_string (edata, sbuf, sizeof (sbuf)),
-		              ifname);
-		return TRUE;
+		              g_strerror (-r));
+		return r;
 	}
+
+	return ethtool_call_handle (&shandle, edata);
 }

 /*****************************************************************************/

 static struct ethtool_gstrings *
-ethtool_get_stringset (int ifindex, int stringset_id)
+ethtool_get_stringset (SocketHandle *shandle, int stringset_id)
 {
 	struct {
 		struct ethtool_sset_info info;
@@ -169,13 +197,11 @@ ethtool_get_stringset (int ifindex, int stringset_id)
 	gs_free struct ethtool_gstrings *gstrings = NULL;
 	guint32 i, len;

-	g_return_val_if_fail (ifindex > 0, NULL);
-
 	sset_info.info.cmd = ETHTOOL_GSSET_INFO;
 	sset_info.info.reserved = 0;
 	sset_info.info.sset_mask = (1ULL << stringset_id);

-	if (!ethtool_get (ifindex, &sset_info))
+	if (ethtool_call_handle (shandle, &sset_info) < 0)
 		return NULL;
 	if (!sset_info.info.sset_mask)
 		return NULL;
@@ -187,7 +213,7 @@ ethtool_get_stringset (int ifindex, int stringset_id)
 	gstrings->string_set = stringset_id;
 	gstrings->len = len;
 	if (gstrings->len > 0) {
-		if (!ethtool_get (ifindex, gstrings))
+		if (ethtool_call_handle (shandle, gstrings) < 0)
 			return NULL;
 		for (i = 0; i < gstrings->len; i++) {
 			/* ensure NUL terminated */
@@ -215,7 +241,7 @@ ethtool_gstrings_find (const struct ethtool_gstrings *gstrings, const char *need
 }

 static int
-ethtool_get_stringset_index (int ifindex, int stringset_id, const char *needle)
+ethtool_get_stringset_index (SocketHandle *shandle, int stringset_id, const char *needle)
 {
 	gs_free struct ethtool_gstrings *gstrings = NULL;

@@ -223,7 +249,7 @@ ethtool_get_stringset_index (int ifindex, int stringset_id, const char *needle)
 	 * that means, we cannot possibly request longer names. */
 	nm_assert (needle && strlen (needle) < ETH_GSTRING_LEN);

-	gstrings = ethtool_get_stringset (ifindex, stringset_id);
+	gstrings = ethtool_get_stringset (shandle, stringset_id);
 	if (gstrings)
 		return ethtool_gstrings_find (gstrings, needle);
 	return -1;
@@ -234,13 +260,14 @@ nmp_utils_ethtool_get_driver_info (int ifindex,
                                   NMPUtilsEthtoolDriverInfo *data)
 {
 	struct ethtool_drvinfo *drvinfo;
-	G_STATIC_ASSERT (sizeof (*data) == sizeof (*drvinfo));
-	G_STATIC_ASSERT (offsetof (NMPUtilsEthtoolDriverInfo, driver)     == offsetof (struct ethtool_drvinfo, driver));
-	G_STATIC_ASSERT (offsetof (NMPUtilsEthtoolDriverInfo, version)    == offsetof (struct ethtool_drvinfo, version));
-	G_STATIC_ASSERT (offsetof (NMPUtilsEthtoolDriverInfo, fw_version) == offsetof (struct ethtool_drvinfo, fw_version));
-	G_STATIC_ASSERT (sizeof (data->driver)     == sizeof (drvinfo->driver));
-	G_STATIC_ASSERT (sizeof (data->version)    == sizeof (drvinfo->version));
-	G_STATIC_ASSERT (sizeof (data->fw_version) == sizeof (drvinfo->fw_version));
+
+	G_STATIC_ASSERT_EXPR (sizeof (*data) == sizeof (*drvinfo));
+	G_STATIC_ASSERT_EXPR (offsetof (NMPUtilsEthtoolDriverInfo, driver)     == offsetof (struct ethtool_drvinfo, driver));
+	G_STATIC_ASSERT_EXPR (offsetof (NMPUtilsEthtoolDriverInfo, version)    == offsetof (struct ethtool_drvinfo, version));
+	G_STATIC_ASSERT_EXPR (offsetof (NMPUtilsEthtoolDriverInfo, fw_version) == offsetof (struct ethtool_drvinfo, fw_version));
+	G_STATIC_ASSERT_EXPR (sizeof (data->driver)     == sizeof (drvinfo->driver));
+	G_STATIC_ASSERT_EXPR (sizeof (data->version)    == sizeof (drvinfo->version));
+	G_STATIC_ASSERT_EXPR (sizeof (data->fw_version) == sizeof (drvinfo->fw_version));

 	g_return_val_if_fail (ifindex > 0, FALSE);
 	g_return_val_if_fail (data, FALSE);
@@ -249,7 +276,7 @@ nmp_utils_ethtool_get_driver_info (int ifindex,

 	memset (drvinfo, 0, sizeof (*drvinfo));
 	drvinfo->cmd = ETHTOOL_GDRVINFO;
-	return ethtool_get (ifindex, drvinfo);
+	return ethtool_call_ifindex (ifindex, drvinfo) >= 0;
 }

 gboolean
@@ -269,7 +296,7 @@ nmp_utils_ethtool_get_permanent_address (int ifindex,
 	edata.e.cmd = ETHTOOL_GPERMADDR;
 	edata.e.size = NM_UTILS_HWADDR_LEN_MAX;

-	if (!ethtool_get (ifindex, &edata.e))
+	if (ethtool_call_ifindex (ifindex, &edata.e) < 0)
 		return FALSE;

 	if (edata.e.size > NM_UTILS_HWADDR_LEN_MAX)
@@ -288,8 +315,8 @@ nmp_utils_ethtool_get_permanent_address (int ifindex,
 		}
 		return FALSE;
 	}
-not_all_0or1:

+not_all_0or1:
 	memcpy (buf, edata.e.data, edata.e.size);
 	*length = edata.e.size;
 	return TRUE;
@@ -306,20 +333,30 @@ nmp_utils_ethtool_supports_carrier_detect (int ifindex)
 	 * assume the device supports carrier-detect, otherwise we assume it
 	 * doesn't.
 	 */
-	return ethtool_get (ifindex, &edata);
+	return ethtool_call_ifindex (ifindex, &edata) >= 0;
 }

 gboolean
 nmp_utils_ethtool_supports_vlans (int ifindex)
 {
+	nm_auto_socket_handle SocketHandle shandle = { };
+	int r;
 	gs_free struct ethtool_gfeatures *features = NULL;
 	int idx, block, bit, size;

 	g_return_val_if_fail (ifindex > 0, FALSE);

-	idx = ethtool_get_stringset_index (ifindex, ETH_SS_FEATURES, "vlan-challenged");
+	if ((r = socket_handle_init (&shandle, ifindex)) < 0) {
+		nm_log_trace (LOGD_PLATFORM, "ethtool[%d]: %s: failed creating ethtool socket: %s",
+		              ifindex,
+		              "support-vlans",
+		              g_strerror (-r));
+		return FALSE;
+	}
+
+	idx = ethtool_get_stringset_index (&shandle, ETH_SS_FEATURES, "vlan-challenged");
 	if (idx < 0) {
-		nm_log_dbg (LOGD_PLATFORM, "ethtool: vlan-challenged ethtool feature does not exist for %d?", ifindex);
+		nm_log_dbg (LOGD_PLATFORM, "ethtool[%d]: vlan-challenged ethtool feature does not exist?", ifindex);
 		return FALSE;
 	}

@@ -331,7 +368,7 @@ nmp_utils_ethtool_supports_vlans (int ifindex)
 	features->cmd = ETHTOOL_GFEATURES;
 	features->size = size;

-	if (!ethtool_get (ifindex, features))
+	if (ethtool_call_handle (&shandle, features) < 0)
 		return FALSE;

 	return !(features->features[block].active & (1 << bit));
@@ -340,21 +377,32 @@ nmp_utils_ethtool_supports_vlans (int ifindex)
 int
 nmp_utils_ethtool_get_peer_ifindex (int ifindex)
 {
+	nm_auto_socket_handle SocketHandle shandle = { };
+	int r;
+
 	gs_free struct ethtool_stats *stats = NULL;
 	int peer_ifindex_stat;

 	g_return_val_if_fail (ifindex > 0, 0);

-	peer_ifindex_stat = ethtool_get_stringset_index (ifindex, ETH_SS_STATS, "peer_ifindex");
+	if ((r = socket_handle_init (&shandle, ifindex)) < 0) {
+		nm_log_trace (LOGD_PLATFORM, "ethtool[%d]: %s: failed creating ethtool socket: %s",
+		              ifindex,
+		              "get-peer-ifindex",
+		              g_strerror (-r));
+		return FALSE;
+	}
+
+	peer_ifindex_stat = ethtool_get_stringset_index (&shandle, ETH_SS_STATS, "peer_ifindex");
 	if (peer_ifindex_stat < 0) {
-		nm_log_dbg (LOGD_PLATFORM, "ethtool: peer_ifindex stat for %d does not exist?", ifindex);
+		nm_log_dbg (LOGD_PLATFORM, "ethtool[%d]: peer_ifindex stat does not exist?", ifindex);
 		return FALSE;
 	}

 	stats = g_malloc0 (sizeof (*stats) + (peer_ifindex_stat + 1) * sizeof (guint64));
 	stats->cmd = ETHTOOL_GSTATS;
 	stats->n_stats = peer_ifindex_stat + 1;
-	if (!ethtool_get (ifindex, stats))
+	if (ethtool_call_ifindex (ifindex, stats) < 0)
 		return 0;

 	return stats->data[peer_ifindex_stat];
@@ -369,7 +417,7 @@ nmp_utils_ethtool_get_wake_on_lan (int ifindex)

 	memset (&wol, 0, sizeof (wol));
 	wol.cmd = ETHTOOL_GWOL;
-	if (!ethtool_get (ifindex, &wol))
+	if (ethtool_call_ifindex (ifindex, &wol) < 0)
 		return FALSE;

 	return wol.wolopts != 0;
@@ -387,7 +435,7 @@ nmp_utils_ethtool_get_link_settings (int ifindex,

 	g_return_val_if_fail (ifindex > 0, FALSE);

-	if (!ethtool_get (ifindex, &edata))
+	if (ethtool_call_ifindex (ifindex, &edata) < 0)
 		return FALSE;

 	if (out_autoneg)
@@ -469,7 +517,7 @@ nmp_utils_ethtool_set_link_settings (int ifindex,
 	                      || (!speed && duplex == NM_PLATFORM_LINK_DUPLEX_UNKNOWN), FALSE);

 	/* retrieve first current settings */
-	if (!ethtool_get (ifindex, &edata))
+	if (ethtool_call_ifindex (ifindex, &edata) < 0)
 		return FALSE;

 	/* then change the needed ones */
@@ -521,7 +569,7 @@ nmp_utils_ethtool_set_link_settings (int ifindex,
 		}
 	}

-	return ethtool_get (ifindex, &edata);
+	return ethtool_call_ifindex (ifindex, &edata) >= 0;
 }

 gboolean
@@ -536,8 +584,8 @@ nmp_utils_ethtool_set_wake_on_lan (int ifindex,
 	if (wol == NM_SETTING_WIRED_WAKE_ON_LAN_IGNORE)
 		return TRUE;

-	nm_log_dbg (LOGD_PLATFORM, "setting Wake-on-LAN options 0x%x, password '%s'",
-	            (unsigned) wol, wol_password);
+	nm_log_dbg (LOGD_PLATFORM, "ethtool[%d]: setting Wake-on-LAN options 0x%x, password '%s'",
+	            ifindex, (unsigned) wol, wol_password);

 	wol_info.cmd = ETHTOOL_SWOL;
 	wol_info.wolopts = 0;
@@ -557,13 +605,13 @@ nmp_utils_ethtool_set_wake_on_lan (int ifindex,

 	if (wol_password) {
 		if (!nm_utils_hwaddr_aton (wol_password, wol_info.sopass, ETH_ALEN)) {
-			nm_log_dbg (LOGD_PLATFORM, "couldn't parse Wake-on-LAN password '%s'", wol_password);
+			nm_log_dbg (LOGD_PLATFORM, "ethtool[%d]: couldn't parse Wake-on-LAN password '%s'", ifindex, wol_password);
 			return FALSE;
 		}
 		wol_info.wolopts |= WAKE_MAGICSECURE;
 	}

-	return ethtool_get (ifindex, &wol_info);
+	return ethtool_call_ifindex (ifindex, &wol_info) >= 0;
 }

 /******************************************************************************