linux-kernel - Re: [PATCH 4/5] acpi/processor: Fix the return value of acpi_processor_ids

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]

Date:   Tue, 22 May 2018 09:47:36 +0800
From:   Dou Liyang <douly.fnst@...fujitsu.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     LKML <linux-kernel@...r.kernel.org>, <x86@...nel.org>,
        <linux-acpi@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Len Brown <lenb@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 4/5] acpi/processor: Fix the return value of
 acpi_processor_ids_walk()



At 05/19/2018 11:06 PM, Thomas Gleixner wrote:
> On Tue, 20 Mar 2018, Dou Liyang wrote:
> 
>> ACPI driver should make sure all the processor IDs in their ACPI Namespace
>> are unique for CPU hotplug. the driver performs a depth-first walk of the
>> namespace tree and calls the acpi_processor_ids_walk().
>>
>> But, the acpi_processor_ids_walk() will return true if one processor is
>> checked, that cause the walk break after walking pass the first processor.
>>
>> Repace the value with AE_OK which is the standard acpi_status value.
>>
>> Fixes 8c8cb30f49b8 ("acpi/processor: Implement DEVICE operator for processor enumeration")
>>
>> Signed-off-by: Dou Liyang <douly.fnst@...fujitsu.com>
>> ---
>>   drivers/acpi/acpi_processor.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 449d86d39965..db5bdb59639c 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -663,11 +663,11 @@ static acpi_status __init (acpi_handle handle,
>>   	}
>>   
>>   	processor_validated_ids_update(uid);
>> -	return true;
>> +	return AE_OK;
>>   
>>   err:
>>   	acpi_handle_info(handle, "Invalid processor object\n");
>> -	return false;
>> +	return AE_OK;
> 
> I'm not sure whether this is the right return value here. Rafael?
> 
Hi, Thomas, Rafael,

Yes, I used AE_OK to make sure it can skip the invalid objects and
continue to do the following other objects, I'm also not sure.

For this bug, recently, I sent another patch to remove this check code
away.

    https://lkml.org/lkml/2018/5/17/320

IMO, the duplicate IDs can be avoid by the other code

    if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) ---- 1)

As the mapping of cpu_id(pr->id) and processor_id is fixed, when
hot-plugging a physical CPU, if its processor_id is duplicated with the
present, the above condition 1) will be 0, and Linux will do not add
this CPU.

And, when every time the system starts, this code will be executed, it
will waste more time with the increase in the number of CPU.

So I prefer to remove this code.

Thanks,
	dou