From: Felix Fietkau <nbd@nbd.name>
Date: Mon, 27 Sep 2021 16:56:21 +0000 (+0200)
Subject: netifd: rework/fix device free handling
X-Git-Url: http://git.lede-project.org./?a=commitdiff_plain;h=5a4ac30c7a15712d01110befec1acfe86c2cbed0;p=project%2Fnetifd.git

netifd: rework/fix device free handling

Instead of explicitly preventing free in specific code sections using
device_lock/device_unlock, defer all device free handling via uloop timeout
This avoids an entire class of lurking use-after-free bugs triggered
by device event processing and simplifies the code

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---

diff --git a/alias.c b/alias.c
index 951e046..98d5410 100644
--- a/alias.c
+++ b/alias.c
@@ -178,13 +178,9 @@ alias_notify_device(const char *name, struct device *dev)
 {
 	struct alias_device *alias;
 
-	device_lock();
-
 	alias = avl_find_element(&aliases, name, alias, avl);
 	if (alias)
 		alias_set_device(alias, dev);
-
-	device_unlock();
 }
 
 struct device *
diff --git a/bonding.c b/bonding.c
index 0bf4f9a..457fe51 100644
--- a/bonding.c
+++ b/bonding.c
@@ -566,8 +566,6 @@ bonding_free_port(struct bonding_port *bp)
 
 	bonding_remove_port(bp);
 
-	device_lock();
-
 	device_remove_user(&bp->dev);
 
 	/*
@@ -582,8 +580,6 @@ bonding_free_port(struct bonding_port *bp)
 		device_set_present(dev, true);
 	}
 
-	device_unlock();
-
 	free(bp);
 }
 
diff --git a/bridge.c b/bridge.c
index 2ce5c2b..7e61b9d 100644
--- a/bridge.c
+++ b/bridge.c
@@ -512,8 +512,6 @@ restart:
 		goto restart;
 	}
 
-	device_lock();
-
 	device_remove_user(&bm->dev);
 	uloop_timeout_cancel(&bm->check_timer);
 
@@ -529,8 +527,6 @@ restart:
 		device_set_present(dev, true);
 	}
 
-	device_unlock();
-
 	free(bm);
 }
 
diff --git a/config.c b/config.c
index d83ea9c..9bbda39 100644
--- a/config.c
+++ b/config.c
@@ -762,7 +762,6 @@ config_init_all(void)
 
 	vlist_update(&interfaces);
 	config_init = true;
-	device_lock();
 
 	device_reset_config();
 	config_init_devices(true);
@@ -775,12 +774,10 @@ config_init_all(void)
 	config_init_wireless();
 
 	config_init = false;
-	device_unlock();
 
 	device_reset_old();
 	device_init_pending();
 	vlist_flush(&interfaces);
-	device_free_unused(NULL);
 	interface_refresh_assignments(false);
 	interface_start_pending();
 	wireless_start_pending();
diff --git a/device.c b/device.c
index bb39ea7..b3d0e85 100644
--- a/device.c
+++ b/device.c
@@ -99,18 +99,6 @@ device_type_get(const char *tname)
 	return NULL;
 }
 
-void device_lock(void)
-{
-	__devlock++;
-}
-
-void device_unlock(void)
-{
-	__devlock--;
-	if (!__devlock)
-		device_free_unused(NULL);
-}
-
 static int device_vlan_len(struct kvlist *kv, const void *data)
 {
 	return sizeof(unsigned int);
@@ -895,14 +883,27 @@ device_free(struct device *dev)
 }
 
 static void
-__device_free_unused(struct device *dev)
+__device_free_unused(struct uloop_timeout *timeout)
 {
-	if (!safe_list_empty(&dev->users) ||
-		!safe_list_empty(&dev->aliases) ||
-	    dev->current_config || __devlock)
-		return;
+	struct device *dev, *tmp;
+
+	avl_for_each_element_safe(&devices, dev, avl, tmp) {
+		if (!safe_list_empty(&dev->users) ||
+			!safe_list_empty(&dev->aliases) ||
+			dev->current_config)
+			continue;
+
+		device_free(dev);
+	}
+}
+
+void device_free_unused(void)
+{
+	static struct uloop_timeout free_timer = {
+		.cb = __device_free_unused,
+	};
 
-	device_free(dev);
+	uloop_timeout_set(&free_timer, 1);
 }
 
 void device_remove_user(struct device_user *dep)
@@ -919,19 +920,7 @@ void device_remove_user(struct device_user *dep)
 	safe_list_del(&dep->list);
 	dep->dev = NULL;
 	D(DEVICE, "Remove user for device '%s', refcount=%d\n", dev->ifname, device_refcount(dev));
-	__device_free_unused(dev);
-}
-
-void
-device_free_unused(struct device *dev)
-{
-	struct device *tmp;
-
-	if (dev)
-		return __device_free_unused(dev);
-
-	avl_for_each_element_safe(&devices, dev, avl, tmp)
-		__device_free_unused(dev);
+	device_free_unused();
 }
 
 void
diff --git a/device.h b/device.h
index 0496e89..37f8c37 100644
--- a/device.h
+++ b/device.h
@@ -300,9 +300,6 @@ extern const struct uci_blob_param_list device_attr_list;
 extern struct device_type simple_device_type;
 extern struct device_type tunnel_device_type;
 
-void device_lock(void);
-void device_unlock(void);
-
 void device_vlan_update(bool done);
 void device_stp_init(void);
 
@@ -346,7 +343,7 @@ void device_release(struct device_user *dep);
 int device_check_state(struct device *dev);
 void device_dump_status(struct blob_buf *b, struct device *dev);
 
-void device_free_unused(struct device *dev);
+void device_free_unused(void);
 
 struct device *get_vlan_device_chain(const char *ifname, int create);
 void alias_notify_device(const char *name, struct device *dev);
diff --git a/extdev.c b/extdev.c
index 977d9c2..5c5e769 100644
--- a/extdev.c
+++ b/extdev.c
@@ -942,11 +942,9 @@ __create(const char *name, struct device_type *type, struct blob_attr *config)
 inv_error:
 	extdev_invocation_error(ret, __extdev_methods[METHOD_CREATE], name);
 error:
-	device_lock();
 	free(edev->dev.config);
 	device_cleanup(&edev->dev);
 	free(edev);
-	device_unlock();
 	netifd_log_message(L_WARNING, "Failed to create %s %s\n", type->name, name);
 	return NULL;
 }
diff --git a/interface.c b/interface.c
index 2391e12..6cf0d30 100644
--- a/interface.c
+++ b/interface.c
@@ -637,8 +637,6 @@ interface_claim_device(struct interface *iface)
 	if (iface->parent_iface.iface)
 		interface_remove_user(&iface->parent_iface);
 
-	device_lock();
-
 	if (iface->parent_ifname) {
 		parent = vlist_find(&interfaces, iface->parent_ifname, parent, node);
 		iface->parent_iface.cb = interface_alias_cb;
@@ -654,8 +652,6 @@ interface_claim_device(struct interface *iface)
 	if (dev)
 		interface_set_main_dev(iface, dev);
 
-	device_unlock();
-
 	if (iface->proto_handler->flags & PROTO_FLAG_INIT_AVAILABLE)
 		interface_set_available(iface, true);
 }
@@ -1087,30 +1083,19 @@ interface_handle_link(struct interface *iface, const char *name,
 		      struct blob_attr *vlan, bool add, bool link_ext)
 {
 	struct device *dev;
-	int ret;
-
-	device_lock();
 
 	dev = device_get(name, add ? (link_ext ? 2 : 1) : 0);
-	if (!dev) {
-		ret = UBUS_STATUS_NOT_FOUND;
-		goto out;
-	}
+	if (!dev)
+		return UBUS_STATUS_NOT_FOUND;
 
-	if (add) {
-		interface_set_device_config(iface, dev);
-		if (!link_ext)
-			device_set_present(dev, true);
+	if (!add)
+		return interface_remove_link(iface, dev, vlan);
 
-		ret = interface_add_link(iface, dev, vlan, link_ext);
-	} else {
-		ret = interface_remove_link(iface, dev, vlan);
-	}
+	interface_set_device_config(iface, dev);
+	if (!link_ext)
+		device_set_present(dev, true);
 
-out:
-	device_unlock();
-
-	return ret;
+	return interface_add_link(iface, dev, vlan, link_ext);
 }
 
 void
diff --git a/ubus.c b/ubus.c
index 56cce81..4770cb6 100644
--- a/ubus.c
+++ b/ubus.c
@@ -291,7 +291,7 @@ netifd_handle_alias(struct ubus_context *ctx, struct ubus_object *obj,
 	return 0;
 
 error:
-	device_free_unused(dev);
+	device_free_unused();
 	return UBUS_STATUS_INVALID_ARGUMENT;
 }