diff mbox series

[v9,05/19] ACPI: processor: Fix memory leaks in error paths of processor_add()

Message ID 20240430142434.10471-6-Jonathan.Cameron@huawei.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series ACPI/arm64: add support for virtual cpu hotplug | expand

Commit Message

Jonathan Cameron April 30, 2024, 2:24 p.m. UTC
If acpi_processor_get_info() returned an error, pr and the associated
pr->throttling.shared_cpu_map were leaked.

The unwind code was in the wrong order wrt to setup, relying on
some unwind actions having no affect (clearing variables that were
never set etc).  That makes it harder to reason about so reorder
and add appropriate labels to only undo what was actually set up
in the first place.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v9: New patch in response to Gavin noticing a memory leak later in the
    series.
---
 drivers/acpi/acpi_processor.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Gavin Shan May 1, 2024, 10:19 a.m. UTC | #1
On 5/1/24 00:24, Jonathan Cameron wrote:
> If acpi_processor_get_info() returned an error, pr and the associated
> pr->throttling.shared_cpu_map were leaked.
> 
> The unwind code was in the wrong order wrt to setup, relying on
> some unwind actions having no affect (clearing variables that were
> never set etc).  That makes it harder to reason about so reorder
> and add appropriate labels to only undo what was actually set up
> in the first place.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> v9: New patch in response to Gavin noticing a memory leak later in the
>      series.
> ---
>   drivers/acpi/acpi_processor.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>
Rafael J. Wysocki May 7, 2024, 6:57 p.m. UTC | #2
On Tue, Apr 30, 2024 at 4:27 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> If acpi_processor_get_info() returned an error, pr and the associated
> pr->throttling.shared_cpu_map were leaked.
>
> The unwind code was in the wrong order wrt to setup, relying on
> some unwind actions having no affect (clearing variables that were
> never set etc).  That makes it harder to reason about so reorder
> and add appropriate labels to only undo what was actually set up
> in the first place.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thank you!

>
> ---
> v9: New patch in response to Gavin noticing a memory leak later in the
>     series.
> ---
>  drivers/acpi/acpi_processor.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 5f062806ca40..16e36e55a560 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -393,7 +393,7 @@ static int acpi_processor_add(struct acpi_device *device,
>
>         result = acpi_processor_get_info(device);
>         if (result) /* Processor is not physically present or unavailable */
> -               return result;
> +               goto err_clear_driver_data;
>
>         BUG_ON(pr->id >= nr_cpu_ids);
>
> @@ -408,7 +408,7 @@ static int acpi_processor_add(struct acpi_device *device,
>                         "BIOS reported wrong ACPI id %d for the processor\n",
>                         pr->id);
>                 /* Give up, but do not abort the namespace scan. */
> -               goto err;
> +               goto err_clear_driver_data;
>         }
>         /*
>          * processor_device_array is not cleared on errors to allow buggy BIOS
> @@ -420,12 +420,12 @@ static int acpi_processor_add(struct acpi_device *device,
>         dev = get_cpu_device(pr->id);
>         if (!dev) {
>                 result = -ENODEV;
> -               goto err;
> +               goto err_clear_per_cpu;
>         }
>
>         result = acpi_bind_one(dev, device);
>         if (result)
> -               goto err;
> +               goto err_clear_per_cpu;
>
>         pr->dev = dev;
>
> @@ -436,10 +436,11 @@ static int acpi_processor_add(struct acpi_device *device,
>         dev_err(dev, "Processor driver could not be attached\n");
>         acpi_unbind_one(dev);
>
> - err:
> -       free_cpumask_var(pr->throttling.shared_cpu_map);
> -       device->driver_data = NULL;
> + err_clear_per_cpu:
>         per_cpu(processors, pr->id) = NULL;
> + err_clear_driver_data:
> +       device->driver_data = NULL;
> +       free_cpumask_var(pr->throttling.shared_cpu_map);
>   err_free_pr:
>         kfree(pr);
>         return result;
> --
> 2.39.2
>
diff mbox series

Patch

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 5f062806ca40..16e36e55a560 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -393,7 +393,7 @@  static int acpi_processor_add(struct acpi_device *device,
 
 	result = acpi_processor_get_info(device);
 	if (result) /* Processor is not physically present or unavailable */
-		return result;
+		goto err_clear_driver_data;
 
 	BUG_ON(pr->id >= nr_cpu_ids);
 
@@ -408,7 +408,7 @@  static int acpi_processor_add(struct acpi_device *device,
 			"BIOS reported wrong ACPI id %d for the processor\n",
 			pr->id);
 		/* Give up, but do not abort the namespace scan. */
-		goto err;
+		goto err_clear_driver_data;
 	}
 	/*
 	 * processor_device_array is not cleared on errors to allow buggy BIOS
@@ -420,12 +420,12 @@  static int acpi_processor_add(struct acpi_device *device,
 	dev = get_cpu_device(pr->id);
 	if (!dev) {
 		result = -ENODEV;
-		goto err;
+		goto err_clear_per_cpu;
 	}
 
 	result = acpi_bind_one(dev, device);
 	if (result)
-		goto err;
+		goto err_clear_per_cpu;
 
 	pr->dev = dev;
 
@@ -436,10 +436,11 @@  static int acpi_processor_add(struct acpi_device *device,
 	dev_err(dev, "Processor driver could not be attached\n");
 	acpi_unbind_one(dev);
 
- err:
-	free_cpumask_var(pr->throttling.shared_cpu_map);
-	device->driver_data = NULL;
+ err_clear_per_cpu:
 	per_cpu(processors, pr->id) = NULL;
+ err_clear_driver_data:
+	device->driver_data = NULL;
+	free_cpumask_var(pr->throttling.shared_cpu_map);
  err_free_pr:
 	kfree(pr);
 	return result;