From: Alexander Sverdlin alexander.sverdlin@nokia.com
irq_create_fwspec_mapping() can race with itself during IRQ trigger type configuration. Possible scenarios include:
- Mapping exists, two irq_create_fwspec_mapping() running in parallel do not detect type mismatch, IRQ remains configured with one of the different trigger types randomly - Second call to irq_create_fwspec_mapping() sees existing mapping just created by first call, but earlier irqd_set_trigger_type() call races with later irqd_set_trigger_type() => totally undetected, IRQ type is being set randomly to either one or another type
Introduce helper function to detect parallel changes to IRQ type.
Cc: stable@vger.kernel.org Signed-off-by: Alexander Sverdlin alexander.sverdlin@nokia.com --- kernel/irq/irqdomain.c | 66 +++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 28 deletions(-)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 176f2cc..af4d30c 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -764,10 +764,45 @@ static void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args, fwspec->param[i] = args[i]; }
+/* Detect races during IRQ type setting */ +static int irq_set_trigger_type_locked(unsigned int virq, unsigned int type, + irq_hw_number_t hwirq, + const struct irq_fwspec *fwspec) +{ + struct irq_data *irq_data; + int ret = 0; + + mutex_lock(&irq_domain_mutex); + /* + * If the trigger type is not specified or matches the current trigger + * type then we are done. + */ + if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq)) + goto unlock; + + /* If the trigger type has not been set yet, then set it now */ + if (irq_get_trigger_type(virq) != IRQ_TYPE_NONE) { + pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n", + hwirq, of_node_full_name(to_of_node(fwspec->fwnode))); + ret = -EINVAL; + goto unlock; + } + + irq_data = irq_get_irq_data(virq); + if (!irq_data) { + ret = -ENOENT; + goto unlock; + } + irqd_set_trigger_type(irq_data, type); + +unlock: + mutex_unlock(&irq_domain_mutex); + return ret; +} + unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) { struct irq_domain *domain; - struct irq_data *irq_data; irq_hw_number_t hwirq; unsigned int type = IRQ_TYPE_NONE; int virq; @@ -802,29 +837,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) */ 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)) - return virq; - - /* - * 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) - return 0; - - irqd_set_trigger_type(irq_data, type); + if (!irq_set_trigger_type_locked(virq, type, hwirq, fwspec)) return virq; - } - - pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n", - hwirq, of_node_full_name(to_of_node(fwspec->fwnode))); return 0; }
@@ -839,8 +853,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) return virq; }
- irq_data = irq_get_irq_data(virq); - if (!irq_data) { + if (irq_set_trigger_type_locked(virq, type, hwirq, fwspec)) { if (irq_domain_is_hierarchy(domain)) irq_domain_free_irqs(virq, 1); else @@ -848,9 +861,6 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) return 0; }
- /* Store trigger type */ - irqd_set_trigger_type(irq_data, type); - return virq; } EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
On 12/09/2019 10:44, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
From: Alexander Sverdlin alexander.sverdlin@nokia.com
irq_create_fwspec_mapping() can race with itself during IRQ trigger type configuration. Possible scenarios include:
- Mapping exists, two irq_create_fwspec_mapping() running in parallel do not detect type mismatch, IRQ remains configured with one of the different trigger types randomly
- Second call to irq_create_fwspec_mapping() sees existing mapping just created by first call, but earlier irqd_set_trigger_type() call races with later irqd_set_trigger_type() => totally undetected, IRQ type is being set randomly to either one or another type
Is that an actual thing? Frankly, the scenario you're describing here seems to carry the hallmarks of a completely broken system. Can you point at a system supported in mainline that would behave as such?
Thanks,
M.
Hi!
On 20/09/2019 18:07, Marc Zyngier wrote:
irq_create_fwspec_mapping() can race with itself during IRQ trigger type configuration. Possible scenarios include:
- Mapping exists, two irq_create_fwspec_mapping() running in parallel do not detect type mismatch, IRQ remains configured with one of the different trigger types randomly
- Second call to irq_create_fwspec_mapping() sees existing mapping just created by first call, but earlier irqd_set_trigger_type() call races with later irqd_set_trigger_type() => totally undetected, IRQ type is being set randomly to either one or another type
Is that an actual thing? Frankly, the scenario you're describing here seems to carry the hallmarks of a completely broken system. Can you point at a system supported in mainline that would behave as such?
Briefly speaking, this race is about not-complaining in case of a broken device tree. This is not something purely theoretical. I don't know if DTs under arch/arm/boot/dts are all correct, but I saw a lot DTs from silicone vendors and very little of them were 100% correct.
In other words, this patch repairs error-handling. With 100% correct DTs (or ACPI tables, have you seen one 100% correct BTW? :)) it's not required.
linux-stable-mirror@lists.linaro.org