Re: [PATCH v12 04/11] drivers: firmware: add riscv SSE support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] firmware: riscv_sbi_sse: sse_pm_notifier() unmasks SSE on unready CPUs
- [High] firmware: riscv_sbi_sse: sse_event_cleanup() leaks retained SSE events
- [High] firmware: riscv_sbi_sse: sse_cpu_teardown() mishandles CPU-up rollback
- [High] firmware: riscv_sbi_sse: missing synchronization in sse_event_*_local()
- [Medium] firmware: riscv_sbi_sse: invalid hart/CPU check in sse_event_register()
- [Medium] firmware: riscv_sbi_sse: bad rollback in sse_event_set_target_cpu_nolock()
--

commit a43ce7f3ee4ccdf7ca9265079df6d0b217323b41
Author: Clément Léger <cleger@rivosinc.com>

drivers: firmware: add riscv SSE support

This commit adds a driver-level interface for RISC-V Supervisor Software
Events (SSE), allowing kernel clients to register handlers, manage target
CPUs for global events, and enable, disable, or unregister local and global
events. It also coordinates per-CPU SSE state across CPU hotplug, idle power
management, panic, and reboot/kexec transitions.

> diff --git a/drivers/firmware/riscv/riscv_sbi_sse.c b/drivers/firmware/riscv/riscv_sbi_sse.c
> new file mode 100644
> index 0000000000000..0cb738bab6c39
> --- /dev/null
> +++ b/drivers/firmware/riscv/riscv_sbi_sse.c
[ ... ]
> +static int sse_sbi_event_func(struct sse_event *event, unsigned long func)
> +{
> +	struct sbiret ret;
> +	u32 evt = event->evt_id;
> +	struct sse_registered_event *reg_evt = sse_get_reg_evt(event);
> +
> +	ret = sbi_ecall(SBI_EXT_SSE, func, evt, 0, 0, 0, 0, 0);
> +	if (ret.error) {
> +		pr_warn("Failed to execute func %lx, event %x, error %ld\n",
> +			func, evt, ret.error);
> +		return sse_err_map_linux_errno(ret.error);
> +	}
> +
> +	if (func == SBI_SSE_EVENT_DISABLE)
> +		reg_evt->is_enabled = false;
> +	else if (func == SBI_SSE_EVENT_ENABLE)
> +		reg_evt->is_enabled = true;
> +
> +	return 0;
> +}
> +
> +int sse_event_disable_local(struct sse_event *event)
> +{
> +	if (!sse_event_is_global(event->evt_id))
> +		lockdep_assert_preemption_disabled();
> +
> +	if (!sse_get_reg_evt(event)->is_enabled)
> +		return 0;
> +
> +	return sse_sbi_event_func(event, SBI_SSE_EVENT_DISABLE);
> +}
> +EXPORT_SYMBOL_GPL(sse_event_disable_local);
> +
> +int sse_event_enable_local(struct sse_event *event)
> +{
> +	struct sse_registered_event *reg_evt = sse_get_reg_evt(event);
> +	int ret;
> +
> +	if (!sse_event_is_global(event->evt_id))
> +		lockdep_assert_preemption_disabled();
> +
> +	rcu_read_lock();
> +	if (READ_ONCE(sse_shutting_down)) {
> +		ret = -ESHUTDOWN;
> +		goto out;
> +	}
> +
> +	if (!reg_evt->is_registered) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (reg_evt->is_enabled) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	ret = sse_sbi_event_func(event, SBI_SSE_EVENT_ENABLE);
> +out:
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(sse_event_enable_local);

[Severity: High]
Can sse_event_enable_local() and sse_event_disable_local() race with hardirqs,
CPU hotplug teardown, or reboot teardown and trigger duplicate or invalid-state
SBI calls?

There appear to be several related synchronization gaps here:

1. In sse_sbi_event_func(), reg_evt->is_enabled is updated only after
sbi_ecall() returns, and neither sse_event_disable_local() nor
sse_event_enable_local() masks interrupts. If a hardirq (such as
perf_event_task_tick(), perf_mux_hrtimer_handler(), or a perf IPI calling
perf_pmu_disable() -> pmu_sbi_sse_disable() -> sse_event_disable_local())
fires after sbi_ecall(SBI_EXT_SSE, SBI_SSE_EVENT_DISABLE) completes in
firmware but before reg_evt->is_enabled = false is written, the IRQ handler
observes stale reg_evt->is_enabled == true and issues a second
SBI_SSE_EVENT_DISABLE call that fails with SBI_ERR_INVALID_STATE (-EINVAL).
If the IRQ handler then calls sse_event_enable_local(), it also sees stale
reg_evt->is_enabled == true and returns 0 without re-enabling the event in
firmware.

2. During reboot or kexec, sse_reboot_notifier() runs at default priority 0
(before perf_reboot_notifier at INT_MIN) and calls
cpuhp_remove_state(CPUHP_AP_RISCV_SSE_ONLINE) while higher states such as
CPUHP_AP_PERF_ONLINE are still active. In sse_cpu_teardown(),
sse_events_mask() only masks SSE delivery, and scoped_guard(preempt) leaves
hardirqs enabled for local events (and is not used for global events).
Because sse_event_disable_local() does not check sse_shutting_down, a racing
timer IRQ or IPI calling sse_event_disable_local() concurrently with
sse_teardown_event() can cause one of the two SBI_SSE_EVENT_DISABLE calls to
fail with -EINVAL. That causes sse_teardown_event() to abort before
sse_sbi_unregister_event(), sets sse_teardown_failed, and triggers
panic("SSE teardown failed; refusing unsafe kexec") during kexec.

3. During normal CPU offline in sse_cpu_teardown() (where sse_shutting_down
is false) or global event migration in sse_event_set_target_cpu_nolock(), the
event is disabled while reg_evt->is_registered remains true. A concurrent
sse_event_enable_local() from an IRQ on the same CPU (or from another CPU for
a global event, since sse_event_enable_local() and sse_event_disable_local()
do not take sse_mutex or cpus_read_lock()) can re-enable the event in
firmware before SBI_SSE_EVENT_UNREGISTER or SBI_SSE_EVENT_ATTR_WRITE runs,
causing firmware to reject the operation with -EINVAL. Also,
sse_event_enable_local() calls sse_get_reg_evt(event) (which uses
smp_processor_id() for local events) before
lockdep_assert_preemption_disabled().

4. If CPU offline aborts and jumps to restore_cpu in sse_cpu_teardown(),
sse_restore_local_events() restores local events using
event->enable_requested, which is only updated by sse_event_enable() and
sse_event_disable(), losing per-CPU enable state set via
sse_event_enable_local() or sse_event_disable_local().

[ ... ]
> +static int sse_event_set_target_cpu_nolock(struct sse_event *event,
> +					   unsigned int cpu)
> +{
> +	unsigned long hart_id, old_hart_id;
> +	struct sse_registered_event *reg_evt = event->global;
> +	u32 evt = event->evt_id;
> +	unsigned int old_cpu;
> +	bool was_enabled;
> +	int ret;
> +
> +	if (!sse_event_is_global(evt))
> +		return -EINVAL;
> +
> +	if (cpu >= nr_cpu_ids || !cpu_online(cpu))
> +		return -EINVAL;
> +	hart_id = cpuid_to_hartid_map(cpu);
> +	old_cpu = event->cpu;
> +	old_hart_id = cpuid_to_hartid_map(old_cpu);
> +
> +	was_enabled = reg_evt->is_enabled;
> +	if (was_enabled) {
> +		ret = sse_event_disable_local(event);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = sse_event_attr_set_nolock(reg_evt, SBI_SSE_ATTR_PREFERRED_HART,
> +					hart_id);
> +	if (ret == 0)
> +		sse_global_event_update_cpu(event, cpu);
> +
> +	if (was_enabled) {
> +		int enable_ret;
> +
> +		enable_ret = sse_event_enable_local(event);
> +		if (enable_ret) {
> +			int rollback_ret;
> +
> +			/*
> +			 * The preferred hart was already changed. Restore both the
> +			 * firmware attribute and Linux's cached target before reporting
> +			 * the failed migration.
> +			 */
> +			rollback_ret = sse_event_attr_set_nolock(reg_evt,
> +								 SBI_SSE_ATTR_PREFERRED_HART,
> +								 old_hart_id);
> +			if (!rollback_ret) {
> +				sse_global_event_update_cpu(event, old_cpu);
> +				rollback_ret = sse_event_enable_local(event);
> +			}
> +			if (!rollback_ret)
> +				return enable_ret;
> +
> +			pr_warn("Failed to restore global event %x to CPU %u: %d\n",
> +				evt, old_cpu, rollback_ret);
> +			event->enable_requested = false;
> +			return enable_ret;
> +		}
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
In sse_event_set_target_cpu_nolock(), what happens if
sse_event_attr_set_nolock(reg_evt, SBI_SSE_ATTR_PREFERRED_HART, hart_id)
fails (ret != 0) when was_enabled is true, and the subsequent
sse_event_enable_local(event) on old_cpu also fails?

Because the if (enable_ret) block does not check whether ret == 0, it will
assume the preferred hart was changed, write old_hart_id to
SBI_SSE_ATTR_PREFERRED_HART again, call sse_event_enable_local(event) a
second time, and return enable_ret instead of the original attribute-write
error in ret.

[ ... ]
> +void sse_event_cleanup(struct sse_event *event)
> +{
> +	guard(mutex)(&sse_mutex);
> +	guard(cpus_read_lock)();
> +
> +	if (event->cleanup_pending)
> +		return;
> +
> +	/*
> +	 * Firmware may still enter the old callback after disable or unregister
> +	 * fails. Publish a core-owned callback, then wait before the client frees
> +	 * its callback data.
> +	 */
> +	event->cleanup_pending = true;
> +	rcu_assign_pointer(event->handler, sse_cleanup_event_handler);
> +	synchronize_rcu();
> +}
> +EXPORT_SYMBOL_GPL(sse_event_cleanup);
> +
> +static void sse_release_cleanup_event(struct sse_event *event)
> +{
> +	if (!event->cleanup_pending || sse_event_is_registered(event))
> +		return;
> +
> +	list_del(&event->list);
> +	sse_event_free(event);
> +}

[Severity: High]
Does sse_event_cleanup() leak struct sse_event (along with its global or
per-CPU registrations, vmap stacks, shadow call stacks, and interrupted
buffers) and leave the event ID permanently blocked in the events list with
-EEXIST?

In sse_event_cleanup(), event->cleanup_pending is set to true and the handler
is replaced with sse_cleanup_event_handler, but sse_release_cleanup_event() is
never called, nor does sse_event_cleanup() clear event->enable_requested or
attempt to disable and unregister remaining online instances.

This leaves the event stranded in two scenarios:

1. If all firmware registrations are already gone by the time
sse_event_cleanup() acquires sse_mutex, sse_event_is_registered(event) is
already false. This can happen if sse_event_unregister() fails on one CPU and
releases sse_mutex before returning the error, and that CPU goes offline via
sse_cpu_teardown() before the client calls sse_event_cleanup() (when
sse_cpu_teardown() ran, cleanup_pending was still false, so
sse_release_cleanup_event() was a no-op). It also happens during reboot/kexec
if sse_reboot_notifier() runs cpuhp_remove_state(CPUHP_AP_RISCV_SSE_ONLINE)
first and a client shutdown callback then receives -ESHUTDOWN from
sse_event_disable() or sse_event_unregister() and calls sse_event_cleanup().
Once sse_event_cleanup() sets cleanup_pending = true on an already
unregistered event without calling sse_release_cleanup_event(event), nothing
may ever revisit it (especially for global events, which sse_cpu_online()
skips), leaking the event and causing future sse_event_register() calls for
that evt_id to fail with -EEXIST.

2. If a client calls sse_event_cleanup() directly after sse_event_disable()
fails on one CPU without calling sse_event_unregister(), sse_event_cleanup()
does not clear event->enable_requested or unregister the event on the online
CPUs where disabling succeeded. Those online CPUs remain registered in
firmware, preventing sse_release_cleanup_event() from ever freeing the event.

[ ... ]
> +struct sse_event *sse_event_register(u32 evt, u32 priority,
> +				     sse_event_handler_fn *handler, void *arg)
> +{
[ ... ]
> +	if (sse_event_is_global(evt)) {
> +		unsigned long preferred_hart;
> +
> +		ret = sse_event_attr_get_no_lock(event->global,
> +						 SBI_SSE_ATTR_PREFERRED_HART,
> +						 &preferred_hart);
> +		if (ret)
> +			return sse_register_failed(event, ret);
> +
> +		cpu = riscv_hartid_to_cpuid(preferred_hart);
> +		if (cpu < 0 || !cpu_online(cpu)) {
> +			cpu = cpumask_first(cpu_online_mask);

[Severity: Medium]
What happens in sse_event_register() if firmware reports preferred_hart ==
INVALID_HARTID when nr_cpu_ids < NR_CPUS?

In arch/riscv/kernel/smp.c, unpopulated entries in __cpuid_to_hartid_map[NR_CPUS]
are initialized to INVALID_HARTID, and riscv_hartid_to_cpuid() loops up to
NR_CPUS:

arch/riscv/kernel/smp.c:riscv_hartid_to_cpuid() {
	for (i = 0; i < NR_CPUS; i++)
		if (cpuid_to_hartid_map(i) == hartid)
			return i;
}

When preferred_hart is INVALID_HARTID, riscv_hartid_to_cpuid() returns the
first unpopulated index (cpu >= nr_cpu_ids) rather than a negative error.
Passing cpu >= nr_cpu_ids to cpu_online(cpu) without first checking
preferred_hart == INVALID_HARTID or cpu >= nr_cpu_ids accesses past
nr_cpu_ids in cpu_online_mask and triggers WARN_ON_ONCE(cpu >=
small_cpumask_bits) in cpumask_check() when CONFIG_DEBUG_PER_CPU_MAPS is
enabled.

[ ... ]
> +static int sse_cpu_teardown(unsigned int cpu)
> +{
> +	/* Only a regular CPU-offline callback may abort the CPUHP operation. */
> +	bool regular_offline = READ_ONCE(sse_available) &&
> +			       !READ_ONCE(sse_shutting_down);
> +	unsigned int next_cpu;
> +	struct sse_event *event, *tmp;
> +	int first_error = 0;
> +	int ret;
> +
> +	/* Do not dismantle CPU state while firmware can still deliver SSE. */
> +	ret = sse_events_mask();
> +	if (ret) {
> +		pr_warn("Failed to mask SSE on CPU %u during teardown: %d\n",
> +			cpu, ret);
> +		if (READ_ONCE(sse_shutting_down))
> +			atomic_set(&sse_teardown_failed, 1);
> +		/* CPUHP installation rollback and state removal cannot fail. */
> +		return regular_offline ? ret : 0;
> +	}
> +
> +	list_for_each_entry_safe(event, tmp, &events, list) {
> +		if (sse_event_is_global(event->evt_id))
> +			continue;
> +
> +		scoped_guard(preempt)
> +			ret = sse_teardown_event(event, cpu);
> +		if (ret && !first_error)
> +			first_error = ret;
> +		sse_release_cleanup_event(event);
> +		if (ret && regular_offline)
> +			goto restore_cpu;
> +	}
> +
> +	list_for_each_entry_safe(event, tmp, &events, list) {
> +		if (!sse_event_is_global(event->evt_id) || event->cpu != cpu)
> +			continue;
> +
> +		/*
> +		 * cpuhp_remove_state() invokes teardown while every CPU remains
> +		 * online. Do not migrate to a CPU whose callback may have run.
> +		 */
> +		if (READ_ONCE(sse_shutting_down) || event->cleanup_pending) {
> +			ret = sse_teardown_event(event, cpu);
> +		} else {
> +			next_cpu = cpumask_any_but(cpu_online_mask, cpu);
> +			if (next_cpu >= nr_cpu_ids) {
> +				ret = sse_teardown_event(event, cpu);
> +			} else {
> +				ret = sse_event_set_target_cpu_nolock(event, next_cpu);
> +				if (ret)
> +					pr_warn("Failed to migrate global event %x from CPU %u: %d\n",
> +						event->evt_id, cpu, ret);
> +			}
> +		}
> +
> +		if (ret && !first_error)
> +			first_error = ret;
> +		sse_release_cleanup_event(event);
> +		if (ret && regular_offline)
> +			goto restore_cpu;
> +	}
> +
> +	if (first_error) {
> +		/* An offline CPU is not revisited when this CPUHP state is removed. */
> +		atomic_set(&sse_teardown_failed, 1);
> +	}
> +
> +	return 0;
> +
> +restore_cpu:
> +	/*
> +	 * CPUHP leaves this CPU online when teardown returns an error. Restore
> +	 * every client-owned local event before making SSE delivery visible again.
> +	 */
> +	ret = sse_restore_local_events(cpu);
> +	if (!ret)
> +		ret = sse_events_unmask();
> +	if (ret) {
> +		atomic_set(&sse_teardown_failed, 1);
> +		pr_warn("Failed to restore SSE after aborting CPU %u offline: %d\n",
> +			cpu, ret);
> +	} else {
> +		pr_warn("Aborted CPU %u offline after SSE teardown failed: %d\n",
> +			cpu, first_error);
> +	}
> +
> +	return first_error;
> +}

[Severity: High]
Does sse_cpu_teardown() handle a post-boot _cpu_up() rollback when a CPUHP
state after CPUHP_AP_RISCV_SSE_ONLINE fails?

Once sse_init() sets sse_available = true, if a CPU is being brought online
via _cpu_up() and sse_cpu_online() succeeds at CPUHP_AP_RISCV_SSE_ONLINE, but
a later startup state (such as CPUHP_AP_PERF_ONLINE) fails, CPUHP sets
st->rollback = true and invokes teardown callbacks on the AP to roll back
towards offline.

During that _cpu_up() rollback, regular_offline is true in
sse_cpu_teardown(). If sse_events_mask() fails, sse_cpu_teardown() returns ret
without setting sse_teardown_failed. If any sse_teardown_event() call fails,
it jumps to restore_cpu, which calls sse_restore_local_events(cpu) and
sse_events_unmask(), leaves sse_teardown_failed unset if the restore succeeds,
and returns first_error (non-zero).

In kernel/cpu.c:cpuhp_thread_fun(), returning non-zero from a teardown
callback during rollback triggers WARN_ON_ONCE(st->rollback) and halts
rollback of earlier AP states, while cpuhp_up_callbacks() still proceeds to
takedown_cpu() and stops the CPU with SSE unmasked, local events still
registered and enabled in firmware, and sse_teardown_failed left at 0.

> +
> +static int sse_pm_notifier(struct notifier_block *nb, unsigned long action,
> +			   void *data)
> +{
> +	int ret;
> +
> +	WARN_ON_ONCE(preemptible());
> +
> +	switch (action) {
> +	case CPU_PM_ENTER:\n> +		ret = sse_events_mask();
> +		break;
> +	case CPU_PM_EXIT:
> +	case CPU_PM_ENTER_FAILED:
> +		if (READ_ONCE(sse_shutting_down))
> +			return NOTIFY_OK;
> +		ret = sse_events_unmask();
> +		break;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	if (ret)
> +		return notifier_from_errno(ret);
> +
> +	return NOTIFY_OK;
> +}
[ ... ]
> +static int __init sse_init(void)
> +{
[ ... ]
> +	ret = cpu_pm_register_notifier(&sse_pm_nb);
> +	if (ret) {
> +		pr_warn("Failed to register CPU PM notifier...\n");
> +		return ret;
> +	}
> +
> +	ret = register_reboot_notifier(&sse_reboot_nb);
> +	if (ret) {
> +		pr_warn("Failed to register reboot notifier...\n");
> +		goto remove_cpupm;
> +	}
> +
> +	ret = atomic_notifier_chain_register(&panic_notifier_list, &sse_panic_nb);
> +	if (ret) {
> +		pr_warn("Failed to register panic notifier...\n");
> +		goto remove_reboot;
> +	}
> +
> +	/* Tear down perf events before dismantling their SSE delivery path. */
> +	ret = cpuhp_setup_state(CPUHP_AP_RISCV_SSE_ONLINE, "riscv/sse:online",
> +				sse_cpu_online, sse_cpu_teardown);

[Severity: High]
Can sse_pm_notifier() unmask SSE on a CPU before sse_cpu_online() has
initialized its per-CPU SSE state, or after sse_cpu_teardown() has already
dismantled it?

In sse_pm_notifier(), CPU_PM_EXIT and CPU_PM_ENTER_FAILED unconditionally call
sse_events_unmask() whenever sse_shutting_down is false, without checking
whether SSE is currently online on this CPU:

1. In sse_init(), cpu_pm_register_notifier(&sse_pm_nb) is registered before
cpuhp_setup_state(CPUHP_AP_RISCV_SSE_ONLINE, ...). Because sbi_cpuidle_init()
is also an arch_initcall and drivers/cpuidle/ is linked before
drivers/firmware/, cpuidle can already be active on online CPUs and call
cpu_pm_exit() -> sse_pm_notifier() -> sse_events_unmask() before
sse_cpu_online() runs arch_sse_init_cpu() to set __sbi_sse_entry_task.

2. During secondary CPU bringup, the AP enables interrupts in smp_callin() and
enters the idle loop at CPUHP_AP_ONLINE_IDLE (cpu_startup_entry() -> do_idle()
-> cpuidle_idle_call() -> cpu_pm_enter() / cpu_pm_exit()) before the cpuhp/%u
thread reaches CPUHP_AP_RISCV_SSE_ONLINE (or after sse_cpu_online() fails and
rolls back), unmasking SSE on an uninitialized hart.

3. During normal CPU offline (_cpu_down()), sse_cpu_teardown() runs at
CPUHP_AP_RISCV_SSE_ONLINE on the cpuhp/%u thread and masks SSE. The cpuhp/%u
thread then parks at CPUHP_AP_ONLINE_IDLE while the offlining CPU remains in
cpu_online_mask and runs do_idle() -> cpuidle_idle_call() waiting for the
control CPU to invoke takedown_cpu() -> stop_machine_cpuslocked(take_cpu_down,
...) at CPUHP_TEARDOWN_CPU. When the CPU wakes from cpuidle (for example, on
the stop_machine IPI), CPU_PM_EXIT in sse_pm_notifier() calls
sse_events_unmask() and re-enables SSE on the dying hart after teardown.

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