Felix Fietkau kirjoitti 27.9.2021 klo 19.17:
On 2021-09-27 17:45, Hannu Nyman wrote:
Felix Fietkau kirjoitti 27.9.2021 klo 13.59:
On a crash, it should drop a .core file to /tmp. Please copy that to
your build host and use ./scripts/remote-gdb to obtain a backtrace from
it. I'd like to know, which line of code in netifd it crashes on, so I
can fix it. So far the bug has not shown up in my own tests...

- Felix

This is probably what you are looking for...
To me it looks like it might actually be a list handling bug in libubox.

Can you please try this netifd patch?

At the first glance, the impact looks ok to me:
  wifi goes down with "with down", netifd stays alive and wifi remains down.  
:-)

Hannu


Thanks,

- Felix

---
diff --git a/alias.c b/alias.c
index 951e046bb3f1..98d54100fef9 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 0bf4f9a331ef..457fe5159899 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 2ce5c2b11b49..7e61b9df8326 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 d83ea9cb6b6c..9bbda39d3fb5 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 bb39ea7f8d71..b3d0e85f8550 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 0496e893cbc9..37f8c37c58a3 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 977d9c2fc7f9..5c5e76901d81 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 2391e121badf..6cf0d309d5f5 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 56cce8163cef..4770cb686534 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;
  }



_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Reply via email to