From d16871c7a55370174eb672edee24feade74cd37e Mon Sep 17 00:00:00 2001 From: Felix Fietkau Date: Wed, 19 Oct 2011 03:20:09 +0200 Subject: [PATCH] rework device hotplug handling some more, add device_lock/device_unlock to prevent use-after-free bugs --- bridge.c | 11 ++++++++--- config.c | 3 +++ device.c | 19 +++++++++++++++++++ device.h | 5 +++++ interface.c | 11 +++++++++-- interface.h | 1 - ubus.c | 29 +++++++++++++++++------------ 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; } -- 2.25.1