Re: [PATCH 02/16] irqchip/loongarch-ir: fix redirect free and alloc leaks

[Radu Rendec <radu@rendec.net>]

On Tue, 2026-07-14 at 20:23 +0800, Haofeng Li wrote:
> redirect_irde_free() ignores its irde argument and always frees
> redirect_descs (node 0). Multi-node teardown therefore leaks non-zero
> nodes.
> 
> On the alloc path, a failed redirect_table_alloc() after
> irq_domain_alloc_irqs_parent() returns without freeing parent IRQs.
> A mid-loop gpid allocation failure also leaks the current item (chip_data
> is not set yet) and never releases the bitmap region obtained from
> bitmap_find_free_region().
> 
> Free &irde->ird_table and &irde->inv_queue, free parent IRQs on table
> alloc failure, kfree(item) on gpid failure, and release the full bitmap
> region on the error path.
> 
> Fixes: 71619266e0a2 ("irqchip/loongarch-ir: Add IR (interrupt redirection) irqchip support")
> Signed-off-by: Haofeng Li <lihaofeng@kylinos.cn>
> ---
>  drivers/irqchip/irq-loongarch-ir.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-loongarch-ir.c b/drivers/irqchip/irq-loongarch-ir.c
> index 21c649a89a70..695dfef6570a 100644
> --- a/drivers/irqchip/irq-loongarch-ir.c
> +++ b/drivers/irqchip/irq-loongarch-ir.c
> @@ -219,6 +219,20 @@ static int redirect_table_alloc(int node, u32 nr_irqs)
>   return index;
>  }
>  
> +static void redirect_table_free_region(int node, int index, u32 nr_irqs)
> +{
> + struct redirect_table *ird_table = &redirect_descs[node].ird_table;
> + int order = 0;
> +
> + if (nr_irqs > 1) {
> + nr_irqs = __roundup_pow_of_two(nr_irqs);
> + order = ilog2(nr_irqs);
> + }
> +
> + guard(raw_spinlock_irqsave)(&ird_table->lock);
> + bitmap_release_region(ird_table->bitmap, index, order);
> +}
> +
>  static void redirect_table_free(struct redirect_item *item)
>  {
>   struct redirect_table *ird_table = &item->irde->ird_table;
> @@ -324,6 +338,7 @@ static int redirect_domain_alloc(struct irq_domain *domain, unsigned int virq,
>   index = redirect_table_alloc(node, nr_irqs);
>   if (index < 0) {
>   pr_err("Alloc redirect table entry failed\n");
> + irq_domain_free_irqs_parent(domain, virq, nr_irqs);
>   return -EINVAL;
>   }
>  
> @@ -347,6 +362,7 @@ static int redirect_domain_alloc(struct irq_domain *domain, unsigned int virq,
>   item->gpid = kzalloc_node(sizeof(*item->gpid), GFP_KERNEL, node);
>   if (!item->gpid) {
>   pr_err("Alloc redirect GPID failed\n");
> + kfree(item);
>   goto out_free_resources;
>   }
>   item->index = index + i;
> @@ -361,6 +377,7 @@ static int redirect_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  
>  out_free_resources:
>   redirect_free_resources(domain, virq, nr_irqs);
> + redirect_table_free_region(node, index, nr_irqs);

I think this is a bit more subtle. The bitmap entries are already
released via redirect_free_resources() -> redirect_table_free() but the
problem is some of them may not released, for one of the following
reasons:
   1. The allocation loop above has ended early due to a kzalloc()
      failure.
   2. nr_irqs is not a power of 2 and has been rounded up in
      redirect_table_alloc(), making the actual bitmap region larger.

Your proposed change addresses both scenarios, but makes the spinlock
locking and the clear_bit() in redirect_table_free() redundant. I'm
more concerned about the spinlock locking, because it also disables the
interrupts - for each step of the loop in redirect_free_resources().

On the other hand, I think a bitmap entry leak is still possible in
redirect_domain_free(), which you're not addressing. For example,
suppose redirect_domain_alloc() is called with nr_irqs=5 and is
successful. That allocates 8 entries in the bitmap. Then, even if
redirect_domain_free() is called with the same virq and nr_irqs (which,
by the way, is not guaranteed), only the first 5 entries of that region
of 8 will be released.

My point is this driver has a deeper design flaw around that bitmap. In
reality, it probably doesn't matter because the bitmap is huge (64K
bits), and interrupts are unlikely to be removed and reallocated in
large enough amounts to exhaust the bitmap. But if you really want to
fix this, better address the underlying design flaw and fix it
properly.

>   irq_domain_free_irqs_common(domain, virq, nr_irqs);
>  
>   return -ENOMEM;
> @@ -436,8 +453,8 @@ static void redirect_irde_cfg(struct redirect_desc *irde)
>  
>  static void __init redirect_irde_free(struct redirect_desc *irde)
>  {
> - struct redirect_table *ird_table = &redirect_descs->ird_table;
> - struct redirect_queue *inv_queue = &redirect_descs->inv_queue;
> + struct redirect_table *ird_table = &irde->ird_table;
> + struct redirect_queue *inv_queue = &irde->inv_queue;
>  
>   if (ird_table->table) {
>   folio_put(virt_to_folio(ird_table->table));