Re: [PATCH v4] irqchip/riscv-aplic: fix asynchronous probing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] irqchip/riscv-aplic: unsynchronized global state in aplic_probe() async path
--

commit 6da3e6c23904896b41105f01bdd39cd511ef2b88
Author: Vladimir Kondratiev <vladimir.kondratiev@xxxxxxxxxxxx>

irqchip/riscv-aplic: fix asynchronous probing

This commit replaces the __ro_after_init boolean aplic_syscore_registered
flag with an atomic_t and uses atomic_cmpxchg() in aplic_syscore_init().
This ensures syscore operations are registered only once during concurrent
asynchronous probing and avoids writing to read-only memory when probing is
deferred past init.

> diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
> index d9afb6ae98cf5..54e1d5602f15a 100644
> --- a/drivers/irqchip/irq-riscv-aplic-main.c
> +++ b/drivers/irqchip/irq-riscv-aplic-main.c
> @@ -116,14 +116,12 @@ static struct syscore aplic_syscore = {
>  	.ops = &aplic_syscore_ops,
>  };
>  
> -static bool aplic_syscore_registered __ro_after_init;
> +static atomic_t aplic_syscore_registered = ATOMIC_INIT(0);
>  
>  static void aplic_syscore_init(void)
>  {
> -	if (!aplic_syscore_registered) {
> +	if (!atomic_cmpxchg(&aplic_syscore_registered, 0, 1))
>  		register_syscore(&aplic_syscore);
> -		aplic_syscore_registered = true;
> -	}
>  }

[Severity: High]
Does making aplic_syscore_registered atomic fully protect the driver during
concurrent asynchronous probing?

Before aplic_probe() reaches aplic_syscore_init(), it invokes either
aplic_msi_setup() or aplic_direct_setup(), both of which call
aplic_setup_priv() -> aplic_pm_add().

In drivers/irqchip/irq-riscv-aplic-main.c, aplic_pm_add() inserts into the
driver-global aplics list without holding a lock:

	priv->saved_hw_regs.srcs = srcs;
	list_add(&priv->head, &aplics);

Furthermore, when aplic_msi_setup() returns -EPROBE_DEFER because the MSI
parent domain is not ready (or fails on another error path), devres cleanup
runs aplic_pm_remove(), which deletes from the same global aplics list
without any lock:

	list_del(&priv->head);
	if (dev->pm_domain && dev->of_node)
		dev_pm_genpd_remove_notifier(dev);

Similarly, in drivers/irqchip/irq-riscv-aplic-direct.c, aplic_direct_setup()
performs an unsynchronized check-then-set on the global
aplic_direct_parent_irq variable:

	if (!aplic_direct_parent_irq && domain) {
		aplic_direct_parent_irq = irq_create_mapping(domain, RV_IRQ_EXT);
		if (aplic_direct_parent_irq) {
			irq_set_chained_handler(aplic_direct_parent_irq,
						aplic_direct_handle_irq);

			/*
			 * Setup CPUHP notifier to enable parent
			 * interrupt on all CPUs
			 */
			cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
					  "irqchip/riscv/aplic:starting",
					  aplic_direct_starting_cpu,
					  aplic_direct_dying_cpu);
		}
	}

When multiple APLIC instances are probed asynchronously at the same time,
can concurrent calls to aplic_pm_add(), aplic_pm_remove(), and
aplic_direct_setup() corrupt the aplics list or register duplicate CPU
hotplug states?

Should the driver-global state in these paths be serialized with a lock, or
should aplic_driver set probe_type to PROBE_FORCE_SYNCHRONOUS (while still
dropping __ro_after_init from aplic_syscore_registered to support deferred
probing after init)?

>  
>  static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-aplic-async-probe-v4-1-68bc35e0ece1@xxxxxxxxxxxx?part=1