linux-kernel - Re: [PATCH v1 09/10] hwmon: adt7x10: use devm_hwmon_device_register_with

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3532ca31-55e9-b000-ec18-910197f13c4f@roeck-us.net>
Date:   Tue, 21 Dec 2021 07:34:42 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Cosmin Tanislav <demonsingur@...il.com>
Cc:     cosmin.tanislav@...log.com, Jean Delvare <jdelvare@...e.com>,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 09/10] hwmon: adt7x10: use
 devm_hwmon_device_register_with_info

On 12/21/21 4:39 AM, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@...log.com>
> 
> To simplify core driver remove function.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@...log.com>
> ---
>   drivers/hwmon/adt7x10.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
> index dd4901299590..c03805c72906 100644
> --- a/drivers/hwmon/adt7x10.c
> +++ b/drivers/hwmon/adt7x10.c
> @@ -54,7 +54,6 @@
>   /* Each client has this additional data */
>   struct adt7x10_data {
>   	const struct adt7x10_ops *ops;
> -	struct device		*hwmon_dev;
>   	struct device		*bus_dev;
>   	struct mutex		update_lock;
>   	u8			config;
> @@ -430,8 +429,8 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   	if (ret)
>   		goto exit_restore;
>   
> -	hdev = hwmon_device_register_with_info(dev, name, data,
> -					       &adt7x10_chip_info, NULL);
> +	hdev = devm_hwmon_device_register_with_info(dev, name, data,
> +						    &adt7x10_chip_info, NULL);
>   
>   	if (IS_ERR(hdev)) {
>   		ret = PTR_ERR(hdev);
> @@ -445,15 +444,11 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   						IRQF_ONESHOT,
>   						dev_name(dev), hdev);
>   		if (ret)
> -			goto exit_hwmon_device_unregister;
> +			goto exit_restore;
>   	}
>   
> -	data->hwmon_dev = hdev;
> -
>   	return 0;
>   
> -exit_hwmon_device_unregister:
> -	hwmon_device_unregister(hdev);
>   exit_restore:
>   	adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
>   	return ret;
> @@ -464,7 +459,6 @@ void adt7x10_remove(struct device *dev, int irq)
>   {
>   	struct adt7x10_data *data = dev_get_drvdata(dev);
>   
> -	hwmon_device_unregister(data->hwmon_dev);
>   	if (data->oldconfig != data->config)
>   		adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);

This doesn't work as-is because the hwmon device still exists at this point
and at least in theory userspace could still write into the device
after the old configuration was restored.

To fix this, you'll need a preceding patch to introduce adt7x10_restore_config()
or similar, and call it through devm_add_action_or_reset() after updating the
chip configuration. You can then drop the restore code from here and from
the exist_restore code in the probe function.
After that, you can use devm_hwmon_device_register_with_info() and drop the
remove function entirely.

Guenter