[PATCH v1 01/10] ACPI: fan: Fix memory leak due to leftover devm_kcalloc() argument

Posted by Rafael J. Wysocki 2 weeks ago

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

An ACPI device object's dev field in passed as the first argument to
devm_kcalloc() in acpi_fan_get_fps() which is incorrect and leads to
a memory leak on driver probe errors and removal because the driver
is not bound to that ACPI device.

Address this by replacing that pointer with a pointer to the device the
driver is actually bound to.

While at it, drop a redundant error message after a memory allocation
failure (that also gets printed relative to the ACPI device).

Fixes: d91a1d129b63 ("ACPI: fan: Use platform device for devres-related actions")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/fan_core.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/fan_core.c b/drivers/acpi/fan_core.c
index 624d0736b581..a31d7beca5d0 100644
--- a/drivers/acpi/fan_core.c
+++ b/drivers/acpi/fan_core.c
@@ -284,7 +284,7 @@ static int acpi_fan_speed_cmp(const void *a, const void *b)
 	return fps1->speed - fps2->speed;
 }
 
-static int acpi_fan_get_fps(struct acpi_device *device)
+static int acpi_fan_get_fps(struct device *dev, struct acpi_device *device)
 {
 	struct acpi_fan *fan = acpi_driver_data(device);
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -304,11 +304,8 @@ static int acpi_fan_get_fps(struct acpi_device *device)
 	}
 
 	fan->fps_count = obj->package.count - 1; /* minus revision field */
-	fan->fps = devm_kcalloc(&device->dev,
-				fan->fps_count, sizeof(struct acpi_fan_fps),
-				GFP_KERNEL);
+	fan->fps = devm_kcalloc(dev, fan->fps_count, sizeof(*fan->fps), GFP_KERNEL);
 	if (!fan->fps) {
-		dev_err(&device->dev, "Not enough memory\n");
 		status = -ENOMEM;
 		goto err;
 	}
@@ -522,7 +519,7 @@ static int acpi_fan_probe(struct platform_device *pdev)
 		if (result)
 			return result;
 
-		result = acpi_fan_get_fps(device);
+		result = acpi_fan_get_fps(&pdev->dev, device);
 		if (result)
 			return result;
 	}
-- 
2.51.0

Re: [PATCH v1 01/10] ACPI: fan: Fix memory leak due to leftover devm_kcalloc() argument

Posted by Armin Wolf 1 week, 6 days ago

Am 11.09.26 um 15:00 schrieb Rafael J. Wysocki:

> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>
> An ACPI device object's dev field in passed as the first argument to
> devm_kcalloc() in acpi_fan_get_fps() which is incorrect and leads to
> a memory leak on driver probe errors and removal because the driver
> is not bound to that ACPI device.
>
> Address this by replacing that pointer with a pointer to the device the
> driver is actually bound to.
>
> While at it, drop a redundant error message after a memory allocation
> failure (that also gets printed relative to the ACPI device).

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Fixes: d91a1d129b63 ("ACPI: fan: Use platform device for devres-related actions")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/fan_core.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/fan_core.c b/drivers/acpi/fan_core.c
> index 624d0736b581..a31d7beca5d0 100644
> --- a/drivers/acpi/fan_core.c
> +++ b/drivers/acpi/fan_core.c
> @@ -284,7 +284,7 @@ static int acpi_fan_speed_cmp(const void *a, const void *b)
>   	return fps1->speed - fps2->speed;
>   }
>   
> -static int acpi_fan_get_fps(struct acpi_device *device)
> +static int acpi_fan_get_fps(struct device *dev, struct acpi_device *device)
>   {
>   	struct acpi_fan *fan = acpi_driver_data(device);
>   	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -304,11 +304,8 @@ static int acpi_fan_get_fps(struct acpi_device *device)
>   	}
>   
>   	fan->fps_count = obj->package.count - 1; /* minus revision field */
> -	fan->fps = devm_kcalloc(&device->dev,
> -				fan->fps_count, sizeof(struct acpi_fan_fps),
> -				GFP_KERNEL);
> +	fan->fps = devm_kcalloc(dev, fan->fps_count, sizeof(*fan->fps), GFP_KERNEL);
>   	if (!fan->fps) {
> -		dev_err(&device->dev, "Not enough memory\n");
>   		status = -ENOMEM;
>   		goto err;
>   	}
> @@ -522,7 +519,7 @@ static int acpi_fan_probe(struct platform_device *pdev)
>   		if (result)
>   			return result;
>   
> -		result = acpi_fan_get_fps(device);
> +		result = acpi_fan_get_fps(&pdev->dev, device);
>   		if (result)
>   			return result;
>   	}

Re: [PATCH v1 01/10] ACPI: fan: Fix memory leak due to leftover devm_kcalloc() argument

Posted by Andy Shevchenko 1 week, 6 days ago

On Fri, Sep 11, 2026 at 03:00:51PM +0200, Rafael J. Wysocki wrote:

> An ACPI device object's dev field in passed as the first argument to
> devm_kcalloc() in acpi_fan_get_fps() which is incorrect and leads to
> a memory leak on driver probe errors and removal because the driver
> is not bound to that ACPI device.
> 
> Address this by replacing that pointer with a pointer to the device the
> driver is actually bound to.
> 
> While at it, drop a redundant error message after a memory allocation
> failure (that also gets printed relative to the ACPI device).

...

>  	fan->fps_count = obj->package.count - 1; /* minus revision field */
> -	fan->fps = devm_kcalloc(&device->dev,
> -				fan->fps_count, sizeof(struct acpi_fan_fps),
> -				GFP_KERNEL);
> +	fan->fps = devm_kcalloc(dev, fan->fps_count, sizeof(*fan->fps), GFP_KERNEL);
>  	if (!fan->fps) {
> -		dev_err(&device->dev, "Not enough memory\n");
>  		status = -ENOMEM;
>  		goto err;

I was about ranting on goto after devm_*(), but looking at the context,
I understand why it's not a problem. While at it, a side note: perhaps it makes
sense to use ACPI_FREE(obj) instead of kfree()? Or even better to have __free()
version of it, so we can declare the object with autoclean.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] ACPI: fan: Fix memory leak due to leftover devm_kcalloc() argument

Posted by Rafael J. Wysocki (Intel) 1 week, 5 days ago

On Fri, Sep 11, 2026 at 6:29 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 11, 2026 at 03:00:51PM +0200, Rafael J. Wysocki wrote:
>
> > An ACPI device object's dev field in passed as the first argument to
> > devm_kcalloc() in acpi_fan_get_fps() which is incorrect and leads to
> > a memory leak on driver probe errors and removal because the driver
> > is not bound to that ACPI device.
> >
> > Address this by replacing that pointer with a pointer to the device the
> > driver is actually bound to.
> >
> > While at it, drop a redundant error message after a memory allocation
> > failure (that also gets printed relative to the ACPI device).
>
> ...
>
> >       fan->fps_count = obj->package.count - 1; /* minus revision field */
> > -     fan->fps = devm_kcalloc(&device->dev,
> > -                             fan->fps_count, sizeof(struct acpi_fan_fps),
> > -                             GFP_KERNEL);
> > +     fan->fps = devm_kcalloc(dev, fan->fps_count, sizeof(*fan->fps), GFP_KERNEL);
> >       if (!fan->fps) {
> > -             dev_err(&device->dev, "Not enough memory\n");
> >               status = -ENOMEM;
> >               goto err;
>
> I was about ranting on goto after devm_*(), but looking at the context,
> I understand why it's not a problem. While at it, a side note: perhaps it makes
> sense to use ACPI_FREE(obj) instead of kfree()?

It should be ACPI_FREE() strictly speaking.

> Or even better to have __free() version of it, so we can declare the object with autoclean.

That one is a bit tricky, but I think I have an idea how to do it.
I'll post something next week.