| 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
| ||
|
Message-ID: <CAJZ5v0gOi5kUKVWSwaPW=4Kmjkj1kj=nzaZ0jTa8fAAy32ZytA@mail.gmail.com>
Date: Wed, 23 Jul 2025 15:35:24 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Huisong Li <lihuisong@...wei.com>
Cc: 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 v2] ACPI: processor: idle: Fix resource leak and potential
concurrent in acpi_processor_power_init()
On Wed, Jul 23, 2025 at 2:10 PM Huisong Li <lihuisong@...wei.com> wrote:
>
> There are three kind of issues:
> 1> There are two resource leak issues in acpi_processor_power_init:
> a> Don't unregister acpi_idle_driver when do kzalloc failed.
This is not a resource leak. As I said before, the driver need not be
unregistered on a memory allocation failure.
> b> Don't free cpuidle device memory when register cpuidle device failed.
This is a separate minor issue that needs to be addressed by a separate patch.
> 2> There isn't lock to prevent the global acpi_processor_registered, which
> may lead to concurrent register cpuidle driver.
That's not obvious because in principle the code in question is only
run during initialization which is serialized.
In theory, it could run in parallel CPU online, but that at least is
not default behavior AFAICS.
In any case, if you claim something like this, it is advisable to
mention a specific scenario in which the race in question can happen.
> 3> The cpuidle driver should be registered in advance when all of the CPUs
> have been brought up instead of being in a CPU hotplug callback.
The "in advance" piece above is rather confusing and it can be dropped
without changing the meaning of the rest of the sentence.
> To solve these issues, so add a new function to initialize acpi_idle_driver
> based on the power management information of an available CPU and register
> cpuidle driver in acpi_processor_driver_init().
I think that the main problem here is that the cpuidle driver is
registered from within a CPU hotplug callback, which is questionable
and confusing. Usually, however, this doesn't lead to any functional
issues AFAICS.
> Signed-off-by: Huisong Li <lihuisong@...wei.com>
> ---
> v2: register cpuidle driver in advance when all of the CPUs have been
> brought up.
> v1 link: https://patchwork.kernel.org/project/linux-acpi/patch/20250619061327.1674384-1-lihuisong@huawei.com/
> ---
> drivers/acpi/processor_driver.c | 5 +++
> drivers/acpi/processor_idle.c | 71 ++++++++++++++++++++++-----------
> include/acpi/processor.h | 9 +++++
> 3 files changed, 62 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 65e779be64ff..ff944c93b6ff 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -263,6 +263,10 @@ static int __init acpi_processor_driver_init(void)
> if (result < 0)
> return result;
>
> + result = acpi_processor_register_idle_driver();
> + if (result)
> + pr_info("register idle driver failed, ret = %d.\n", result);
This registers the cpuidle driver before registering cpuidle devices
for all CPUs.
It would be better to make acpi_processor_register_idle_driver() print
the diagnostic message on failures and then it won't need to return a
value.
Note that it may fail if intel_idle is already registered, for
example, so the message should rather be a debug-level one.
> +
> result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> "acpi/cpu-drv:online",
> acpi_soft_cpu_online, NULL);
> @@ -301,6 +305,7 @@ static void __exit acpi_processor_driver_exit(void)
>
> cpuhp_remove_state_nocalls(hp_online);
> cpuhp_remove_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD);
> + acpi_processor_unregister_idle_driver();
> driver_unregister(&acpi_processor_driver);
> }
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 2c2dc559e0f8..2408f1076631 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1360,7 +1360,52 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> return 0;
> }
>
> -static int acpi_processor_registered;
> +int acpi_processor_register_idle_driver(void)
> +{
> + struct acpi_processor *pr;
> + int cpu;
> + int ret;
The ret variable needs to be initialized here or tools will complain,
and so it may be initialized to -ENODEV:
int ret = -ENODEV;
> +
> + /*
> + * Acpi idle driver is used by all possible CPUs.
> + * Install the idle handler by the processor power info of one in them.
> + * Note that we use previously set idle handler will be used on
> + * platforms that only support C1.
> + */
> + for_each_cpu(cpu, (struct cpumask *)cpu_possible_mask) {
> + pr = per_cpu(processors, cpu);
> + if (pr == NULL)
"if (!pr)" please.
> + continue;
> +
> + ret = acpi_processor_get_power_info(pr);
if (ret)
continue;
> + if (!ret) {
> + pr->flags.power_setup_done = 1;
I think this is set here to prevent the subsequent
acpi_processor_setup_cpuidle_states() call from bailing out, but is
this not too early to set it?
> + break;
> + }
> + }
> +
> + if (unlikely(!pr))
> + return -ENODEV;
This is unnecessary if ret is initialized to -ENODEV;
> +
> + if (ret) {
> + pr_err("%s get power information failed.\n",
> + acpi_idle_driver.name);
This message is confusing at best. It should be something like "No
ACPI power information for any CPUs" and the driver name in it has no
purpose.
> + return ret;
> + }
> +
> + acpi_processor_setup_cpuidle_states(pr);
I'd call this in the loop right before breaking out of it, so the
scope of pr is clear.
> + ret = cpuidle_register_driver(&acpi_idle_driver);
> + if (ret)
Print a diagnostic message here and do not return a value (ie. make
the function void).
> + return ret;
> +
> + pr_debug("%s registered with cpuidle\n", acpi_idle_driver.name);
> + return 0;
> +}
> +
> +void acpi_processor_unregister_idle_driver(void)
> +{
> + cpuidle_unregister_driver(&acpi_idle_driver);
> +}
>
> int acpi_processor_power_init(struct acpi_processor *pr)
> {
> @@ -1375,22 +1420,7 @@ int acpi_processor_power_init(struct acpi_processor *pr)
> if (!acpi_processor_get_power_info(pr))
> pr->flags.power_setup_done = 1;
>
> - /*
> - * Install the idle handler if processor power management is supported.
> - * Note that we use previously set idle handler will be used on
> - * platforms that only support C1.
> - */
> if (pr->flags.power) {
> - /* Register acpi_idle_driver if not already registered */
> - if (!acpi_processor_registered) {
> - acpi_processor_setup_cpuidle_states(pr);
> - retval = cpuidle_register_driver(&acpi_idle_driver);
> - if (retval)
> - return retval;
> - pr_debug("%s registered with cpuidle\n",
> - acpi_idle_driver.name);
> - }
> -
> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> if (!dev)
> return -ENOMEM;
> @@ -1403,11 +1433,10 @@ int acpi_processor_power_init(struct acpi_processor *pr)
> */
> retval = cpuidle_register_device(dev);
> if (retval) {
> - if (acpi_processor_registered == 0)
> - cpuidle_unregister_driver(&acpi_idle_driver);
> + per_cpu(acpi_cpuidle_device, pr->id) = NULL;
> + kfree(dev);
These two lines should be added in a separate patch.
> return retval;
> }
> - acpi_processor_registered++;
> }
> return 0;
> }
> @@ -1421,10 +1450,6 @@ int acpi_processor_power_exit(struct acpi_processor *pr)
>
> if (pr->flags.power) {
> cpuidle_unregister_device(dev);
> - acpi_processor_registered--;
> - if (acpi_processor_registered == 0)
> - cpuidle_unregister_driver(&acpi_idle_driver);
> -
> kfree(dev);
> }
>
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index d0eccbd920e5..3cb41a3f2d9a 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -423,6 +423,8 @@ int acpi_processor_power_init(struct acpi_processor *pr);
> int acpi_processor_power_exit(struct acpi_processor *pr);
> int acpi_processor_power_state_has_changed(struct acpi_processor *pr);
> int acpi_processor_hotplug(struct acpi_processor *pr);
> +int acpi_processor_register_idle_driver(void);
> +void acpi_processor_unregister_idle_driver(void);
> #else
> static inline int acpi_processor_power_init(struct acpi_processor *pr)
> {
> @@ -443,6 +445,13 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
> {
> return -ENODEV;
> }
> +static int acpi_processor_register_idle_driver(void)
> +{
> + return -ENODEV;
> +}
> +static void acpi_processor_unregister_idle_driver(void)
> +{
> +}
> #endif /* CONFIG_ACPI_PROCESSOR_IDLE */
>
> /* in processor_thermal.c */
> --
> 2.33.0
>
Powered by blists - more mailing lists