Re: [PATCH v3 2/9] stop_machine: Add NMI-based execution path

[Nikolay Borisov <nik.borisov@suse.com>]

On 16.10.26 г. 0:14 ч., Chang S. Bae wrote:
> Currently multi_cpu_stop() executes the stop function in the
> MULTI_STOP_RUN state. But NMIs can still preempt the execution. For use
> cases requiring stricter isolation from them, provide an option to run
> the stop function from NMI context.
> 
> Then, the NMI stop function must be built without instrumentation, as
> exceptions may re-enable NMIs on return via IRET. Annotate the NMI
> handler entirely with noinstr.
> 
> However, objtool cannot reliably determine whether the indirect call
> target is located in a noinstr section, so it may emit false positives.
> To avoid this, temporarily lift the instrumentation restriction around
> the indirect call site and document the intention.
> 
> The x86 microcode loader is currently the primary user for this. But
> other architectures are not expected to use it. Add a build option to
> make it opt-in.
> 
> Originally-by: David Kaplan <david.kaplan@amd.com>
> Suggested-by: Borislav Petkov <bp@alien8.de>
> Suggested-by: Thomas Gleixner <tglx@kernel.org>
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Link: https://lore.kernel.org/lkml/20260129121729.GRaXtP2aeWkQKegxC2@fat_crate.local
> Link: https://lore.kernel.org/lkml/87wm0zl8p2.ffs@tglx
> ---
> V2 -> V3: Address Sashiko review
> * Avoid instrumentable functions:
>    smp_processor_id() -> raw_smp_processor_id()
>    cpumask_test_and_clear_cpu() -> arch_test_and_clear_bit()
> * Remove `done` tag to avoid false setting. Instead, data == NULL as indicator
> ---
>   arch/Kconfig                 |  3 ++
>   include/linux/stop_machine.h | 16 ++++++
>   kernel/stop_machine.c        | 99 +++++++++++++++++++++++++++++++++++-
>   3 files changed, 117 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 45c657772362..afa564261e1c 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -1878,4 +1878,7 @@ config ARCH_HAS_CPU_ATTACK_VECTORS
>   config HAVE_ARCH_GET_SECUREBOOT
>   	def_bool EFI
>   
> +config STOP_MACHINE_NMI
> +	bool
> +
>   endmenu
> diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
> index 84e7fb627ba4..b53b028296e0 100644
> --- a/include/linux/stop_machine.h
> +++ b/include/linux/stop_machine.h
> @@ -20,6 +20,12 @@
>    */
>   typedef int (*cpu_stop_fn_t)(void *arg);
>   
> +/*
> + * Stop function variant runnable from NMI context. This makes the
> + * noinstr requirement explicit at the type level.
> + */
> +typedef int (*cpu_stop_nmisafe_fn_t)(void *arg);
> +
>   #ifdef CONFIG_SMP
>   
>   struct cpu_stop_work {
> @@ -186,4 +192,14 @@ stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
>   }
>   
>   #endif	/* CONFIG_SMP || CONFIG_HOTPLUG_CPU */
> +
> +#ifdef CONFIG_STOP_MACHINE_NMI
> +
> +void arch_send_self_nmi(void);
> +bool noinstr stop_machine_nmi_handler(void);
> +
> +#else
> +static inline bool stop_machine_nmi_handler(void) { return false; }
> +#endif /* CONFIG_STOP_MACHINE_NMI */
> +
>   #endif	/* _LINUX_STOP_MACHINE */
> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> index 8dcd5be67b85..ad01b7c5266c 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -23,6 +23,7 @@
>   #include <linux/atomic.h>
>   #include <linux/nmi.h>
>   #include <linux/sched/wake_q.h>
> +#include <linux/delay.h>
>   
>   /*
>    * Structure to determine completion condition and record errors.  May
> @@ -175,6 +176,15 @@ struct multi_stop_data {
>   
>   	enum multi_stop_state	state;
>   	atomic_t		thread_ack;
> +
> +#ifdef CONFIG_STOP_MACHINE_NMI
> +	/* Used in the NMI stop_machine variant */
> +	bool			use_nmi;
> +	/* A separate function type to highlight noinstr requirement */
> +	cpu_stop_nmisafe_fn_t	nmisafe_fn;
> +	/* cpumask of CPUs on which to raise an NMI */
> +	cpumask_var_t		nmi_cpus;
> +#endif
>   };
>   
>   static void set_state(struct multi_stop_data *msdata,
> @@ -198,6 +208,8 @@ notrace void __weak stop_machine_yield(const struct cpumask *cpumask)
>   	cpu_relax();
>   }
>   
> +static int multi_stop_run(struct multi_stop_data *msdata);
> +
>   /* This is the cpu_stop function which stops the CPU. */
>   static int multi_cpu_stop(void *data)
>   {
> @@ -236,7 +248,7 @@ static int multi_cpu_stop(void *data)
>   				break;
>   			case MULTI_STOP_RUN:
>   				if (is_active)
> -					err = msdata->fn(msdata->data);
> +					err = multi_stop_run(msdata);
>   				break;
>   			default:
>   				break;
> @@ -713,3 +725,88 @@ int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
>   	mutex_unlock(&stop_cpus_mutex);
>   	return ret ?: done.ret;
>   }
> +
> +#ifdef CONFIG_STOP_MACHINE_NMI
> +
> +struct nmi_stop {
> +	struct multi_stop_data	*data;
> +	int			ret;
> +};
> +
> +static DEFINE_PER_CPU(struct nmi_stop, nmi_stop);
> +
> +/*
> + * Instrumentation may trigger nested exceptions such as #INT3, #DB,
> + * or #PF. IRET from those would re-enable NMIs, which opposes the goal
> + * of this NMI stop-machine facility.
> + */
> +bool noinstr stop_machine_nmi_handler(void)
> +{
> +	struct multi_stop_data *msdata = raw_cpu_read(nmi_stop.data);
> +	unsigned int cpu = raw_smp_processor_id();
> +	int ret;
> +
> +	if (!msdata || !arch_test_and_clear_bit(cpu, cpumask_bits(msdata->nmi_cpus)))
> +		return false;
> +
> +	/*
> +	 * The indirect call to @nmisafe_fn() is indistinguishable at
> +	 * post-compilation. Temporarily enabling instrumentation avoids
> +	 * objtool false positives.
> +	 */
> +	instrumentation_begin();
> +	ret = msdata->nmisafe_fn(msdata->data);
> +	instrumentation_end();
> +
> +	raw_cpu_write(nmi_stop.ret,  ret);
> +	raw_cpu_write(nmi_stop.data, NULL);
> +
> +	return true;
> +}

I think this function should be moved over to patch 5 and by having 
moved/subsumed patch 4, Patch 5 should become patch 3 really so related 
functionality is introduced in subsequent patches.

<snip>