Re: [PATCH 2/5] irqchip/gic-v5: Clear per-CPU IRS data on teardown

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Unconditional call to irq_domain_remove() with a potentially NULL pointer in gicv5_free_lpi_domain() during error teardown causes a fatal kernel panic.
- [High] Missing release of requested memory regions on initialization error paths and module removal results in a permanent physical memory leak.
- [High] The ACPI MADT generic interrupt parser does not validate if get_logical_index() successfully found a logical CPU index, leading to out-of-bounds memory access.
- [High] Using smp_processor_id() in preemptible context triggers a BUG and can lead to stale per-CPU data access.
- [High] The GICv5 IRS hardware is left enabled on initialization error and teardown paths.
--

--- Patch [2]: [PATCH 2/5] irqchip/gic-v5: Clear per-CPU IRS data on teardown ---
commit 280b220e4ca7867e34828e36c13d55683f188fae
Author: Sascha Bischoff <[email protected]>

irqchip/gic-v5: Clear per-CPU IRS data on teardown

IRS affinity setup publishes an IRS pointer and IAFFID state before
the remaining IRS initialization can fail. This commit adds a teardown
function to clear the published state to prevent CPUs from being associated
with freed memory on initialization failure and module removal.

Link: https://sashiko.dev/#/patchset/[email protected]?part=2

> diff --git a/drivers/irqchip/irq-gic-v5-irs.c b/drivers/irqchip/irq-gic-v5-irs.c
> index f3fce0b1e25d9..67940ea9e39b7 100644
> --- a/drivers/irqchip/irq-gic-v5-irs.c
> +++ b/drivers/irqchip/irq-gic-v5-irs.c

[ ... ]

> @@ -773,6 +786,7 @@ static int __init gicv5_irs_of_init(struct device_node *node)
>  	return ret;
>  
>  out_iomem:
> +	gicv5_irs_clear_affinity(irs_data);
>  	iounmap(irs_base);

[Severity: High]
This is a pre-existing issue, but should the IRS hardware be explicitly
disabled before we unmap the registers?

The error and teardown paths call iounmap(irs_base) and free the structures,
but there doesn't seem to be a step to clear GICV5_IRS_CR0_IRSEN. Does this
abandon active DMA-capable hardware?

[Severity: High]
This is a pre-existing issue, but are we missing a call to
release_mem_region() on these teardown paths?

The setup path uses of_io_request_and_map() which internally requests a
memory region, but the cleanup paths only call iounmap(). Does this leave the
physical memory permanently stranded in the iomem tree?

>  out_err:
>  	kfree(irs_data);
> @@ -787,6 +801,7 @@ void __init gicv5_irs_remove(void)
>  	struct gicv5_irs_chip_data *irs_data, *tmp_data;
>  
>  	gicv5_free_lpi_domain();

[Severity: High]
This is a pre-existing issue, but does gicv5_free_lpi_domain() safely handle
a NULL domain?

Looking at drivers/irqchip/irq-gic-v5.c:gicv5_free_lpi_domain(), it
unconditionally calls irq_domain_remove(gicv5_global_data.lpi_domain). If
initialization fails early leaving lpi_domain uninitialized, could this
trigger a kernel panic during teardown?

>  	gicv5_deinit_lpis();
>  
>  	list_for_each_entry_safe(irs_data, tmp_data, &irs_nodes, entry) {
> +		gicv5_irs_clear_affinity(irs_data);
>  		iounmap(irs_data->irs_base);
>  		list_del(&irs_data->entry);
>  		kfree(irs_data);

[ ... ]

[Severity: High]
This is a pre-existing issue, but does gic_acpi_parse_iaffid() validate the
return value of get_logical_index()?

drivers/irqchip/irq-gic-v5-irs.c:gic_acpi_parse_iaffid() {
    ...
    cpu = get_logical_index(gicc->arm_mpidr);
    ...
}

If get_logical_index() fails and returns a negative error code, could this
lead to out-of-bounds memory corruption when cpu is used as an array index?

[Severity: High]
This is a pre-existing issue, but is it safe to use smp_processor_id() in
gicv5_irs_iste_alloc()?

drivers/irqchip/irq-gic-v5-irs.c:gicv5_irs_iste_alloc() {
    ...
    irs_data = per_cpu(per_cpu_irs_data, smp_processor_id());
    ...
}

Since this function executes in a sleepable context bound by GFP_KERNEL
constraints, could using smp_processor_id() without preemption disabled
trigger a BUG splat or lead to stale per-CPU data access?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2