hwmon: (dell-smm) Simplify with cleanup.h

[PATCH] hwmon: (dell-smm) Simplify with cleanup.h

Posted by Krzysztof Kozlowski 1 year, 7 months ago

Allocate memory, which is being freed at end of the scope, to make the
code a bit simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 0362a13f6525..e72e26db6e10 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -14,6 +14,7 @@
 
 #include <linux/acpi.h>
 #include <linux/capability.h>
+#include <linux/cleanup.h>
 #include <linux/cpu.h>
 #include <linux/ctype.h>
 #include <linux/delay.h>
@@ -1095,9 +1096,9 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
 	struct thermal_cooling_device *cdev;
 	struct dell_smm_cooling_data *cdata;
 	int ret = 0;
-	char *name;
 
-	name = kasprintf(GFP_KERNEL, "dell-smm-fan%u", fan_num + 1);
+	char *name __free(kfree) = kasprintf(GFP_KERNEL, "dell-smm-fan%u",
+					     fan_num + 1);
 	if (!name)
 		return -ENOMEM;
 
@@ -1115,8 +1116,6 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
 		ret = -ENOMEM;
 	}
 
-	kfree(name);
-
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH] hwmon: (dell-smm) Simplify with cleanup.h

Posted by Guenter Roeck 1 year, 7 months ago

On 7/3/24 01:31, Krzysztof Kozlowski wrote:
> Allocate memory, which is being freed at end of the scope, to make the
> code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>   drivers/hwmon/dell-smm-hwmon.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 0362a13f6525..e72e26db6e10 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -14,6 +14,7 @@
>   
>   #include <linux/acpi.h>
>   #include <linux/capability.h>
> +#include <linux/cleanup.h>
>   #include <linux/cpu.h>
>   #include <linux/ctype.h>
>   #include <linux/delay.h>
> @@ -1095,9 +1096,9 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
>   	struct thermal_cooling_device *cdev;
>   	struct dell_smm_cooling_data *cdata;
>   	int ret = 0;
> -	char *name;
>   
> -	name = kasprintf(GFP_KERNEL, "dell-smm-fan%u", fan_num + 1);
> +	char *name __free(kfree) = kasprintf(GFP_KERNEL, "dell-smm-fan%u",
> +					     fan_num + 1);
>   	if (!name)
>   		return -ENOMEM;
>   
> @@ -1115,8 +1116,6 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
>   		ret = -ENOMEM;
>   	}
>   
> -	kfree(name);
> -
>   	return ret;
>   }
>   

If you really want to clean this up, just use
	char name[32];
	...
	snprintf(name, sizeof(name), "dell-smm-fan%u", fan_num + 1);

I don't see the point of all this complexity.

Guenter

Re: [PATCH] hwmon: (dell-smm) Simplify with cleanup.h

Posted by Pali Rohár 1 year, 7 months ago

On Wednesday 03 July 2024 11:52:14 Guenter Roeck wrote:
> On 7/3/24 01:31, Krzysztof Kozlowski wrote:
> > Allocate memory, which is being freed at end of the scope, to make the
> > code a bit simpler.
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > ---
> >   drivers/hwmon/dell-smm-hwmon.c | 7 +++----
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > index 0362a13f6525..e72e26db6e10 100644
> > --- a/drivers/hwmon/dell-smm-hwmon.c
> > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > @@ -14,6 +14,7 @@
> >   #include <linux/acpi.h>
> >   #include <linux/capability.h>
> > +#include <linux/cleanup.h>
> >   #include <linux/cpu.h>
> >   #include <linux/ctype.h>
> >   #include <linux/delay.h>
> > @@ -1095,9 +1096,9 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
> >   	struct thermal_cooling_device *cdev;
> >   	struct dell_smm_cooling_data *cdata;
> >   	int ret = 0;
> > -	char *name;
> > -	name = kasprintf(GFP_KERNEL, "dell-smm-fan%u", fan_num + 1);
> > +	char *name __free(kfree) = kasprintf(GFP_KERNEL, "dell-smm-fan%u",
> > +					     fan_num + 1);
> >   	if (!name)
> >   		return -ENOMEM;
> > @@ -1115,8 +1116,6 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
> >   		ret = -ENOMEM;
> >   	}
> > -	kfree(name);
> > -
> >   	return ret;
> >   }
> 
> If you really want to clean this up, just use
> 	char name[32];
> 	...
> 	snprintf(name, sizeof(name), "dell-smm-fan%u", fan_num + 1);
> 
> I don't see the point of all this complexity.
> 
> Guenter
> 

Lets first ask a question: And what the problem we are solving there?
I do not see any memory leak here, it is neither mentioned in the commit
message. So I think that there is no real problem, and code has just
clear and explicit alloc/free pattern.

On the other hand proposed change with __free does not make it simpler.
It has still same complexity, plus magic around.

snprintf with stack allocation at the first glance looks simpler.

But has a problem that if in future the device name will change then it
would be required also check (and maybe modify) size of stack buffer. In
its usage you are specifying pair <sizeof(name), "dell-smm-fan%u"> which
has size not related to the string. But something more common sense
would be to specify pair <32,"dell-smm-fan%u"> which could say that it
is always maximally 32 (and you can easily check if the string is not
going to be larger). So for long term maintenance this is maybe worse.

What could be the real cleanup (if some is really needed) is to use
kasprintf variant which allocates buffer on the stack. But I do not know
if such printf-alloca function variant is available for us.

E.g.: const char *name = alloca_snprintf(32, "dell-smm-fan%u", fan_num + 1);

Or maybe just alloca_snprintf("dell-smm-fan%u", fan_num + 1) and some
sanitizer can calculate that the function never allocates more than some
sane size (because there is fixed string of few chars and %u which needs
maximally 10 bytes).

And anyway, explicit specification of the buffer size is lot of times
reason for overflows (because is specified incorrectly). Why cannot
compiler / library / etc... in year 2024 compute the correct required
buffer size of us automatically? Ah :-(

Re: [PATCH] hwmon: (dell-smm) Simplify with cleanup.h

Posted by Guenter Roeck 1 year, 7 months ago

On 7/3/24 12:16, Pali Rohár wrote:
> On Wednesday 03 July 2024 11:52:14 Guenter Roeck wrote:
>> On 7/3/24 01:31, Krzysztof Kozlowski wrote:
>>> Allocate memory, which is being freed at end of the scope, to make the
>>> code a bit simpler.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> ---
>>>    drivers/hwmon/dell-smm-hwmon.c | 7 +++----
>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>> index 0362a13f6525..e72e26db6e10 100644
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -14,6 +14,7 @@
>>>    #include <linux/acpi.h>
>>>    #include <linux/capability.h>
>>> +#include <linux/cleanup.h>
>>>    #include <linux/cpu.h>
>>>    #include <linux/ctype.h>
>>>    #include <linux/delay.h>
>>> @@ -1095,9 +1096,9 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
>>>    	struct thermal_cooling_device *cdev;
>>>    	struct dell_smm_cooling_data *cdata;
>>>    	int ret = 0;
>>> -	char *name;
>>> -	name = kasprintf(GFP_KERNEL, "dell-smm-fan%u", fan_num + 1);
>>> +	char *name __free(kfree) = kasprintf(GFP_KERNEL, "dell-smm-fan%u",
>>> +					     fan_num + 1);
>>>    	if (!name)
>>>    		return -ENOMEM;
>>> @@ -1115,8 +1116,6 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
>>>    		ret = -ENOMEM;
>>>    	}
>>> -	kfree(name);
>>> -
>>>    	return ret;
>>>    }
>>
>> If you really want to clean this up, just use
>> 	char name[32];
>> 	...
>> 	snprintf(name, sizeof(name), "dell-smm-fan%u", fan_num + 1);
>>
>> I don't see the point of all this complexity.
>>
>> Guenter
>>
> 
> Lets first ask a question: And what the problem we are solving there?

None.

> I do not see any memory leak here, it is neither mentioned in the commit
> message. So I think that there is no real problem, and code has just
> clear and explicit alloc/free pattern.
> 
Correct.

> On the other hand proposed change with __free does not make it simpler.
> It has still same complexity, plus magic around.
> 
Again correct.

> snprintf with stack allocation at the first glance looks simpler.
> 
> But has a problem that if in future the device name will change then it
> would be required also check (and maybe modify) size of stack buffer. In
> its usage you are specifying pair <sizeof(name), "dell-smm-fan%u"> which
> has size not related to the string. But something more common sense
> would be to specify pair <32,"dell-smm-fan%u"> which could say that it
> is always maximally 32 (and you can easily check if the string is not
> going to be larger). So for long term maintenance this is maybe worse.
> 

Agreed, but I very much prefer that over the complexity introduced with
the "free" magic.

Unfortunately, once framework code such as the free magic is introduced, it
doesn't help to just say "please go away", because people will just keep
submitting patches like this one which don't improve the code but still take
review time.

Guenter

Re: [PATCH] hwmon: (dell-smm) Simplify with cleanup.h

Posted by Christophe JAILLET 1 year, 7 months ago

Le 03/07/2024 à 10:31, Krzysztof Kozlowski a écrit :
> Allocate memory, which is being freed at end of the scope, to make the
> code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>   drivers/hwmon/dell-smm-hwmon.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 0362a13f6525..e72e26db6e10 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -14,6 +14,7 @@
>   
>   #include <linux/acpi.h>
>   #include <linux/capability.h>
> +#include <linux/cleanup.h>
>   #include <linux/cpu.h>
>   #include <linux/ctype.h>
>   #include <linux/delay.h>
> @@ -1095,9 +1096,9 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
>   	struct thermal_cooling_device *cdev;
>   	struct dell_smm_cooling_data *cdata;
>   	int ret = 0;
> -	char *name;
>   
> -	name = kasprintf(GFP_KERNEL, "dell-smm-fan%u", fan_num + 1);
> +	char *name __free(kfree) = kasprintf(GFP_KERNEL, "dell-smm-fan%u",
> +					     fan_num + 1);
>   	if (!name)
>   		return -ENOMEM;
>   
> @@ -1115,8 +1116,6 @@ static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
>   		ret = -ENOMEM;
>   	}
>   
> -	kfree(name);
> -
>   	return ret;
>   }
>   

Hi,

going one step further, this could even be:

	cdata = devm_kmalloc(dev, sizeof(*cdata), GFP_KERNEL);
	if (!cdata)
		return -ENOMEM;

	cdata->fan_num = fan_num;
	cdata->data = data;
	cdev = devm_thermal_of_cooling_device_register(dev, NULL, name, cdata, 
&dell_smm_cooling_ops);
	if (IS_ERR(cdev)) {
		devm_kfree(dev, cdata);
		return PTR_ERR(cdev);
	}

	return 0;


This reduces indentation and is slightly more common when testing after 
devm_kmalloc()


Just my 2c,

CJ