Re: [PATCH v6 4/4] power: supply: core: add UAPI to discover currently used extensions

[Thomas Weißschuh]

On 2024-12-11 20:57:58+0100, Thomas Weißschuh wrote:
> Userspace wants to now about the used power supply extensions,
> for example to handle a device extended by a certain extension
> differently or to discover information about the extending device.
> 
> Add a sysfs directory to the power supply device.
> This directory contains links which are named after the used extension
> and point to the device implementing that extension.
> 
> Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> ---
>  Documentation/ABI/testing/sysfs-class-power |  9 +++++++++
>  drivers/power/supply/cros_charge-control.c  |  5 ++++-
>  drivers/power/supply/power_supply.h         |  2 ++
>  drivers/power/supply/power_supply_core.c    | 19 +++++++++++++++++--
>  drivers/power/supply/power_supply_sysfs.c   | 10 ++++++++++
>  drivers/power/supply/test_power.c           |  4 +++-
>  include/linux/power_supply.h                |  2 ++
>  7 files changed, 47 insertions(+), 4 deletions(-)

[..]

> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1346,17 +1346,21 @@ static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
>  }
>  
>  int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
> -				    void *data)
> +				    struct device *dev, void *data)
>  {
>  	struct power_supply_ext_registration *reg;
>  	size_t i;
>  	int ret;
>  
> -	if (!psy || !ext || !ext->properties || !ext->num_properties)
> +	if (!psy || !dev || !ext || !ext->name || !ext->properties || !ext->num_properties)
>  		return -EINVAL;
>  
>  	guard(rwsem_write)(&psy->extensions_sem);
>  
> +	power_supply_for_each_extension(reg, psy)
> +		if (strcmp(ext->name, reg->ext->name) == 0)
> +			return -EEXIST;
> +
>  	for (i = 0; i < ext->num_properties; i++)
>  		if (power_supply_has_property(psy, ext->properties[i]))
>  			return -EEXIST;
> @@ -1366,9 +1370,15 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
>  		return -ENOMEM;
>  
>  	reg->ext = ext;
> +	reg->dev = dev;
>  	reg->data = data;
>  	list_add(&reg->list_head, &psy->extensions);
>  
> +	ret = sysfs_add_link_to_group(&psy->dev.kobj, power_supply_extension_group.name,
> +				      &dev->kobj, ext->name);
> +	if (ret)
> +		goto sysfs_link_failed;
> +
>  	ret = power_supply_update_sysfs_and_hwmon(psy);
>  	if (ret)
>  		goto sysfs_hwmon_failed;
> @@ -1376,6 +1386,8 @@ int power_supply_register_extension(struct power_supply *psy, const struct power
>  	return 0;
>  
>  sysfs_hwmon_failed:
> +	sysfs_remove_link_from_group(&psy->dev.kobj, power_supply_extension_group.name, ext->name);
> +sysfs_link_failed:
>  	list_del(&reg->list_head);
>  	kfree(reg);
>  	return ret;
> @@ -1392,6 +1404,9 @@ void power_supply_unregister_extension(struct power_supply *psy, const struct po
>  		if (reg->ext == ext) {
>  			list_del(&reg->list_head);
>  			kfree(reg);
> +			sysfs_remove_link_from_group(&psy->dev.kobj,
> +						     power_supply_extension_group.name,
> +						     reg->ext->name);

Dan Carpenter's sparse bot detected a use after free here.
Could you move the call to sysfs_remove_link_from_group() before the
kfree() when applying?

(Or switch reg->ext->name to ext->name)

>  			power_supply_update_sysfs_and_hwmon(psy);
>  			return;
>  		}

[..]


Thanks,
Thomas