linux-kernel - Re: [PATCH v1 03/10] hwmon: adt7x10: use devm_request_threaded

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <82012996-b266-873a-699a-26d77bcb7e5a@roeck-us.net>
Date:   Tue, 21 Dec 2021 08:08:58 -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 03/10] hwmon: adt7x10: use devm_request_threaded_irq

On 12/21/21 4:39 AM, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@...log.com>
> 
> To simplify the core driver remove function.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@...log.com>
> ---
>   drivers/hwmon/adt7x10.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
> index dbe9f1ad7db0..48adc0344e88 100644
> --- a/drivers/hwmon/adt7x10.c
> +++ b/drivers/hwmon/adt7x10.c
> @@ -402,9 +402,11 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   	}
>   
>   	if (irq > 0) {
> -		ret = request_threaded_irq(irq, NULL, adt7x10_irq_handler,
> -				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				dev_name(dev), dev);
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +						adt7x10_irq_handler,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						dev_name(dev), dev);
>   		if (ret)
>   			goto exit_hwmon_device_unregister;
>   	}
> @@ -425,9 +427,6 @@ void adt7x10_remove(struct device *dev, int irq)
>   {
>   	struct adt7x10_data *data = dev_get_drvdata(dev);
>   
> -	if (irq > 0)
> -		free_irq(irq, dev);
> -
>   	hwmon_device_unregister(data->hwmon_dev);
>   	sysfs_remove_group(&dev->kobj, &adt7x10_group);
>   	if (data->oldconfig != data->config)
> 

This will keep the interrupt running after the hwmon device was removed,
and after the configuration was restored. If an interrupt occurs at that time,
the handler may potentially access released data. I don't mind making those
changes, but the patches will have to be well sequenced to ensure that each
patch on its own doesn't leave a crippled driver behind. Again, "oh, this will
be ok after the entire series was applied" is not acceptable.

Thanks,
Guenter