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

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

[Herve Codina]

Hi all,

On Tue, 12 May 2026 21:16:43 +0000
sashiko-bot@kernel.org wrote:

> 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?

Well dev->fwnode is checked for NULL in fwnode_for_each_available_child_node()
iterator.
  https://elixir.bootlin.com/linux/v7.1-rc3/source/include/linux/property.h#L175
  https://elixir.bootlin.com/linux/v7.1-rc3/source/drivers/base/property.c#L840

Adding an additional check for dev->fwnode is not needed.

Best regards,
Hervé

> 
> 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);
> >  
> 



-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com