Re: [PATCH v7 2/3] of: dynamic: Fix overlayed devices not probing because of fw_devlink

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing `dev->fwnode` validation in `fw_devlink_pickup_dangling_consumers()` leads to a kernel panic if the device's fwnode is NULL.
--

commit eebf7e6f50a0aa0439d5a1099141444a70ec8219
Author: Saravana Kannan <saravanak@kernel.org>

of: dynamic: Fix overlayed devices not probing because of fw_devlink

When an overlay is applied to an already probed device, fw_devlink logic
is rerun. This allows newly created dangling consumers of overlayed device
tree nodes to become consumers of the target device.

Link: https://lore.kernel.org/lkml/20240411235623.1260061-3-saravanak@google.com/

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index bd2ddf2aab505..478aa3fbf1e83 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -235,6 +235,79 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
>  		__fw_devlink_pickup_dangling_consumers(child, new_sup);
>  }
>  
> +static void fw_devlink_pickup_dangling_consumers(struct device *dev)
> +{
> +	struct fwnode_handle *child;
> +
> +	guard(mutex)(&fwnode_link_lock);
> +
> +	fwnode_for_each_available_child_node(dev->fwnode, child)
> +		__fw_devlink_pickup_dangling_consumers(child, dev->fwnode);
> +	__fw_devlink_link_to_consumers(dev);
> +}

Is it possible for dev->fwnode to be NULL here?

The original logic in device_links_driver_bound() was protected by a
dev->fwnode check. The newly added caller fw_devlink_refresh_fwnode() 
appears to call this without checking if dev->fwnode is still valid:

fw_devlink_refresh_fwnode()
  dev = get_dev_from_fwnode(fwnode);
  ...
  if (dev->links.status == DL_DEV_DRIVER_BOUND) {
      fw_devlink_pickup_dangling_consumers(dev);

If dev->fwnode is NULL, such as when it is cleared by a concurrent
device_remove_of_node() before device_del() runs, the nested call to
__fw_devlink_link_to_consumers() will dereference it:

__fw_devlink_link_to_consumers(struct device *dev)
{
        struct fwnode_handle *fwnode = dev->fwnode;
        ...
        list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) {

Could this lead to a crash when &fwnode->consumers is evaluated? Would it
make sense to add an explicit check for !dev->fwnode at the start of
fw_devlink_pickup_dangling_consumers() to mirror the structural safety
previously provided in device_links_driver_bound()?

[ ... ]

> @@ -1312,16 +1385,8 @@ void device_links_driver_bound(struct device *dev)
>  	 * child firmware node.
>  	 */
>  	if (dev->fwnode && dev->fwnode->dev == dev) {
> -		struct fwnode_handle *child;
> -
>  		fwnode_links_purge_suppliers(dev->fwnode);
> -
> -		guard(mutex)(&fwnode_link_lock);
> -
> -		fwnode_for_each_available_child_node(dev->fwnode, child)
> -			__fw_devlink_pickup_dangling_consumers(child,
> -							       dev->fwnode);
> -		__fw_devlink_link_to_consumers(dev);
> +		fw_devlink_pickup_dangling_consumers(dev);
>  	}
>  	device_remove_file(dev, &dev_attr_waiting_for_supplier);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511155755.34428-1-herve.codina@bootlin.com?part=2