| 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: <b5da2c2a-8621-45a8-865f-b6e90089b586@huawei.com>
Date: Thu, 24 Jul 2025 21:15:21 +0800
From: "lihuisong (C)" <lihuisong@...wei.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: <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>,
<lihuisong@...wei.com>
Subject: Re: [PATCH v2] ACPI: processor: idle: Fix resource leak and potential
concurrent in acpi_processor_power_init()
在 2025/7/23 21:35, Rafael J. Wysocki 写道:
> 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.
Ok, will remove it.
>
>> 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.
Got it. will add a separate patch in next version.
>
>> 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.
Ack.
>
> 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.
Yeah.
Anyway, it is ok if the initialization is serialized as you mentioned.
>
>> 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.
Ack
>
>> 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.
Ok, this patch will use this tone in the commit log.
>
>> 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.
Ack
>
> Note that it may fail if intel_idle is already registered, for
> example, so the message should rather be a debug-level one.
All right.
>> +
>> 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;
Ack
>
>> +
>> + /*
>> + * 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.
Ack
>
>> + continue;
>> +
>> + ret = acpi_processor_get_power_info(pr);
> if (ret)
> continue;
Please see the following reply.
>
>> + 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?
Setting to 1 is necessary here,
otherwise acpi_processor_setup_cpuidle_states would not initialize
acpi_idle_driver because this flag is false.
>
>> + break;
>> + }
>> + }
>> +
>> + if (unlikely(!pr))
>> + return -ENODEV;
> This is unnecessary if ret is initialized to -ENODEV;
ok
>
>> +
>> + 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.
Ack
>
>> + 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.
will fix it as you said.
>
>> + 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).
All right.
>
>> + 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.
Ack.
>
>
>> 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