[PATCH v2] irqchip: crossbar: Fix data race in allocate_gic_irq

[PATCH v2] irqchip: crossbar: Fix data race in allocate_gic_irq

[Bhargav Joshi]

In allocate_gic_irq(), if irq_domain_alloc_irqs_parent() fails, the
error path resets cb->irq_map[i] to IRQ_FREE. It modifies cb->irq_map[]
without holding cb->lock. modifying without lock could cause data race.

Fix this by acquiring raw_spin_lock around cb->irq_map[] modification.

Fixes: 783d31863fb8 ("irqchip: crossbar: Convert dra7 crossbar to stacked domains")

Signed-off-by: Bhargav Joshi <j.bhargav.u@gmail.com>
---
This bug was flagged by the Sashiko AI bot during the review process for
the DT schema conversion of ti,irq-crossbar binding.
https://lore.kernel.org/linux-devicetree/20260605210647.CCC881F00893@smtp.kernel.org/
---
Changes in v2:
- Fixed typo in spin_unlock
- Link to v1: https://patch.msgid.link/20260610-irq-spinlock-fix-v1-1-6f227ea9fa34@gmail.com
---
 drivers/irqchip/irq-crossbar.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index cd1134101ace..3d8bb37c9141 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -100,8 +100,11 @@ static int allocate_gic_irq(struct irq_domain *domain, unsigned virq,
 	fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
 
 	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
-	if (err)
+	if (err) {
+		raw_spin_lock(&cb->lock);
 		cb->irq_map[i] = IRQ_FREE;
+		raw_spin_unlock(&cb->lock);
+	}
 	else
 		cb->write(i, hwirq);
 

---
base-commit: 2d3090a8aeb596a26935db0955d46c9a5db5c6ce
change-id: 20260610-irq-spinlock-fix-1c90d8bc0f13

Best regards,
-- 
Bhargav

Re: [PATCH v2] irqchip: crossbar: Fix data race in allocate_gic_irq

[Thomas Gleixner]

On Wed, Jun 10 2026 at 16:44, Bhargav Joshi wrote:

Subject prefix is: irqchip/crossbar: .....

allocate_git_irq is a function and wants to be denoted as such. See

  https://docs.kernel.org/process/maintainer-tip.html

> In allocate_gic_irq(), if irq_domain_alloc_irqs_parent() fails, the
> error path resets cb->irq_map[i] to IRQ_FREE. It modifies cb->irq_map[]
> without holding cb->lock. modifying without lock could cause data race.

That's not a useful explanation. You fail to tell what races against it
and what the side effects of a racy access are.

> Fix this by acquiring raw_spin_lock around cb->irq_map[] modification.
>
> Fixes: 783d31863fb8 ("irqchip: crossbar: Convert dra7 crossbar to stacked domains")
>

Pointless newline.

> Signed-off-by: Bhargav Joshi <j.bhargav.u@gmail.com>
> ---
> This bug was flagged by the Sashiko AI bot during the review process for
> the DT schema conversion of ti,irq-crossbar binding.
> https://lore.kernel.org/linux-devicetree/20260605210647.CCC881F00893@smtp.kernel.org/

Interesting. Because Sashiko said so, there is a bug, right?

Did you actually analyze whether there is a real bug or Sashiko just
pointing out something what looks like a bug? I don't think so otherwise
you could explain what the real problem with that unlocked access is and
ideally you could explain what the lock actually protects.

It protects nothing at all because interrupt domain allocation/free
operations related to a particular domain and the related hierarchy down
to the root domain are serialized in the core code already. So there is
_zero_ concurrency possible. The lock is a leftover from the original
code which predates hierarchical interrupt domains and modern core
locking and only provides voodoo protection.

So the real solution here is to remove the lock completely.

> ---
> Changes in v2:
> - Fixed typo in spin_unlock
> - Link to v1: https://patch.msgid.link/20260610-irq-spinlock-fix-v1-1-6f227ea9fa34@gmail.com
> ---
>  drivers/irqchip/irq-crossbar.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index cd1134101ace..3d8bb37c9141 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -100,8 +100,11 @@ static int allocate_gic_irq(struct irq_domain *domain, unsigned virq,
>  	fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
>  
>  	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
> -	if (err)
> +	if (err) {
> +		raw_spin_lock(&cb->lock);
>  		cb->irq_map[i] = IRQ_FREE;
> +		raw_spin_unlock(&cb->lock);

Not that it matters, but this should use guard() or scoped_guard()

> +	}
>  	else
>  		cb->write(i, hwirq);

If you add brackets to the if () part, you need to add them to the else
part too even if there is only a single code line. This

     if () {
        ....
     }
     else
        ...

is asymmetric and unreadable gunk.

Re: [PATCH v2] irqchip: crossbar: Fix data race in allocate_gic_irq

[Bhargav Joshi]

Hi,

On Thu, Jun 11, 2026 at 8:13 PM Thomas Gleixner <tglx@kernel.org> wrote:
>
> On Wed, Jun 10 2026 at 16:44, Bhargav Joshi wrote:
>
> Subject prefix is: irqchip/crossbar: .....
>
> allocate_git_irq is a function and wants to be denoted as such. See
>
>   https://docs.kernel.org/process/maintainer-tip.html
>
> > In allocate_gic_irq(), if irq_domain_alloc_irqs_parent() fails, the
> > error path resets cb->irq_map[i] to IRQ_FREE. It modifies cb->irq_map[]
> > without holding cb->lock. modifying without lock could cause data race.
>
> That's not a useful explanation. You fail to tell what races against it
> and what the side effects of a racy access are.
>
> > Fix this by acquiring raw_spin_lock around cb->irq_map[] modification.
> >
> > Fixes: 783d31863fb8 ("irqchip: crossbar: Convert dra7 crossbar to stacked domains")
> >
>
> Pointless newline.
>
> > Signed-off-by: Bhargav Joshi <j.bhargav.u@gmail.com>
> > ---
> > This bug was flagged by the Sashiko AI bot during the review process for
> > the DT schema conversion of ti,irq-crossbar binding.
> > https://lore.kernel.org/linux-devicetree/20260605210647.CCC881F00893@smtp.kernel.org/
>
> Interesting. Because Sashiko said so, there is a bug, right?
No, my initial analysis considered two concurrent allocate_gic_irq()
calls one failing irq_domain_alloc_irqs_parent() and writing
cb->irq_map[i] = IRQ_FREE without the lock, while the other is scanning
the array inside the lock in loop. That would be an unlocked write
concurrent with a locked read.
But I missed that the IRQ domain core serializes .alloc()/.free().
Apologies for that. I will drop this patch.

Best Regards,
Bhargav