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

On Mon, Aug 10, 2026 at 10:47:47AM +0000, [email protected] wrote:
> 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.

Looking into those.

Thanks,
Lorenzo

> --
> 
> --- 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