[v4,09/19] irqdomain: Fix mapping-creation race
Commit Message
Parallel probing (e.g. due to asynchronous probing) of devices that share
interrupts can currently result in two mappings for the same hardware
interrupt to be created.
Make sure to hold the irq_domain_mutex when creating mappings so that
looking for an existing mapping before creating a new one is done
atomically.
Fixes: 765230b5f084 ("driver-core: add asynchronous probing support for drivers")
Fixes: b62b2cf5759b ("irqdomain: Fix handling of type settings for existing mappings")
Link: https://lore.kernel.org/r/YuJXMHoT4ijUxnRb@hovoldconsulting.com
Cc: Dmitry Torokhov <dtor@chromium.org>
Cc: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
kernel/irq/irqdomain.c | 47 ++++++++++++++++++++++++++++++++----------
1 file changed, 36 insertions(+), 11 deletions(-)
Comments
On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> Parallel probing (e.g. due to asynchronous probing) of devices that share
> interrupts can currently result in two mappings for the same hardware
> interrupt to be created.
This lacks an explanation why this can happen.
> @@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
> type &= IRQ_TYPE_SENSE_MASK;
>
> + mutex_lock(&irq_domain_mutex);
> +
> /*
> * If we've already configured this interrupt,
> * don't do it again, or hell will break loose.
> @@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> * interrupt number.
> */
> if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> - return virq;
> + goto out;
>
> /*
> * If the trigger type has not been set yet, then set
> @@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> irq_data = irq_get_irq_data(virq);
> if (!irq_data)
> - return 0;
> + goto err;
>
> irqd_set_trigger_type(irq_data, type);
> - return virq;
> + goto out;
> }
>
> pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
> hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> - return 0;
> + goto err;
> }
>
> if (irq_domain_is_hierarchy(domain)) {
> - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> + virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> + fwspec, false, NULL);
> if (virq <= 0)
> - return 0;
> + goto err;
> } else {
> /* Create mapping */
> virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
> if (!virq)
> - return virq;
> + goto err;
> }
>
> irq_data = irq_get_irq_data(virq);
> if (WARN_ON(!irq_data))
> - return 0;
> + goto err;
>
> /* Store trigger type */
> irqd_set_trigger_type(irq_data, type);
> +out:
> + mutex_unlock(&irq_domain_mutex);
>
> return virq;
> +err:
> + mutex_unlock(&irq_domain_mutex);
> +
> + return 0;
> }
> EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
You can spare that goto churn by renaming the existing function to
irq_create_fwspec_mapping_locked() and invoked that guarded by the
mutex, no?
Thanks,
tglx
On Tue, Jan 17, 2023 at 10:39:51PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
>
> > Parallel probing (e.g. due to asynchronous probing) of devices that share
> > interrupts can currently result in two mappings for the same hardware
> > interrupt to be created.
>
> This lacks an explanation why this can happen.
The explanation is there (parallel probing), but I can amend it with the
concrete example from the input subsystem if that's what you're after?
Or do you mean that the above doesn't say that the current locking is
incomplete? I believe that's covered by the next paragraph:
Make sure to hold the irq_domain_mutex when creating mappings so that
looking for an existing mapping before creating a new one is done
atomically.
> > @@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
> > type &= IRQ_TYPE_SENSE_MASK;
> >
> > + mutex_lock(&irq_domain_mutex);
> > +
> > /*
> > * If we've already configured this interrupt,
> > * don't do it again, or hell will break loose.
> > @@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > * interrupt number.
> > */
> > if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> > - return virq;
> > + goto out;
> >
> > /*
> > * If the trigger type has not been set yet, then set
> > @@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> > irq_data = irq_get_irq_data(virq);
> > if (!irq_data)
> > - return 0;
> > + goto err;
> >
> > irqd_set_trigger_type(irq_data, type);
> > - return virq;
> > + goto out;
> > }
> >
> > pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
> > hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> > - return 0;
> > + goto err;
> > }
> >
> > if (irq_domain_is_hierarchy(domain)) {
> > - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> > + virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> > + fwspec, false, NULL);
> > if (virq <= 0)
> > - return 0;
> > + goto err;
> > } else {
> > /* Create mapping */
> > virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
> > if (!virq)
> > - return virq;
> > + goto err;
> > }
> >
> > irq_data = irq_get_irq_data(virq);
> > if (WARN_ON(!irq_data))
> > - return 0;
> > + goto err;
> >
> > /* Store trigger type */
> > irqd_set_trigger_type(irq_data, type);
> > +out:
> > + mutex_unlock(&irq_domain_mutex);
> >
> > return virq;
> > +err:
> > + mutex_unlock(&irq_domain_mutex);
> > +
> > + return 0;
> > }
> > EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
>
> You can spare that goto churn by renaming the existing function to
> irq_create_fwspec_mapping_locked() and invoked that guarded by the
> mutex, no?
That may be possible, but I'm not sure it's better. It wasn't really
obvious which of the above returns where error paths and which were
success paths before this change.
I'll try and see what it would look like.
Johan
On Wed, Jan 18, 2023 at 10:40:57AM +0100, Johan Hovold wrote:
> On Tue, Jan 17, 2023 at 10:39:51PM +0100, Thomas Gleixner wrote:
> > On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > > @@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > > if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
> > > type &= IRQ_TYPE_SENSE_MASK;
> > >
> > > + mutex_lock(&irq_domain_mutex);
> > > +
> > > /*
> > > * If we've already configured this interrupt,
> > > * don't do it again, or hell will break loose.
> > > @@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > > * interrupt number.
> > > */
> > > if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> > > - return virq;
> > > + goto out;
> > >
> > > /*
> > > * If the trigger type has not been set yet, then set
> > > @@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > > if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> > > irq_data = irq_get_irq_data(virq);
> > > if (!irq_data)
> > > - return 0;
> > > + goto err;
> > >
> > > irqd_set_trigger_type(irq_data, type);
> > > - return virq;
> > > + goto out;
> > > }
> > >
> > > pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
> > > hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> > > - return 0;
> > > + goto err;
> > > }
> > >
> > > if (irq_domain_is_hierarchy(domain)) {
> > > - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> > > + virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> > > + fwspec, false, NULL);
> > > if (virq <= 0)
> > > - return 0;
> > > + goto err;
> > > } else {
> > > /* Create mapping */
> > > virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
> > > if (!virq)
> > > - return virq;
> > > + goto err;
> > > }
> > >
> > > irq_data = irq_get_irq_data(virq);
> > > if (WARN_ON(!irq_data))
> > > - return 0;
> > > + goto err;
> > >
> > > /* Store trigger type */
> > > irqd_set_trigger_type(irq_data, type);
> > > +out:
> > > + mutex_unlock(&irq_domain_mutex);
> > >
> > > return virq;
> > > +err:
> > > + mutex_unlock(&irq_domain_mutex);
> > > +
> > > + return 0;
> > > }
> > > EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> >
> > You can spare that goto churn by renaming the existing function to
> > irq_create_fwspec_mapping_locked() and invoked that guarded by the
> > mutex, no?
>
> That may be possible, but I'm not sure it's better. It wasn't really
> obvious which of the above returns where error paths and which were
> success paths before this change.
>
> I'll try and see what it would look like.
I gave it a try but the resulting diff is even bigger and also obscures
the reason for why the locking is there (i.e. that the lookup and and
creation needs to be done atomically).
Adding unlocked domain lookup helpers as a preparatory patch could
possible help somewhat with the diff size, but that doesn't really make
sense with the per-domain locking added by the final patch.
(I've made some experiments to quantify the parallel probe speed up that
using per-domain locks can result in so I'm including that for v5.)
Johan
@@ -25,6 +25,9 @@ static DEFINE_MUTEX(irq_domain_mutex);
static struct irq_domain *irq_default_domain;
+static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+ unsigned int nr_irqs, int node, void *arg,
+ bool realloc, const struct irq_affinity_desc *affinity);
static void irq_domain_check_hierarchy(struct irq_domain *domain);
struct irqchip_fwid {
@@ -692,7 +695,7 @@ static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
return 0;
}
- if (irq_domain_associate(domain, virq, hwirq)) {
+ if (__irq_domain_associate(domain, virq, hwirq)) {
irq_free_desc(virq);
return 0;
}
@@ -728,14 +731,20 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
return 0;
}
+ mutex_lock(&irq_domain_mutex);
+
/* Check if mapping already exists */
virq = irq_find_mapping(domain, hwirq);
if (virq) {
pr_debug("existing mapping on virq %d\n", virq);
- return virq;
+ goto out;
}
- return __irq_create_mapping_affinity(domain, hwirq, affinity);
+ virq = __irq_create_mapping_affinity(domain, hwirq, affinity);
+out:
+ mutex_unlock(&irq_domain_mutex);
+
+ return virq;
}
EXPORT_SYMBOL_GPL(irq_create_mapping_affinity);
@@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
type &= IRQ_TYPE_SENSE_MASK;
+ mutex_lock(&irq_domain_mutex);
+
/*
* If we've already configured this interrupt,
* don't do it again, or hell will break loose.
@@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
* interrupt number.
*/
if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
- return virq;
+ goto out;
/*
* If the trigger type has not been set yet, then set
@@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
irq_data = irq_get_irq_data(virq);
if (!irq_data)
- return 0;
+ goto err;
irqd_set_trigger_type(irq_data, type);
- return virq;
+ goto out;
}
pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
- return 0;
+ goto err;
}
if (irq_domain_is_hierarchy(domain)) {
- virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
+ virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
+ fwspec, false, NULL);
if (virq <= 0)
- return 0;
+ goto err;
} else {
/* Create mapping */
virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
if (!virq)
- return virq;
+ goto err;
}
irq_data = irq_get_irq_data(virq);
if (WARN_ON(!irq_data))
- return 0;
+ goto err;
/* Store trigger type */
irqd_set_trigger_type(irq_data, type);
+out:
+ mutex_unlock(&irq_domain_mutex);
return virq;
+err:
+ mutex_unlock(&irq_domain_mutex);
+
+ return 0;
}
EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
@@ -1877,6 +1895,13 @@ void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
irq_set_handler_data(virq, handler_data);
}
+static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+ unsigned int nr_irqs, int node, void *arg,
+ bool realloc, const struct irq_affinity_desc *affinity)
+{
+ return -EINVAL;
+}
+
static void irq_domain_check_hierarchy(struct irq_domain *domain)
{
}