Wifi bug
Hannu Nyman
hannu.nyman at iki.fi
Mon Sep 27 09:30:40 PDT 2021
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;
> }
>
More information about the openwrt-devel
mailing list