linux-kernel - Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping

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:   Thu, 6 Oct 2016 16:06:42 +0800
From:   Dou Liyang <douly.fnst@...fujitsu.com>
To:     Yinghai Lu <yinghai@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>
CC:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        "linux-tip-commits@...r.kernel.org" 
        <linux-tip-commits@...r.kernel.org>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid
 <-> apicid mapping

Hi Yinghai,

At 10/06/2016 12:53 PM, Yinghai Lu wrote:
> On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
>>> @@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u
>>>          return -EINVAL;
>>>      }
>>>
>>> +    if (!enabled && (id == disabled_id)) {
>>> +        ++disabled_cpus;
>>> +        return -EINVAL;
>>> +    }
>>
>> Why would you need that disabled_id thing at all? The proper fix is to let
>> the apic driver detect the issue and this boils down to a 5 lines
>> change. Does the patch below fix the issue for you?
>> 8<----------------
>> --- a/arch/x86/kernel/apic/apic.c
>> +++ b/arch/x86/kernel/apic/apic.c
>> @@ -2076,6 +2076,11 @@ int __generic_processor_info(int apicid,
>>         bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
>>                                 phys_cpu_present_map);
>>
>> +       if (!apic->apic_id_valid(apicid)) {
>> +               disabled_cpus++;
>> +               return -EINVAL;
>> +       }
>> +
>>         /*
>>          * boot_cpu_physical_apicid is designed to have the apicid
>>          * returned by read_apic_id(), i.e, the apicid of the
>>
>>
>
> No, That does not fix the issue.
>
> the system have x2apic pre_enabled from BIOS, so at the time
> apic is set to &apic_x2apic_cluster.
>
> early_acpi_boot_init ==> early_acpi_process_madt ==> acpi_parse_madt
> ==> default_acpi_madt_oem_check
>
> default_acpi_madt_oem_check
>       ==> apic_x2apic_cluster/x2apic_acpi_madt_oem_check ==> x2apic_enabled
>       ==> apic = &apic_x2apic_cluster
>
> and
> static int x2apic_apic_id_valid(int apicid)
> {
>         return 1;
> }
>
> To make your change work, may need to update x2apic_apic_id_valid to
>
> static int x2apic_apic_id_valid(int apicid)
> {
>         if (apicid == 0xff || apicid == -1)
>             return 0;
>

I seem to remember that in x2APIC Spec the x2APIC ID may be at 255 or
greater.
If we do that judgment, it may be affect x2APIC's work in some other places.

I saw the MADT, the main reason may be that we define 0xff to acpi_id
in LAPIC mode.
As you said, it was like:
[   42.107902] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   42.120125] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   42.132361] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
...

How about doing the acpi_id check when we parse it in
acpi_parse_lapic().

8<----------------

--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -233,6 +233,11 @@ acpi_parse_lapic(struct acpi_subtable_header * 
header, const unsigned long end)

         acpi_table_print_madt_entry(header);

+       if (processor->id >= 255) {
+               ++disabled_cpus;
+               return -EINVAL;
+       }
+
         /*
          * We need to register disabled CPU as well to permit
          * counting disabled CPUs. This allows us to size


Thanks

Dou

>         return 1;
> }
>
>
> Thanks
>
> Yinghai
>
>