Re: [PATCH v12 03/11] riscv: sse: mask events during shutdown and kexec

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] riscv: sse: unmasked SSE on other harts and preemptible machine_restart()
- [Medium] riscv: sse: unreachable !CONFIG_SMP panic_smp_self_stop() in reset.c
--

commit 62f5a8c35f215daa60d30a9c5320a99ed72eb26e
Author: Zhanpeng Zhang <zhangzhanpeng.jasper@bytedance.com>

riscv: sse: mask events during shutdown and kexec

This commit masks SBI Supervisor Software Events (SSE) on the local hart
during panic stop, CPU stop, restart, poweroff, and crash shutdown paths,
and rejects normal kexec from a crash kernel when firmware advertises SSE.

Link: https://lore.kernel.org/r/as7-3PbWDxygadnJ@hu-himchau-blr.qualcomm.com/

> diff --git a/arch/riscv/kernel/reset.c b/arch/riscv/kernel/reset.c
> index 14eb08a6db855..fdab37e7ae52d 100644
> --- a/arch/riscv/kernel/reset.c
> +++ b/arch/riscv/kernel/reset.c
> @@ -6,6 +6,20 @@
>  #include <linux/efi.h>
>  #include <linux/reboot.h>
>  #include <linux/pm.h>
> +#include <linux/smp.h>
> +
> +#include <asm/sse.h>
> +
> +#ifndef CONFIG_SMP
> +void __noreturn panic_smp_self_stop(void)
> +{
> +	riscv_sse_mask_current_hart();
> +	local_irq_disable();
> +
> +	for (;;)
> +		cpu_relax();
> +}
> +#endif

[Severity: Medium]
Can this !CONFIG_SMP implementation of panic_smp_self_stop() in
arch/riscv/kernel/reset.c ever be reached, and does a panic under !CONFIG_SMP
leave SSE unmasked on the panicking hart?

In kernel/panic.c, panic_smp_self_stop() is only called from vpanic() and
nmi_panic() -> nmi_panic_self_stop():

kernel/panic.c:vpanic() {
	if (panic_try_force_cpu(fmt, args)) {
		set_cpu_online(smp_processor_id(), false);
		panic_smp_self_stop();
	}
	...
	if (panic_try_start()) {
		/* go ahead */
	} else if (panic_on_other_cpu())
		panic_smp_self_stop();
}

When !CONFIG_SMP, panic_try_force_cpu() returns false directly. Furthermore,
panic_on_other_cpu() checks:

kernel/panic.c:panic_on_other_cpu() {
	return (panic_in_progress() && !panic_on_this_cpu());
}

Because raw_smp_processor_id() is 0 on !CONFIG_SMP and panic_try_start() only
ever sets panic_cpu to 0, panic_on_this_cpu() is always true whenever a panic
is in progress, making panic_on_other_cpu() always false.

Meanwhile, on CONFIG_SMP kernels, the panicking CPU masks SSE in
panic_other_cpus_shutdown() via smp_send_stop() or crash_smp_send_stop() in
arch/riscv/kernel/smp.c:

kernel/panic.c:panic_other_cpus_shutdown() {
	if (!crash_kexec)
		smp_send_stop();
	else
		crash_smp_send_stop();
}

On !CONFIG_SMP kernels, arch/riscv/kernel/smp.c is not built and both
smp_send_stop() and the weak crash_smp_send_stop() reduce to the no-op
smp_send_stop() in include/linux/smp.h. Without the panic notifier from the
follow-up commit "drivers: firmware: add riscv SSE support" (for example, when
CONFIG_RISCV_SBI_SSE is disabled, in a kdump kernel, or before arch_initcall
runs sse_init()), nothing masks SSE on the panicking hart under !CONFIG_SMP.

> @@ -18,6 +32,8 @@ EXPORT_SYMBOL(pm_power_off);
>  
>  void machine_restart(char *cmd)
>  {
> +	riscv_sse_mask_current_hart();
> +
>  	/*
>  	 * UpdateCapsule() depends on the system being reset via ResetSystem().
>  	 */
> @@ -30,12 +46,14 @@ void machine_restart(char *cmd)
>  
>  void machine_halt(void)
>  {
> +	riscv_sse_mask_current_hart();
>  	do_kernel_power_off();
>  	default_power_off();
>  }
>  
>  void machine_power_off(void)
>  {
> +	riscv_sse_mask_current_hart();
>  	do_kernel_power_off();
>  	default_power_off();
>  }

[Severity: High]
Should machine_restart(), machine_halt(), and machine_power_off() in
arch/riscv/kernel/reset.c also disable local interrupts and call
smp_send_stop() before masking SSE?

In arch/riscv/kernel/reset.c, riscv_sse_mask_current_hart() only masks SSE on
the local hart via SBI_SSE_HART_MASK. Although this patch adds
riscv_sse_mask_current_hart() to ipi_stop() and smp_send_stop() in
arch/riscv/kernel/smp.c, none of machine_restart(), machine_halt(), or
machine_power_off() call local_irq_disable() or smp_send_stop().

While the follow-up commit "drivers: firmware: add riscv SSE support" adds
sse_reboot_notifier() on reboot_notifier_list to tear down and mask SSE via
cpuhp_remove_state(CPUHP_AP_RISCV_SSE_ONLINE) during normal kernel_restart(),
kernel_halt(), and kernel_power_off(), emergency_restart() invokes
machine_emergency_restart() -> machine_restart(NULL) directly without running
reboot_notifier_list or migrate_to_reboot_cpu():

emergency_restart()
  -> machine_emergency_restart()
    -> machine_restart(NULL)

During emergency_restart() (for example from sysrq_handle_reboot(),
hw_failure_emergency_action_func(), watchdog handlers, or driver recovery
workqueues), or during normal restart/halt/poweroff if sse_cpu_teardown()
fails on a CPU, secondary CPUs never receive IPI_CPU_STOP and remain online
with SSE unmasked.

In addition, because machine_restart() does not disable interrupts or
preemption before calling riscv_sse_mask_current_hart(), callers with
preemption enabled can be migrated to another unmasked hart after
riscv_sse_mask_current_hart(), or preempted into idle if do_kernel_restart()
falls through to the infinite loop, where sse_pm_notifier() (CPU_PM_EXIT) can
unmask SSE on the local hart because sse_shutting_down remains false.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029074853.35467-1-zhangzhanpeng.jasper@bytedance.com?part=3