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 951e046bb3f135b582ca4a371f01f3a647a282ab..98d54100fef9294eae187275b934850a3a054098 100644 (file)
--- 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 0bf4f9a331efe3dfd8a0a5105667bf424b3e9eee..457fe515989910e84c670a79fa22de05b75f2433 100644 (file)
--- 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 2ce5c2b11b49fd23c14b7c707e9af0c4463e13a8..7e61b9df8326175d8e28cbdd0bacbfc8e0a693c1 100644 (file)
--- 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 d83ea9cb6b6c77ef012b7481aa0f332055bb61da..9bbda39d3fb5259499cc50c10309920b39537364 100644 (file)
--- 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 bb39ea7f8d71b46c750a770b771a5bc33d362bb9..b3d0e85f8550de45fbabeb8f17028ddf8af1573e 100644 (file)
--- 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 0496e893cbc93f53691649c8096f7d0a7af0c5c0..37f8c37c58a35f948c2476ac9c89389838246136 100644 (file)
--- 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 977d9c2fc7f9b818cf184939fcebaeb03a028a5e..5c5e76901d810d6cbb8f68a52d0f914660b4854d 100644 (file)
--- 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 2391e121badf56540679309c01bff5b86091a8af..6cf0d309d5f53ded71a717c39394bc184be13ca8 100644 (file)
--- 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 56cce8163cefa01f4fcf81a7b59addab0960ebd7..4770cb686534ae37f230b3067e638ed16373d1f1 100644 (file)
--- 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;
 }