| 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:
<DS7PR12MB820178355A3D8B7D0EC20FBD9491A@DS7PR12MB8201.namprd12.prod.outlook.com>
Date: Wed, 28 Jan 2026 01:31:44 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Borislav Petkov <bp@...en8.de>
CC: "Chang S. Bae" <chang.seok.bae@...el.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
"tglx@...utronix.de" <tglx@...utronix.de>, "mingo@...hat.com"
<mingo@...hat.com>, "dave.hansen@...ux.intel.com"
<dave.hansen@...ux.intel.com>, "peterz@...radead.org" <peterz@...radead.org>
Subject: RE: [PATCH 1/7] stop_machine: Introduce stop_machine_nmi()
[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Borislav Petkov <bp@...en8.de>
> Sent: Tuesday, January 27, 2026 12:50 PM
> To: Kaplan, David <David.Kaplan@....com>
> Cc: Chang S. Bae <chang.seok.bae@...el.com>; linux-kernel@...r.kernel.org;
> x86@...nel.org; tglx@...utronix.de; mingo@...hat.com;
> dave.hansen@...ux.intel.com; peterz@...radead.org
> Subject: Re: [PATCH 1/7] stop_machine: Introduce stop_machine_nmi()
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Tue, Jan 27, 2026 at 04:00:42PM +0000, Kaplan, David wrote:
> > The existing stop_machine() function description in the same header uses
> > lowercase cpu. This was intended to match.
>
> You can change yours to "CPU" and I can convert the rest. Or I can do them all
> - not a biggie.
>
> > Not entirely sure I follow the suggestion, but keep in mind that
> > stop_machine.c is only included in the build if CONFIG_SMP. So I had to put
> > some stuff in the header to ensure that a non-SMP build would not fail.
>
> Something like this:
>
> diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
> index 86113084456a..933fe8ddbec9 100644
> --- a/include/linux/stop_machine.h
> +++ b/include/linux/stop_machine.h
> @@ -184,13 +184,8 @@ int stop_core_cpuslocked(unsigned int cpu,
> cpu_stop_fn_t fn, void *data);
> int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
> const struct cpumask *cpus);
>
> +bool stop_machine_nmi_handler_enabled(void);
> bool noinstr stop_machine_nmi_handler(void);
> -DECLARE_STATIC_KEY_FALSE(stop_machine_nmi_handler_enable);
> -static __always_inline bool stop_machine_nmi_handler_enabled(void)
> -{
> - return static_branch_unlikely(&stop_machine_nmi_handler_enable);
> -}
> -
> #else /* CONFIG_SMP || CONFIG_HOTPLUG_CPU */
>
> static __always_inline int stop_machine_cpuslocked(cpu_stop_fn_t fn, void
> *data,
> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> index 55a350048d4c..0ecec98d52a6 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -205,7 +205,13 @@ struct stop_machine_nmi_ctrl {
> int err;
> };
>
> -DEFINE_STATIC_KEY_FALSE(stop_machine_nmi_handler_enable);
> +static DEFINE_STATIC_KEY_FALSE(stop_machine_nmi_handler_enable);
> +
> +bool stop_machine_nmi_handler_enabled(void)
> +{
> + return static_branch_unlikely(&stop_machine_nmi_handler_enable);
> +}
> +
> static DEFINE_PER_CPU(struct stop_machine_nmi_ctrl,
> stop_machine_nmi_ctrl);
>
> static void enable_nmi_handler(struct multi_stop_data *msdata)
Ok, that's what I thought you were suggesting. But as I noted, I don't think that works if CONFIG_SMP=n because stop_machine.c isn't built.
I think you could fix this by adding an #ifdef CONFIG_SMP in the header and a dummy definition of stop_machine_nmi_handler_enabled in that case, but is it worth adding another #ifdef/#else/#endif just to move the static key declaration?
>
> > This was in the existing CPU patching logic. I believe it is intended to
> > protect against the handler running multiple times.
> >
> > In particular, there are some cases where NMIs can get unmasked early so
> > there could be a risk I'd think of a second NMI coming in while the handler
> > is running. The Boolean protects against that. Maybe Chang knows more
> > history here.
> >
> > That said, the whole point of stop_machine_nmi() is to avoid an NMI coming
> > in, and for dynamic mitigations I explicitly made it mutually exclusive with
> > DEBUG_ENTRY so this wouldn't happen...
>
> Except that you don't need any of that fun. You can do:
>
> struct multi_stop_data {
> cpu_stop_fn_t fn;
> void *data;
> /* Like num_online_cpus(), but hotplug cpu uses us, so we need this. */
> unsigned int num_threads;
> const struct cpumask *active_cpus;
>
> enum multi_stop_state state;
> atomic_t thread_ack;
>
> bool use_nmi;
>
> /* cpumask of CPUs which need to run an NMI handler */
> const struct cpumask nmi_cpusmask;
> ^^^^^^
>
> add this thing
>
> };
>
> which you initialize to the cpumask you want NMIs to run on.
>
> stop_machine_nmi_handler() tests its CPU ID against the mask, if the bit is
> set, it clears it and runs the function pointer.
>
> You don't need to enable anything as it is implicitly enabled when you have
> that bit in the cpumask set and you don't need to test whether anything's
> enabled because, well, also implicitly enabled for only once on the respective
> CPU.
>
> And then you can wrap all that logic:
>
> case MULTI_STOP_RUN:
> if (is_active) {
> if (msdata->use_nmi) {
> arch_send_self_nmi();
> err = raw_cpu_read(stop_machine_nmi_ctrl.err);
> } else {
> err = msdata->fn(msdata->data);
> }
> }
>
> into a single function which multi_cpu_stop() calls and in that function you
> decide whether to do an NMI or do a normal function call.
>
> Sounds good?
>
I think that can also work, but just curious why that would be preferred? It seems very similar in theory (test a bit to see if the handler hasn't been run yet on this cpu and if so clear it and run the handler). It might be slightly less memory usage (I think) but creates more cache contention with every core trying to modify the one nmi_cpumask value. And while the perf of any stop machine isn't meant to be the best, we still don't want the NMI handler to run longer than it has to and having hundreds of cores fighting over the same cacheline to clear their bit doesn't seem ideal.
Thanks
--David Kaplan
Powered by blists - more mailing lists