linux-kernel - Re: [PATCH v12 14/25] irqchip/sifive-plic: Convert PLIC driver into a platform driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 16 Feb 2024 16:33:23 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Anup Patel <apatel@...tanamicro.com>, Palmer Dabbelt
 <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, Rob
 Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Frank Rowand
 <frowand.list@...il.com>, Conor Dooley <conor+dt@...nel.org>
Cc: Marc Zyngier <maz@...nel.org>, Björn Töpel
 <bjorn@...nel.org>, Atish
 Patra <atishp@...shpatra.org>, Andrew Jones <ajones@...tanamicro.com>,
 Sunil V L <sunilvl@...tanamicro.com>, Saravana Kannan
 <saravanak@...gle.com>, Anup Patel <anup@...infault.org>,
 linux-riscv@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, Anup Patel
 <apatel@...tanamicro.com>
Subject: Re: [PATCH v12 14/25] irqchip/sifive-plic: Convert PLIC driver into
 a platform driver

On Sat, Jan 27 2024 at 21:47, Anup Patel wrote:
> +	priv->irqdomain = irq_domain_create_linear(dev->fwnode, nr_irqs + 1,
> +						   &plic_irqdomain_ops, priv);
> +	if (WARN_ON(!priv->irqdomain))
> +		return -ENOMEM;

While some of the stuff is cleaned up by devm, the error handling in
this code looks pretty fragile as it leaves initialized contexts,
hardware state, chained handlers etc. around.

The question is whether the system can actually boot or work at all if
any of this fails.

> +
>  	/*
>  	 * We can have multiple PLIC instances so setup cpuhp state
> -	 * and register syscore operations only when context handler
> -	 * for current/boot CPU is present.
> +	 * and register syscore operations only after context handlers
> +	 * of all online CPUs are initialized.
>  	 */
> -	handler = this_cpu_ptr(&plic_handlers);
> -	if (handler->present && !plic_cpuhp_setup_done) {
> +	cpuhp_setup = true;
> +	for_each_online_cpu(cpu) {
> +		handler = per_cpu_ptr(&plic_handlers, cpu);
> +		if (!handler->present) {
> +			cpuhp_setup = false;
> +			break;
> +		}
> +	}
> +	if (cpuhp_setup) {
>  		cpuhp_setup_state(CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING,
>  				  "irqchip/sifive/plic:starting",
>  				  plic_starting_cpu, plic_dying_cpu);
>  		register_syscore_ops(&plic_irq_syscore_ops);
> -		plic_cpuhp_setup_done = true;

I don't think that removing the setup protection is correct.

Assume you have maxcpus=N on the kernel command line, then the above
for_each_online_cpu() loop would result in cpuhp_setup == true when the
instances for the not onlined CPUs are set up, no?

Thanks,

        tglx