linux-kernel - Re: [PATCH 3/3] x86/microcode: Quiesce all threads before a microcode update.

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]

Message-ID: <20180221211503.GF16888@pd.tnic>
Date:   Wed, 21 Feb 2018 22:15:03 +0100
From:   Borislav Petkov <bp@...e.de>
To:     "Raj, Ashok" <ashok.raj@...el.com>
Cc:     X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        Tom Lendacky <thomas.lendacky@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        Andi Kleen <andi.kleen@...el.com>,
        Arjan Van De Ven <arjan.van.de.ven@...el.com>
Subject: Re: [PATCH 3/3] x86/microcode: Quiesce all threads before a
 microcode update.

On Wed, Feb 21, 2018 at 12:13:08PM -0800, Raj, Ashok wrote:
> This is ensuring no 2 cpus do ucode update at the same time.

And that is a problem?

We don't do any of that mutual exclusion for early loading. Why isn't it
there a problem?

> That's what we are doing here, but simply returning number of cpus
> that encountered failure instead of a per-cpu retval
> like before.

You still don't need ->errors.

> When we online any of the offline cpu's we do a microcode load again right?

That doesn't make any sense. First you say:

"the Intel microcode team asked us to make sure the system is in a quiet
state during these updates."

When you've updated the microcode on a subset of the cores and then the
other cores come up and you do that in the hotplug notifier, the system
is far from quiet. On the contrary, it is busy bootstrapping cores.

Which makes me wonder if that "quiet" argument even means anything.

Because if you wanna do that in the notifiers, you can just as well
offline all the cores but the BSP, update the microcode there and then
online the rest again ---> no need for that patch at all.

> Not sure what you mean by jumping through hoops need to be extracted away.. 

Take that code:

+       memset(&uc_data, 0, sizeof(struct ucode_update_param));
+       spin_lock_init(&uc_data.ucode_lock);
+       atomic_set(&uc_data.enter, num_online_cpus());
+       /*
+        * Wait for a 1 sec
+        */
+       uc_data.timeout = USEC_PER_SEC;
+       stop_machine(ucode_load_rendezvous, &uc_data, cpu_online_mask);
+
+       pr_debug("Total CPUS = %d uperrors = %d\n",
+               atomic_read(&uc_data.count), atomic_read(&uc_data.errors));
+
+       if (atomic_read(&uc_data.errors))

and put it in a separate function which you call in reload_store().

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--