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