rework device hotplug handling some more, add device_lock/device_unlock to prevent use-after-free bugs

7 files changed, 61 insertions, 18 deletions

diff --git a/bridge.c b/bridge.c
index 1adabb9..1f1a220 100644
--- a/bridge.c
+++ b/bridge.c
@@ -77,6 +77,8 @@ struct bridge_member {
 	bool present;
 };
 
+static void bridge_free_member(struct bridge_member *bm);
+
 static int
 bridge_disable_member(struct bridge_member *bm)
 {
@@ -147,6 +149,8 @@ bridge_member_cb(struct device_user *dev, enum device_event ev)
 		if (bst->n_present == 0)
 			device_set_present(&bst->dev, false);
 
+		if (dev->hotplug)
+			bridge_free_member(bm);
 		break;
 	default:
 		return;
@@ -213,7 +217,7 @@ bridge_set_state(struct device *dev, bool up)
 }
 
 static struct bridge_member *
-bridge_create_member(struct bridge_state *bst, struct device *dev)
+bridge_create_member(struct bridge_state *bst, struct device *dev, bool hotplug)
 {
 	struct bridge_member *bm;
 
@@ -221,6 +225,7 @@ bridge_create_member(struct bridge_state *bst, struct device *dev)
 	bm->bst = bst;
 	bm->dev.cb = bridge_member_cb;
 	device_add_user(&bm->dev, dev);
+	bm->dev.hotplug = hotplug;
 
 	list_add_tail(&bm->list, &bst->members);
 
@@ -254,7 +259,7 @@ bridge_add_member(struct bridge_state *bst, const char *name)
 	if (!dev)
 		return;
 
-	bridge_create_member(bst, dev);
+	bridge_create_member(bst, dev, false);
 }
 
 static int
@@ -262,7 +267,7 @@ bridge_hotplug_add(struct device *dev, struct device *member)
 {
 	struct bridge_state *bst = container_of(dev, struct bridge_state, dev);
 
-	bridge_create_member(bst, member);
+	bridge_create_member(bst, member, true);
 
 	return 0;
 }
diff --git a/config.c b/config.c
index 9f42471..86c2599 100644
--- a/config.c
+++ b/config.c
@@ -344,6 +344,7 @@ config_init_interfaces(const char *name)
 
 	uci_network = p;
 	config_init = true;
+	device_lock();
 
 	device_reset_config();
 	config_init_devices();
@@ -357,7 +358,9 @@ config_init_interfaces(const char *name)
 		if (!strcmp(s->type, "interface"))
 			config_parse_interface(s);
 	}
+
 	config_init = false;
+	device_unlock();
 
 	device_reset_old();
 	device_init_pending();
diff --git a/device.c b/device.c
index 1136380..0a33f67 100644
--- a/device.c
+++ b/device.c
@@ -41,6 +41,20 @@ const struct config_param_list device_attr_list = {
 	.params = dev_attrs,
 };
 
+static int __devlock = 0;
+
+void device_lock(void)
+{
+	__devlock++;
+}
+
+void device_unlock(void)
+{
+	__devlock--;
+	if (!__devlock)
+		device_free_unused(NULL);
+}
+
 static struct device *
 simple_device_create(const char *name, struct blob_attr *attr)
 {
@@ -172,6 +186,8 @@ 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)
 		return;
@@ -189,6 +205,8 @@ alias_notify_device(const char *name, struct device *dev)
 
 	if (!dev && alias->dep.dev && !alias->dep.dev->active)
 		device_remove_user(&alias->dep);
+
+	device_unlock();
 }
 
 static int set_device_state(struct device *dev, bool state)
@@ -398,6 +416,7 @@ void device_remove_user(struct device_user *dep)
 	if (!dep->dev)
 		return;
 
+	dep->hotplug = false;
 	if (dep->claimed)
 		device_release(dep);
 
diff --git a/device.h b/device.h
index f7718cc..3d233d7 100644
--- a/device.h
+++ b/device.h
@@ -100,6 +100,8 @@ struct device_user {
 	struct list_head list;
 
 	bool claimed;
+	bool hotplug;
+
 	struct device *dev;
 	void (*cb)(struct device_user *, enum device_event);
 };
@@ -113,6 +115,9 @@ extern const struct config_param_list device_attr_list;
 extern const struct device_type simple_device_type;
 extern const struct device_type bridge_device_type;
 
+void device_lock(void);
+void device_unlock(void);
+
 struct device *device_create(const char *name, const struct device_type *type,
 			     struct blob_attr *config);
 void device_init_settings(struct device *dev, struct blob_attr **tb);
diff --git a/interface.c b/interface.c
index 8e42f21..befb194 100644
--- a/interface.c
+++ b/interface.c
@@ -88,13 +88,19 @@ interface_event(struct interface *iface, enum interface_event ev)
 }
 
 static void
-mark_interface_down(struct interface *iface)
+interface_flush_state(struct interface *iface)
 {
 	interface_clear_dns(iface);
 	vlist_flush_all(&iface->proto_addr);
 	vlist_flush_all(&iface->proto_route);
 	if (iface->main_dev.dev)
 		device_release(&iface->main_dev);
+}
+
+static void
+mark_interface_down(struct interface *iface)
+{
+	interface_flush_state(iface);
 	iface->state = IFS_DOWN;
 }
 
@@ -134,6 +140,8 @@ __interface_set_down(struct interface *iface, bool force)
 	iface->state = IFS_TEARDOWN;
 	interface_event(iface, IFEV_DOWN);
 	interface_proto_event(iface->proto, PROTO_CMD_TEARDOWN, force);
+	if (force)
+		interface_flush_state(iface);
 }
 
 static void
@@ -208,7 +216,6 @@ interface_cleanup(struct interface *iface)
 {
 	struct interface_user *dep, *tmp;
 
-	iface->hotplug_dev = false;
 	list_for_each_entry_safe(dep, tmp, &iface->users, list)
 		interface_remove_user(dep);
 
diff --git a/interface.h b/interface.h
index 91cffca..8a00900 100644
--- a/interface.h
+++ b/interface.h
@@ -62,7 +62,6 @@ struct interface {
 
 	/* main interface that the interface is bound to */
 	struct device_user main_dev;
-	bool hotplug_dev;
 
 	/* interface that layer 3 communication will go through */
 	struct device_user *l3_dev;
diff --git a/ubus.c b/ubus.c
index a9b0057..c729c5d 100644
--- a/ubus.c
+++ b/ubus.c
@@ -284,14 +284,18 @@ netifd_iface_handle_device(struct ubus_context *ctx, struct ubus_object *obj,
 		return UBUS_STATUS_INVALID_ARGUMENT;
 
 	devname = blobmsg_data(tb[DEV_NAME]);
-	dev = iface->main_dev.dev;
-	if (iface->hotplug_dev && dev && !add) {
-		if (strcmp(dev->ifname, devname) != 0)
-			return UBUS_STATUS_INVALID_ARGUMENT;
-	}
 
-	if (iface->hotplug_dev) {
-		if (iface->main_dev.dev) {
+	device_lock();
+
+	if (iface->main_dev.hotplug) {
+		dev = iface->main_dev.dev;
+
+		if (dev) {
+			if (!add && strcmp(dev->ifname, devname) != 0) {
+				ret = UBUS_STATUS_INVALID_ARGUMENT;
+				goto out;
+			}
+
 			interface_set_available(iface, false);
 			device_remove_user(&iface->main_dev);
 		}
@@ -299,13 +303,15 @@ netifd_iface_handle_device(struct ubus_context *ctx, struct ubus_object *obj,
 		main_dev = iface->main_dev.dev;
 
 	dev = device_get(blobmsg_data(tb[DEV_NAME]), add);
-	if (!dev)
-		return UBUS_STATUS_NOT_FOUND;
+	if (!dev && (main_dev || add)) {
+		ret = UBUS_STATUS_NOT_FOUND;
+		goto out;
+	}
 
 	if (!main_dev) {
 		if (add) {
 			device_add_user(&iface->main_dev, dev);
-			iface->hotplug_dev = true;
+			iface->main_dev.hotplug = true;
 		}
 		ret = 0;
 		goto out;
@@ -328,8 +334,7 @@ netifd_iface_handle_device(struct ubus_context *ctx, struct ubus_object *obj,
 	}
 
 out:
-	if (add)
-		device_free_unused(dev);
+	device_unlock();
 
 	return ret;
 }