linux-kernel - Re: [PATCH v6 01/11] x86/apic/x2apic: Fix parallel handling of cluster

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, 07 Feb 2023 10:57:05 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Usama Arif <usama.arif@...edance.com>, arjan@...ux.intel.com
Cc:     mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        hpa@...or.com, x86@...nel.org, pbonzini@...hat.com,
        paulmck@...nel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, rcu@...r.kernel.org, mimoja@...oja.de,
        hewenliang4@...wei.com, thomas.lendacky@....com, seanjc@...gle.com,
        pmenzel@...gen.mpg.de, fam.zheng@...edance.com,
        punit.agrawal@...edance.com, simon.evans@...edance.com,
        liangma@...ngbit.com
Subject: Re: [PATCH v6 01/11] x86/apic/x2apic: Fix parallel handling of
 cluster_mask

On Tue, 2023-02-07 at 00:20 +0100, Thomas Gleixner wrote:
> 
> 
> TBH. The logic of this code is anything but obvious. Something like the
> uncompiled below perhaps?

Looks sane to me. I'll tweak the comments a bit and give it a spin;
thanks.

...

> +        * At boot time CPU present mask is stable. If the cluster is not
> +        * yet initialized, allocate the mask and propagate it to all
> +        * siblings in this cluster.
>          */
> -       if (cluster_hotplug_mask) {
> -               if (cluster_hotplug_mask->node == node)
> -                       return 0;
> -               kfree(cluster_hotplug_mask);
> -       }
> +       if (system_state < SYSTEM_RUNNING)
> +               goto alloc;
> +
> +       /*
> +        * On post boot hotplug iterate over the present CPUs to handle the
> +        * case of partial clusters as they might be presented by
> +        * virtualization.
> +        */
> +       for_each_present_cpu(cpu_i) {


So... if this CPU was *present* at boot time (and if any other CPU in
this cluster was present), it will already have a cluster_mask.

Which means we get here in two cases: 

 • This CPU wasn't actually present (was just 'possible') at boot time.
   (Is that actually a thing that happens?)

 • This CPU was present but no other CPU in this cluster was actually
   brought up at boot time so the cluster_mask wasn't allocated.

The code looks right, I don't grok the comment about partial clusters
and virtualization, and would have worded it something along the above
lines?


Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)