linux-kernel - Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0hXHgyCKoEOMTtp0c_yu__vGGDcPnqaUML2Xg7hyJWc3g@mail.gmail.com>
Date: Thu, 3 Jul 2025 13:09:21 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: "lihuisong (C)" <lihuisong@...wei.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, lenb@...nel.org, linux-acpi@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linuxarm@...wei.com, 
	jonathan.cameron@...wei.com, zhanjie9@...ilicon.com, zhenglifeng1@...wei.com, 
	yubowen8@...wei.com, liuyonglong@...wei.com
Subject: Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init

On Thu, Jul 3, 2025 at 8:23 AM lihuisong (C) <lihuisong@...wei.com> wrote:
>
> Hi,
>
> Thanks for your review.
>
>
> 在 2025/7/3 1:42, Rafael J. Wysocki 写道:
> > On Thu, Jun 19, 2025 at 8:13 AM Huisong Li <lihuisong@...wei.com> wrote:
> >> There are two resource rollback issues in acpi_processor_power_init:
> >> 1> Do not unregister acpi_idle_driver when do kzalloc failed.
> >> 2> Do not free cpuidle device memory when register cpuidle device failed.
> >>
> >> Signed-off-by: Huisong Li <lihuisong@...wei.com>
> >> ---
> >>   drivers/acpi/processor_idle.c | 24 +++++++++++++++++-------
> >>   1 file changed, 17 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >> index 2c2dc559e0f8..3548ab9dac9e 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -1392,8 +1392,10 @@ int acpi_processor_power_init(struct acpi_processor *pr)
> >>                  }
> >>
> >>                  dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >> -               if (!dev)
> >> -                       return -ENOMEM;
> >> +               if (!dev) {
> >> +                       retval = -ENOMEM;
> >> +                       goto unregister_driver;
> > No, unregistering the driver here is pointless.
> I don't quite understand why here is pointless. Can you explain it?

When this function is run for another CPU, it will attempt to register
the driver again if it is unregistered here.

Quite frankly, the driver should be registered before running this
function because it is a CPU hotplug callback and registering a
cpuidle driver from within it is quite questionable.

Alternatively, it can be registered when all of the CPUs have been brought up.

> >
> >> +               }
> >>                  per_cpu(acpi_cpuidle_device, pr->id) = dev;
> >>
> >>                  acpi_processor_setup_cpuidle_dev(pr, dev);
> >> @@ -1402,14 +1404,22 @@ int acpi_processor_power_init(struct acpi_processor *pr)
> >>                   * must already be registered before registering device
> >>                   */
> >>                  retval = cpuidle_register_device(dev);
> >> -               if (retval) {
> >> -                       if (acpi_processor_registered == 0)
> >> -                               cpuidle_unregister_driver(&acpi_idle_driver);
> > Pretty much the same as here.
> >
> > It would be good to clean up dev here though.
> It is ok if above is pointless.
> >
> >> -                       return retval;
> >> -               }
> >> +               if (retval)
> >> +                       goto free_cpuidle_device;
> >> +
> >>                  acpi_processor_registered++;
> >>          }
> >>          return 0;
> >> +
> >> +free_cpuidle_device:
> >> +       per_cpu(acpi_cpuidle_device, pr->id) = NULL;
> >> +       kfree(dev);
> >> +
> >> +unregister_driver:
> >> +       if (acpi_processor_registered == 0)
> >> +               cpuidle_unregister_driver(&acpi_idle_driver);
> >> +
> >> +       return retval;
> >>   }
> >>
> >>   int acpi_processor_power_exit(struct acpi_processor *pr)
> >> --
> >> 2.33.0
> >>