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

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

[Vladimir Kondratiev]

The APLIC driver leaves probe_type unset, so it follows
PROBE_DEFAULT_STRATEGY and may be probed asynchronously when the
command line requests it. Multiple APLIC instances can therefore
enter aplic_syscore_init() at the same time. The check-then-set of
aplic_syscore_registered is not atomic: two threads can both observe
the flag as false and both call register_syscore(), inserting the
same list node twice. That trips CHECK_DATA_CORRUPTION
("list_add double add") in __list_add_valid_or_report().

The flag is also marked __ro_after_init, but aplic_probe() can run
after init. With MSI, aplic_msi_setup() returns -EPROBE_DEFER while
the MSI parent is not ready, so the later successful probe writes the
flag after it has been made read-only. That write fault exists even
without asynchronous probing.

Replace the flag with an atomic and claim registration with
atomic_cmpxchg(), so only one caller registers the syscore ops and
the store remains writable after init.

Fixes: b330fbfd34d7 ("irqchip/riscv-aplic: Register syscore operations only once")
Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
---
Changes in v4:
- rework commit message
- rebase onto v7.3
- Link to v3: https://lore.kernel.org/r/20261012-aplic-async-probe-v3-1-5cf52dc28690@mobileye.com

Changes in v3:
- drop __ro_after_init attribute due to write to read-only page concern
- Link to v2: https://lore.kernel.org/r/20261012-aplic-async-probe-v2-1-868d7684c9d6@mobileye.com

Changes in v2:
- fixed potential access to read-only page
- Link to v1: https://lore.kernel.org/r/20261012-aplic-async-probe-v1-1-4c2c4f5b0916@mobileye.com

To: Anup Patel <anup@brainfault.org>
To: Thomas Gleixner <tglx@kernel.org>
To: Radu Rendec <radu@rendec.net>
To: Paul Walmsley <pjw@kernel.org>
To: Palmer Dabbelt <palmer@dabbelt.com>
To: Albert Ou <aou@eecs.berkeley.edu>
To: Alexandre Ghiti <alex@ghiti.fr>
To: Jessica Liu <liu.xuemei1@zte.com.cn>
Cc: linux-riscv@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/irqchip/irq-riscv-aplic-main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
index d9afb6ae98cf..54e1d5602f15 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;
-	}
 }
 
 static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)

---
base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
change-id: 20261012-aplic-async-probe-81624e035607

Best regards,
--  
Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

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

[Vladimir Kondratiev]

The APLIC driver leaves probe_type unset, so it follows
PROBE_DEFAULT_STRATEGY and may be probed asynchronously when the
command line requests it. Multiple APLIC instances can therefore
enter aplic_syscore_init() at the same time. The check-then-set of
aplic_syscore_registered is not atomic: two threads can both observe
the flag as false and both call register_syscore(), inserting the
same list node twice. That trips CHECK_DATA_CORRUPTION
("list_add double add") in __list_add_valid_or_report().

The flag is also marked __ro_after_init, but aplic_probe() can run
after init. With MSI, aplic_msi_setup() returns -EPROBE_DEFER while
the MSI parent is not ready, so the later successful probe writes the
flag after it has been made read-only. That write fault exists even
without asynchronous probing.

Replace the flag with an atomic and claim registration with
atomic_cmpxchg(), so only one caller registers the syscore ops and
the store remains writable after init.

Fixes: b330fbfd34d7 ("irqchip/riscv-aplic: Register syscore operations only once")
Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
---
Changes in v4:
- rework commit message
- rebase onto v7.3
- Link to v3: https://lore.kernel.org/r/20261012-aplic-async-probe-v3-1-5cf52dc28690@mobileye.com

Changes in v3:
- drop __ro_after_init attribute due to write to read-only page concern
- Link to v2: https://lore.kernel.org/r/20261012-aplic-async-probe-v2-1-868d7684c9d6@mobileye.com

Changes in v2:
- fixed potential access to read-only page
- Link to v1: https://lore.kernel.org/r/20261012-aplic-async-probe-v1-1-4c2c4f5b0916@mobileye.com

To: Anup Patel <anup@brainfault.org>
To: Thomas Gleixner <tglx@kernel.org>
To: Radu Rendec <radu@rendec.net>
To: Paul Walmsley <pjw@kernel.org>
To: Palmer Dabbelt <palmer@dabbelt.com>
To: Albert Ou <aou@eecs.berkeley.edu>
To: Alexandre Ghiti <alex@ghiti.fr>
To: Jessica Liu <liu.xuemei1@zte.com.cn>
Cc: linux-riscv@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/irqchip/irq-riscv-aplic-main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
index d9afb6ae98cf..54e1d5602f15 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;
-	}
 }
 
 static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)

---
base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
change-id: 20261012-aplic-async-probe-81624e035607

Best regards,
--  
Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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@mobileye.com>

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@mobileye.com?part=1