* [PATCH v2 1/3] genirq/irqdomain: Check for existing mapping in irq_domain_associate()
2019-09-27 15:00 [PATCH v2 0/3] Fix irq_domain vs. irq user race Sverdlin, Alexander (Nokia - DE/Ulm)
@ 2019-09-27 15:00 ` Sverdlin, Alexander (Nokia - DE/Ulm)
2019-09-27 15:00 ` [PATCH v2 2/3] genirq/irqdomain: Re-check mapping after associate in irq_create_mapping() Sverdlin, Alexander (Nokia - DE/Ulm)
2019-09-27 15:00 ` [PATCH v2 3/3] genirq/irqdomain: Detect type race in irq_create_fwspec_mapping() Sverdlin, Alexander (Nokia - DE/Ulm)
2 siblings, 0 replies; 4+ messages in thread
From: Sverdlin, Alexander (Nokia - DE/Ulm) @ 2019-09-27 15:00 UTC (permalink / raw)
To: Marc Zyngier, Thomas Gleixner, linux-kernel, Grant Likely
Cc: Sverdlin, Alexander (Nokia - DE/Ulm),
Mark Brown, Jon Hunter, Glavinic-Pecotic, Matija (EXT - DE/Ulm),
Adamski, Krzysztof (Nokia - PL/Wroclaw),
Bachorski, Tomasz (Nokia - PL/Wroclaw),
Kosnikowski, Wojciech (Nokia - PL/Wroclaw),
stable
From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
irq_domain_associate() is the only place where irq_find_mapping() can be
used reliably (under irq_domain_mutex) to make a decision if the mapping
shall be created or not. Other calls to irq_find_mapping() (not under
any lock) cannot be used for this purpose and lead to race conditions in
particular inside irq_create_mapping().
Give the callers of irq_domain_associate() an ability to detect existing
domain reliably by examining the return value.
Reported-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
Reported-by: Tomasz Bachorski <tomasz.bachorski@nokia.com>
Reported-by: Wojciech Kosnikowski <wojciech.kosnikowski@nokia.com>
Cc: stable@vger.kernel.org
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
kernel/irq/irqdomain.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 132672b..ccbb048 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -545,6 +545,15 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
return -EINVAL;
mutex_lock(&irq_domain_mutex);
+
+ /* Check if mapping already exists */
+ if (irq_find_mapping(domain, hwirq)) {
+ mutex_unlock(&irq_domain_mutex);
+ pr_debug("%s: conflicting mapping for hwirq 0x%x\n",
+ domain->name, (int)hwirq);
+ return -EEXIST;
+ }
+
irq_data->hwirq = hwirq;
irq_data->domain = domain;
if (domain->ops->map) {
--
2.4.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 2/3] genirq/irqdomain: Re-check mapping after associate in irq_create_mapping()
2019-09-27 15:00 [PATCH v2 0/3] Fix irq_domain vs. irq user race Sverdlin, Alexander (Nokia - DE/Ulm)
2019-09-27 15:00 ` [PATCH v2 1/3] genirq/irqdomain: Check for existing mapping in irq_domain_associate() Sverdlin, Alexander (Nokia - DE/Ulm)
@ 2019-09-27 15:00 ` Sverdlin, Alexander (Nokia - DE/Ulm)
2019-09-27 15:00 ` [PATCH v2 3/3] genirq/irqdomain: Detect type race in irq_create_fwspec_mapping() Sverdlin, Alexander (Nokia - DE/Ulm)
2 siblings, 0 replies; 4+ messages in thread
From: Sverdlin, Alexander (Nokia - DE/Ulm) @ 2019-09-27 15:00 UTC (permalink / raw)
To: Marc Zyngier, Thomas Gleixner, linux-kernel, Grant Likely
Cc: Sverdlin, Alexander (Nokia - DE/Ulm),
Mark Brown, Jon Hunter, Glavinic-Pecotic, Matija (EXT - DE/Ulm),
Adamski, Krzysztof (Nokia - PL/Wroclaw),
Bachorski, Tomasz (Nokia - PL/Wroclaw),
Kosnikowski, Wojciech (Nokia - PL/Wroclaw),
stable
From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
If two irq_create_mapping() calls perform a mapping of the same hwirq on
two CPU cores in parallel they both will get 0 from irq_find_mapping(),
both will allocate unique virq using irq_domain_alloc_descs() and both
will finally irq_domain_associate() it. Giving different virq numbers
to their callers.
In practice the first caller is usually an interrupt controller driver and
the seconds is some device requesting the interrupt providede by the above
interrupt controller.
In this case either the interrupt controller driver configures virq which
is not the one being "associated" with hwirq, or the "slave" device
requests the virq which is never being triggered.
Reported-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
Reported-by: Tomasz Bachorski <tomasz.bachorski@nokia.com>
Reported-by: Wojciech Kosnikowski <wojciech.kosnikowski@nokia.com>
Cc: stable@vger.kernel.org
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
kernel/irq/irqdomain.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index ccbb048..ad62c08 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -661,6 +661,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
{
struct device_node *of_node;
int virq;
+ int ret;
pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
@@ -675,13 +676,6 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
of_node = irq_domain_get_of_node(domain);
- /* Check if mapping already exists */
- virq = irq_find_mapping(domain, hwirq);
- if (virq) {
- pr_debug("-> existing mapping on virq %d\n", virq);
- return virq;
- }
-
/* Allocate a virtual interrupt number */
virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node), NULL);
if (virq <= 0) {
@@ -689,8 +683,11 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
return 0;
}
- if (irq_domain_associate(domain, virq, hwirq)) {
+ ret = irq_domain_associate(domain, virq, hwirq);
+ if (ret) {
irq_free_desc(virq);
+ if (ret == -EEXIST)
+ return irq_find_mapping(domain, hwirq);
return 0;
}
--
2.4.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 3/3] genirq/irqdomain: Detect type race in irq_create_fwspec_mapping()
2019-09-27 15:00 [PATCH v2 0/3] Fix irq_domain vs. irq user race Sverdlin, Alexander (Nokia - DE/Ulm)
2019-09-27 15:00 ` [PATCH v2 1/3] genirq/irqdomain: Check for existing mapping in irq_domain_associate() Sverdlin, Alexander (Nokia - DE/Ulm)
2019-09-27 15:00 ` [PATCH v2 2/3] genirq/irqdomain: Re-check mapping after associate in irq_create_mapping() Sverdlin, Alexander (Nokia - DE/Ulm)
@ 2019-09-27 15:00 ` Sverdlin, Alexander (Nokia - DE/Ulm)
2 siblings, 0 replies; 4+ messages in thread
From: Sverdlin, Alexander (Nokia - DE/Ulm) @ 2019-09-27 15:00 UTC (permalink / raw)
To: Marc Zyngier, Thomas Gleixner, linux-kernel, Grant Likely
Cc: Sverdlin, Alexander (Nokia - DE/Ulm),
Mark Brown, Jon Hunter, Glavinic-Pecotic, Matija (EXT - DE/Ulm),
Adamski, Krzysztof (Nokia - PL/Wroclaw),
stable
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 ad62c08..4ff4073 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);
^ permalink raw reply related [flat|nested] 4+ messages in thread