Re: [PATCH v3] hwmon: (lm90) Switch channel parsing to fwnode APIs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Switching to `device_for_each_child_node_scoped()` exposes a critical pre-existing bug in the core `fwnode_get_next_child_node()` which causes an infinite loop and double-frees if a device has both OF and secondary firmware nodes.
--

--- Patch [1]: [PATCH v3] hwmon: (lm90) Switch channel parsing to fwnode APIs ---
commit 39e5cdd2d72333fe0fa7b34c25ad27651948158c
Author: Flaviu Nistor <[email protected]>

hwmon: (lm90) Switch channel parsing to fwnode APIs

Replace OF property handling with fwnode in the probe function to read
the channels properties, improving the driver compatibility since this
method is not limited to Device Tree only.
Add also the needed headers for explicit include and clean up related
function naming.

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 4b9c0ccdf2609..03779c1a251a2 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c

[ ... ]

> @@ -2739,18 +2744,17 @@ static int lm90_probe_channel_from_dt(struct i2c_client *client,
>  	return 0;
>  }
>  
> -static int lm90_parse_dt_channel_info(struct i2c_client *client,
> -				      struct lm90_data *data)
> +static int lm90_parse_channel_info(struct i2c_client *client,
> +				   struct lm90_data *data)
>  {
>  	int err;
>  	struct device *dev = &client->dev;
> -	const struct device_node *np = dev->of_node;
>  
> -	for_each_child_of_node_scoped(np, child) {
> -		if (strcmp(child->name, "channel"))
> +	device_for_each_child_node_scoped(dev, child) {
> +		if (!fwnode_name_eq(child, "channel"))

[Severity: Critical]
This is a pre-existing issue, but switching to
device_for_each_child_node_scoped() exposes this driver to a regression
in the core fwnode_get_next_child_node() framework.

If a device has both Open Firmware and secondary firmware nodes (swnodes),
can this iterator trigger an infinite loop and a double-free?

When transitioning from the primary Open Firmware fwnode to the secondary
swnode, the core fwnode_get_next_child_node() passes the already-put
primary child to the secondary fwnode backend:

drivers/base/property.c:fwnode_get_next_child_node() {
	...
	/* When no more children in primary, continue with secondary */
	return fwnode_call_ptr_op(fwnode->secondary, get_next_child_node, child);
}

Since the passed child is an Open Firmware node rather than a software node,
to_swnode() inside software_node_get_next_child() returns a null pointer.
This causes it to drop the reference on the node a second time (the first
drop was in of_get_next_available_child()), leading to a double-free:

drivers/base/swnode.c:software_node_get_next_child() {
	...
	if (!p || list_empty(&p->children) ||
	    (c && list_is_last(&c->entry, &p->children))) {
		fwnode_handle_put(child);
		return NULL;
	}
}

Furthermore, on the next iteration, the returned secondary child is passed
back to the primary backend. Since it is not an Open Firmware node,
to_of_node() returns a null pointer, causing of_get_next_available_child()
to restart the search and return the first node again, creating an
infinite loop between node types.

Could this driver probe freeze the kernel on systems where a software node
is attached alongside Open Firmware child nodes?

>  			continue;
>  
> -		err = lm90_probe_channel_from_dt(client, child, data);
> +		err = lm90_probe_channel(client, child, data);
>  		if (err)
>  			return err;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1