[PATCH 1/3] genirq/irqdomain: Check for existing mapping in irq_domain_associate()

[PATCH 1/3] genirq/irqdomain: Check for existing mapping in irq_domain_associate()

[Sverdlin, Alexander (Nokia - DE/Ulm)]

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.

Cc: stable@vger.kernel.org
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 kernel/irq/irqdomain.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 3078d0e..7bc07b6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -532,6 +532,7 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 			 irq_hw_number_t hwirq)
 {
 	struct irq_data *irq_data = irq_get_irq_data(virq);
+	unsigned int eirq;
 	int ret;
 
 	if (WARN(hwirq >= domain->hwirq_max,
@@ -543,6 +544,16 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 		return -EINVAL;
 
 	mutex_lock(&irq_domain_mutex);
+
+	/* Check if mapping already exists */
+	eirq = irq_find_mapping(domain, hwirq);
+	if (eirq) {
+		mutex_unlock(&irq_domain_mutex);
+		pr_debug("%s: conflicting mapping for hwirq 0x%x\n",
+			 domain->name, (int)hwirq);
+		return -EBUSY;
+	}
+
 	irq_data->hwirq = hwirq;
 	irq_data->domain = domain;
 	if (domain->ops->map) {
-- 
2.4.6

[PATCH 2/3] genirq/irqdomain: Re-check mapping after associate in irq_create_mapping()

[Sverdlin, Alexander (Nokia - DE/Ulm)]

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.

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 7bc07b6..176f2cc 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -675,13 +675,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) {
@@ -691,7 +684,11 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
 
 	if (irq_domain_associate(domain, virq, hwirq)) {
 		irq_free_desc(virq);
-		return 0;
+
+		virq = irq_find_mapping(domain, hwirq);
+		if (virq)
+			pr_debug("-> existing mapping on virq %d\n", virq);
+		return virq;
 	}
 
 	pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
-- 
2.4.6

[PATCH 3/3] genirq/irqdomain: Detect type race in irq_create_fwspec_mapping()

[Sverdlin, Alexander (Nokia - DE/Ulm)]

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);
-- 
2.4.6

Re: [PATCH 1/3] genirq/irqdomain: Check for existing mapping in irq_domain_associate()

[Marc Zyngier]

On 12/09/2019 10:44, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> 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.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>  kernel/irq/irqdomain.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 3078d0e..7bc07b6 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -532,6 +532,7 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
>  			 irq_hw_number_t hwirq)
>  {
>  	struct irq_data *irq_data = irq_get_irq_data(virq);
> +	unsigned int eirq;
>  	int ret;
>  
>  	if (WARN(hwirq >= domain->hwirq_max,
> @@ -543,6 +544,16 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
>  		return -EINVAL;
>  
>  	mutex_lock(&irq_domain_mutex);
> +
> +	/* Check if mapping already exists */
> +	eirq = irq_find_mapping(domain, hwirq);
> +	if (eirq) {

nit: Just have

	if (irq_find_mapping(...)) {

and get rid the extra variable, given that it's not used for anything.

> +		mutex_unlock(&irq_domain_mutex);
> +		pr_debug("%s: conflicting mapping for hwirq 0x%x\n",
> +			 domain->name, (int)hwirq);
> +		return -EBUSY;

I'm overall OK with this, although I'd rather we return -EEXIST rather
than -EBUSY.

> +	}
> +
>  	irq_data->hwirq = hwirq;
>  	irq_data->domain = domain;
>  	if (domain->ops->map) {
> 

Thanks,

	M.
-- 
Jazz is not dead, it just smells funny...

Re: [PATCH 2/3] genirq/irqdomain: Re-check mapping after associate in irq_create_mapping()

[Marc Zyngier]

On 12/09/2019 10:44, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> 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.

I disagree with this "In practice". An irqchip controller should *very
rarely* call irq_create_mapping on its own. It usually indicates some
level of brokenness, unless the mapped interrupt is exposed by the
irqchip itself (the GIC maintenance interrupt, for example).

> 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.

Why should the interrupt controller configure that interrupt? On any
sane platform, the mapping should be created by the user of the
interrupt, and not by the provider.

This doesn't mean we shouldn't fix the irqdomain races, but I tend to
disagree with the analysis here.

> 
> 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 7bc07b6..176f2cc 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -675,13 +675,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) {
> @@ -691,7 +684,11 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  
>  	if (irq_domain_associate(domain, virq, hwirq)) {
>  		irq_free_desc(virq);
> -		return 0;
> +
> +		virq = irq_find_mapping(domain, hwirq);
> +		if (virq)
> +			pr_debug("-> existing mapping on virq %d\n", virq);

I'd rather you limit this second irq_find_mapping() to cases where we're
sure we've found a duplicate:

	ret = irq_domain_associate(domain, virq, hwirq);
	if (ret) {
		irq_free_desc(virq);
		if (ret == -EEXIST)
			return irq_find_mapping(domain, hwirq);

		return 0;
	}

> +		return virq;
>  	}
>  
>  	pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
> 

Thanks,

	M.
-- 
Jazz is not dead, it just smells funny...

Re: [PATCH 2/3] genirq/irqdomain: Re-check mapping after associate in irq_create_mapping()

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hi Marc,

On 20/09/2019 17:52, Marc Zyngier wrote:
>> 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.
> I disagree with this "In practice". An irqchip controller should *very
> rarely* call irq_create_mapping on its own. It usually indicates some
> level of brokenness, unless the mapped interrupt is exposed by the
> irqchip itself (the GIC maintenance interrupt, for example).

I also didn't understand the reason the irqchip in question calls
irq_create_mapping(), but as 9 upstream irqchips do this as well I was
not really interested in the reasons for this.

>> 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.
> Why should the interrupt controller configure that interrupt? On any
> sane platform, the mapping should be created by the user of the
> interrupt, and not by the provider.
> 
> This doesn't mean we shouldn't fix the irqdomain races, but I tend to
> disagree with the analysis here.

That's in fact what happens in our case and may happen with 9 upstream
irqchips as well. Same race would however happen with any IRQ client
driver calling of_irq_get(), if they share same HW IRQ line.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 3/3] genirq/irqdomain: Detect type race in irq_create_fwspec_mapping()

[Marc Zyngier]

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.
-- 
Jazz is not dead, it just smells funny...

Re: [PATCH 3/3] genirq/irqdomain: Detect type race in irq_create_fwspec_mapping()

[Sverdlin, Alexander (Nokia - DE/Ulm)]

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.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 2/3] genirq/irqdomain: Re-check mapping after associate in irq_create_mapping()

[Alexander Sverdlin]

Hello Marc,

On 20/09/2019 17:52, Marc Zyngier wrote:
> On 12/09/2019 10:44, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
>> 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.
> I disagree with this "In practice". An irqchip controller should *very
> rarely* call irq_create_mapping on its own. It usually indicates some
> level of brokenness, unless the mapped interrupt is exposed by the
> irqchip itself (the GIC maintenance interrupt, for example).
> 
>> 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.
> Why should the interrupt controller configure that interrupt? On any
> sane platform, the mapping should be created by the user of the
> interrupt, and not by the provider.
> 
> This doesn't mean we shouldn't fix the irqdomain races, but I tend to
> disagree with the analysis here.

would you have time to review v2 of this series?

-- 
Best regards,
Alexander Sverdlin.