Re: [PATCH v1] genirq/msi: Fix the mapping race of wire to MSI interrupts

[Thomas Gleixner <tglx@kernel.org>]

On Fri, Oct 16 2026 at 22:54, Nickolai Zeldovich wrote:
> @@ -942,63 +942,61 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  	 * don't do it again, or hell will break loose.
>  	 */
>  	virq = irq_find_mapping(domain, hwirq);
> -	if (virq) {
> -		/*
> -		 * If the trigger type is not specified or matches the
> -		 * current trigger type then we are done so return the
> -		 * interrupt number.
> -		 */
> -		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> -			goto out;
> -
> -		/*
> -		 * If the trigger type has not been set yet, then set
> -		 * it now and return the interrupt number.
> -		 */
> -		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> -			irq_data = irq_get_irq_data(virq);
> -			if (!irq_data) {
> +	if (!virq) {
> +		if (irq_domain_is_hierarchy(domain)) {
> +			if (irq_domain_is_msi_device(domain)) {

Why on earth has all of this to be completely reordered so that the
actual change is hidden in all this churn?

Due to that I almost missed this subtle breakage:

> +	if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
>		goto out;
...
> +	/*
> +	 * If the trigger type has not been set yet, then set
> +	 * it now and return the interrupt number.
> +	 */
> +	if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {

For the first allocation one of the involved domains can set a default
type other than NONE in the allocation path. So if the domain default
type does not match the requested type then the initial type setting
won't happen.

+	/* Lost the race against a concurrent mapping of the same wire? */
+	virq = irq_find_mapping(domain, hwirq);
+	if (virq)
+		return virq;

That comment is really not sufficient. It's non-obvious that the
descriptor lock is sufficient to prevent concurrent modifications of the
mapping.

Something like the uncompiled below is less intrusive and avoids the
subtle breakage.

Thanks,

        tglx
---
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 4fdcb6df5306..10b0ac97b040 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -938,6 +938,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 
 	mutex_lock(&domain->root->mutex);
 
+retry:
 	/*
 	 * If we've already configured this interrupt,
 	 * don't do it again, or hell will break loose.
@@ -978,9 +979,17 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 			mutex_unlock(&domain->root->mutex);
 			virq = msi_device_domain_alloc_wired(domain, hwirq, type);
 			mutex_lock(&domain->root->mutex);
-		} else
+			/*
+			 * Handle the case where a concurrent mapping raced and
+			 * allocated the interrupt after the mutex_unlock()
+			 * above.
+			 */
+			if (virq == -EEXIST)
+				goto retry;
+		} else {
 			virq = irq_domain_alloc_irqs_locked(domain, -1, 1, NUMA_NO_NODE,
 							    fwspec, false, NULL);
+		}
 		if (virq <= 0) {
 			virq = 0;
 			goto out;
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index e1d85035dbc8..3fdad0063be6 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -1572,6 +1572,12 @@ struct msi_map msi_domain_alloc_irq_at(struct device *dev, unsigned int domid, u
  * msi_desc::cookie so the underlying interrupt chip and domain code can
  * retrieve it.
  *
+ * The caller looks @hwirq up before invoking this function, but it cannot
+ * hold the domain lock across the call. Allocations are serialized by the
+ * MSI descriptor lock, so the lookup is repeated under it and a mapping
+ * which a concurrent caller created in the meantime is returned instead
+ * of a second one.
+ *
  * Return: The Linux interrupt number (> 0) or an error code
  */
 int msi_device_domain_alloc_wired(struct irq_domain *domain, unsigned int hwirq,
@@ -1589,9 +1595,18 @@ int msi_device_domain_alloc_wired(struct irq_domain *domain, unsigned int hwirq,
 
 	guard(msi_descs_lock)(dev);
 	if (WARN_ON_ONCE(msi_get_device_domain(dev, domid) != domain))
-		map.index = -EINVAL;
-	else
-		map = __msi_domain_alloc_irq_at(dev, domid, MSI_ANY_INDEX, NULL, &icookie);
+		return -EINVAL;
+
+	/*
+	 * A concurrent mapping might have allocated @hwirq already after the
+	 * caller dropped the domain mutex. Re-evaluate under the MSI descriptor
+	 * lock, which is sufficient as it is the outermost lock for both
+	 * alloc() and free().
+	 */
+	if (irq_find_mapping(domain, hwirq) > 0)
+		return -EEXIST;
+
+	map = __msi_domain_alloc_irq_at(dev, domid, MSI_ANY_INDEX, NULL, &icookie);
 	return map.index >= 0 ? map.virq : map.index;
 }