diff mbox

discover: Wait for net interfaces to be marked ready

Message ID 20170628053954.18441-1-sam@mendozajonas.com
State Accepted
Headers show

Commit Message

Sam Mendoza-Jonas June 28, 2017, 5:39 a.m. UTC
If pb-discover is started before udev has settled there is a race
between Petitboot configuring interfaces and udev renaming them. If an
interface is set "up" the name change will fail and interfaces can be
inconsistently named, eg:

  Device:        (*) eth0 [0c:c4:7a:f4:1c:50, link up]
                 ( ) enP1p9s0f1 [0c:c4:7a:f4:1c:51, link down]
                 ( ) enP1p9s0f2 [0c:c4:7a:f4:1c:52, link down]
                 ( ) enP1p9s0f3 [0c:c4:7a:f4:1c:53, link down]

Add "net" devices to the udev filter and wait for them to be announced
by udev before configuring them.
udev_enumerate_add_match_is_initialized() ensures that by the time an
interface appears via udev its name will be consistent.

This also swaps the network and udev init order, but since interfaces
now will not be configured until after udev is ready this should not
have a user-visible effect.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
We can probably trim out some of the error checking in both
network_mark_interface_ready() and udev_check_interface_ready(), trust that
udev has made everything consistent and just operate on the interface index.
Will spend some more double checking that assumption.

 discover/device-handler.c | 12 +++----
 discover/network.c        | 71 ++++++++++++++++++++++++++++++++++++++
 discover/network.h        |  3 ++
 discover/udev.c           | 88 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 168 insertions(+), 6 deletions(-)
diff mbox

Patch

diff --git a/discover/device-handler.c b/discover/device-handler.c
index ec1eee3..edda725 100644
--- a/discover/device-handler.c
+++ b/discover/device-handler.c
@@ -1382,15 +1382,15 @@  static void device_handler_update_lang(const char *lang)
 static int device_handler_init_sources(struct device_handler *handler)
 {
 	/* init our device sources: udev, network and user events */
-	handler->udev = udev_init(handler, handler->waitset);
-	if (!handler->udev)
-		return -1;
-
 	handler->network = network_init(handler, handler->waitset,
 			handler->dry_run);
 	if (!handler->network)
 		return -1;
 
+	handler->udev = udev_init(handler, handler->waitset);
+	if (!handler->udev)
+		return -1;
+
 	handler->user_event = user_event_init(handler, handler->waitset);
 	if (!handler->user_event)
 		return -1;
@@ -1407,11 +1407,11 @@  static void device_handler_reinit_sources(struct device_handler *handler)
 		return;
 	}
 
-	udev_reinit(handler->udev);
-
 	network_shutdown(handler->network);
 	handler->network = network_init(handler, handler->waitset,
 			handler->dry_run);
+
+	udev_reinit(handler->udev);
 }
 
 static inline const char *get_device_path(struct discover_device *dev)
diff --git a/discover/network.c b/discover/network.c
index 8ca4561..c38bf25 100644
--- a/discover/network.c
+++ b/discover/network.c
@@ -53,6 +53,7 @@  struct interface {
 	struct list_item list;
 	struct process *udhcpc_process;
 	struct discover_device *dev;
+	bool ready;
 };
 
 struct network {
@@ -595,6 +596,11 @@  static int network_handle_nlmsg(struct network *network, struct nlmsghdr *nlmsg)
 	if (!interface->dev)
 		create_interface_dev(network, interface);
 
+	if (!interface->ready && strcmp(interface->name, "lo")) {
+		pb_log("%s not marked ready yet\n", interface->name);
+		return 0;
+	}
+
 	configure_interface(network, interface,
 			info->ifi_flags & IFF_UP,
 			info->ifi_flags & IFF_LOWER_UP);
@@ -602,6 +608,71 @@  static int network_handle_nlmsg(struct network *network, struct nlmsghdr *nlmsg)
 	return 0;
 }
 
+void network_mark_interface_ready(struct device_handler *handler,
+		int ifindex, const char *ifname, uint8_t *mac, int hwsize)
+{
+	struct network *network = device_handler_get_network(handler);
+	struct interface *interface, *tmp = NULL;
+	char *macstr;
+
+	if (!network) {
+		pb_log("Network not ready - can not mark interface ready\n");
+		return;
+	}
+
+	if (hwsize != HWADDR_SIZE)
+		return;
+
+	if (strncmp(ifname, "lo", strlen("lo")) == 0)
+		return;
+
+	interface = find_interface_by_ifindex(network, ifindex);
+	if (!interface) {
+		pb_debug("Creating ready interface %d - %s\n",
+				ifindex, ifname);
+		interface = talloc_zero(network, struct interface);
+		interface->ifindex = ifindex;
+		interface->state = IFSTATE_NEW;
+		memcpy(interface->hwaddr, mac, HWADDR_SIZE);
+		strncpy(interface->name, ifname, sizeof(interface->name) - 1);
+
+		list_for_each_entry(&network->interfaces, tmp, list)
+			if (memcmp(interface->hwaddr, tmp->hwaddr,
+				   sizeof(interface->hwaddr)) == 0) {
+				pb_log("%s: %s has duplicate MAC address, ignoring\n",
+				       __func__, interface->name);
+				talloc_free(interface);
+				return;
+			}
+
+		list_add(&network->interfaces, &interface->list);
+		create_interface_dev(network, interface);
+	}
+
+	if (interface->ready) {
+		pb_log("%s already ready\n", interface->name);
+		return;
+	}
+
+	if (strncmp(interface->name, ifname, strlen(ifname)) != 0) {
+		pb_log("New name for interface %d do not match: %s vs %s\n",
+				ifindex, ifname, interface->name);
+		return;
+	}
+
+	if (memcmp(interface->hwaddr, mac, HWADDR_SIZE) != 0) {
+		macstr = mac_bytes_to_string(interface, mac, hwsize);
+		pb_log("New MAC for interface %d does not match: %s\n",
+				ifindex, macstr);
+		talloc_free(macstr);
+		return;
+	}
+
+	pb_log("Interface %s ready\n", ifname);
+	interface->ready = true;
+	configure_interface(network, interface, false, false);
+}
+
 static int network_netlink_process(void *arg)
 {
 	struct network *network = arg;
diff --git a/discover/network.h b/discover/network.h
index e5e05d5..bf1f2de 100644
--- a/discover/network.h
+++ b/discover/network.h
@@ -18,5 +18,8 @@  void network_unregister_device(struct network *network,
 uint8_t *find_mac_by_name(void *ctx, struct network *network,
 		const char *name);
 
+void network_mark_interface_ready(struct device_handler *handler,
+		int ifindex, const char *ifname, uint8_t *mac, int hwsize);
+
 #endif /* NETWORK_H */
 
diff --git a/discover/udev.c b/discover/udev.c
index 1e04313..029aa3d 100644
--- a/discover/udev.c
+++ b/discover/udev.c
@@ -26,6 +26,7 @@ 
 #include "device-handler.h"
 #include "cdrom.h"
 #include "devmapper.h"
+#include "network.h"
 
 /* We set a default monitor buffer size, as we may not process monitor
  * events while performing device discvoery. systemd uses a 128M buffer, so
@@ -186,6 +187,69 @@  static int udev_handle_block_add(struct pb_udev *udev, struct udev_device *dev,
 	return 0;
 }
 
+/*
+ * Mark valid interfaces as 'ready'.
+ * The udev_enumerate_add_match_is_initialized() filter in udev_enumerate()
+ * ensures that any device we see is properly initialized by udev (eg. interface
+ * names); here we check that the properties are sane and mark the interface
+ * as ready for configuration in discover/network.
+ */
+static int udev_check_interface_ready(struct device_handler *handler,
+		struct udev_device *dev)
+{
+	const char *name, *name_path, *ifindex, *interface, *mac_name;
+	uint8_t *mac;
+	char byte[3];
+	unsigned int i, j;
+
+
+	name = udev_device_get_sysname(dev);
+	if (!name) {
+		pb_debug("udev_device_get_sysname failed\n");
+		return -1;
+	}
+
+	name_path = udev_device_get_property_value(dev, "ID_NET_NAME_PATH");
+	ifindex = udev_device_get_property_value(dev, "IFINDEX");
+	interface = udev_device_get_property_value(dev, "INTERFACE");
+	mac_name = udev_device_get_property_value(dev, "ID_NET_NAME_MAC");
+
+	/* Physical interfaces should have all of these properties */
+	if (!name_path || !ifindex || !interface || !mac_name) {
+		pb_debug("%s: interface %s missing properties\n",
+				__func__, name);
+		return -1;
+	}
+
+	/* ID_NET_NAME_MAC format is enxMACADDR */
+	if (strlen(mac_name) < 15) {
+		pb_debug("%s: Unexpected MAC format: %s\n",
+				__func__, mac_name);
+		return -1;
+	}
+
+	mac = talloc_array(handler, uint8_t, HWADDR_SIZE);
+	if (!mac)
+		return -1;
+
+	/*
+	 * ID_NET_NAME_MAC is not a conventionally formatted MAC
+	 * string - convert it before passing it to network.c
+	 */
+	byte[2] = '\0';
+	for (i = strlen("enx"), j = 0;
+			i < strlen(mac_name) && j < HWADDR_SIZE; i += 2) {
+		memcpy(byte, &mac_name[i], 2);
+		mac[j++] = strtoul(byte, NULL, 16);
+	}
+
+	network_mark_interface_ready(handler,
+			atoi(ifindex), interface, mac, HWADDR_SIZE);
+
+	talloc_free(mac);
+	return 0;
+}
+
 static int udev_handle_dev_add(struct pb_udev *udev, struct udev_device *dev)
 {
 	const char *subsys;
@@ -203,6 +267,10 @@  static int udev_handle_dev_add(struct pb_udev *udev, struct udev_device *dev)
 		return -1;
 	}
 
+	/* If we see a net device, check if it is ready to be used */
+	if (!strncmp(subsys, "net", strlen("net")))
+		return udev_check_interface_ready(udev->handler, dev);
+
 	if (device_lookup_by_id(udev->handler, name)) {
 		pb_debug("device %s is already present?\n", name);
 		return -1;
@@ -280,10 +348,16 @@  static bool udev_handle_cdrom_events(struct pb_udev *udev,
 static int udev_handle_dev_change(struct pb_udev *udev, struct udev_device *dev)
 {
 	struct discover_device *ddev;
+	const char *subsys;
 	const char *name;
 	int rc = 0;
 
 	name = udev_device_get_sysname(dev);
+	subsys = udev_device_get_subsystem(dev);
+
+	/* If we see a net device, check if it is ready to be used */
+	if (!strncmp(subsys, "net", strlen("net")))
+		return udev_check_interface_ready(udev->handler, dev);
 
 	ddev = device_lookup_by_id(udev->handler, name);
 
@@ -348,6 +422,12 @@  static int udev_enumerate(struct udev *udev)
 		goto fail;
 	}
 
+	result = udev_enumerate_add_match_subsystem(enumerate, "net");
+	if (result) {
+		pb_log("udev_enumerate_add_match_subsystem failed\n");
+		goto fail;
+	}
+
 	result = udev_enumerate_add_match_is_initialized(enumerate);
 	if (result) {
 		pb_log("udev_enumerate_add_match_is_initialised failed\n");
@@ -405,6 +485,14 @@  static int udev_setup_monitor(struct udev *udev, struct udev_monitor **monitor)
 		goto out_err;
 	}
 
+	result = udev_monitor_filter_add_match_subsystem_devtype(m, "net",
+		NULL);
+
+	if (result) {
+		pb_log("udev_monitor_filter_add_match_subsystem_devtype failed\n");
+		goto out_err;
+	}
+
 	result = udev_monitor_enable_receiving(m);
 
 	if (result) {