linux-kernel - Re: [PATCH] irqdomain: Avoid activating interrupts more than once

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dade7d1e-b1dd-31c1-2798-f85568802aa7@arm.com>
Date:   Wed, 25 Jan 2017 14:58:09 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     linux-kernel@...r.kernel.org
Cc:     Andre Przywara <andre.przywara@....com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] irqdomain: Avoid activating interrupts more than once

Hi Thomas,

Any comment on that one?

Thanks,

	M.

On 17/01/17 16:00, Marc Zyngier wrote:
> Since commit f3b0946d629c ("genirq/msi: Make sure PCI MSIs are
> activated early"), we can end-up activating a PCI/MSI twice (once
> at allocation time, and once at startup time).
> 
> This is normally of no consequences, except that there is some
> HW out there that may misbehave if activate is used more than once
> (the GICv3 ITS, for example, uses the activate callback
> to issue the MAPVI command, and the architecture spec says that
> "If there is an existing mapping for the EventID-DeviceID
> combination, behavior is UNPREDICTABLE").
> 
> While this could be worked around in each individual driver, it may
> make more sense to tackle the issue at the core level. In order to
> avoid getting in that situation, let's have a per-interrupt flag
> to remember if we have already activated that interrupt or not.
> 
> Fixes: f3b0946d629c ("genirq/msi: Make sure PCI MSIs are activated early")
> Reported-by: Andre Przywara <andre.przywara@....com>
> Tested-by: Andre Przywara <andre.przywara@....com>
> Signed-off-by: Marc Zyngier <marc.zyngier@....com>
> ---
>  include/linux/irq.h    | 17 +++++++++++++++++
>  kernel/irq/irqdomain.c | 44 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 47 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index e798755..39e3254 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -184,6 +184,7 @@ struct irq_data {
>   *
>   * IRQD_TRIGGER_MASK		- Mask for the trigger type bits
>   * IRQD_SETAFFINITY_PENDING	- Affinity setting is pending
> + * IRQD_ACTIVATED		- Interrupt has already been activated
>   * IRQD_NO_BALANCING		- Balancing disabled for this IRQ
>   * IRQD_PER_CPU			- Interrupt is per cpu
>   * IRQD_AFFINITY_SET		- Interrupt affinity was set
> @@ -202,6 +203,7 @@ struct irq_data {
>  enum {
>  	IRQD_TRIGGER_MASK		= 0xf,
>  	IRQD_SETAFFINITY_PENDING	= (1 <<  8),
> +	IRQD_ACTIVATED			= (1 <<  9),
>  	IRQD_NO_BALANCING		= (1 << 10),
>  	IRQD_PER_CPU			= (1 << 11),
>  	IRQD_AFFINITY_SET		= (1 << 12),
> @@ -312,6 +314,21 @@ static inline bool irqd_affinity_is_managed(struct irq_data *d)
>  	return __irqd_to_state(d) & IRQD_AFFINITY_MANAGED;
>  }
>  
> +static inline bool irqd_is_activated(struct irq_data *d)
> +{
> +	return __irqd_to_state(d) & IRQD_ACTIVATED;
> +}
> +
> +static inline void irqd_set_activated(struct irq_data *d)
> +{
> +	__irqd_to_state(d) |= IRQD_ACTIVATED;
> +}
> +
> +static inline void irqd_clr_activated(struct irq_data *d)
> +{
> +	__irqd_to_state(d) &= ~IRQD_ACTIVATED;
> +}
> +
>  #undef __irqd_to_state
>  
>  static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 8c0a0ae..b59e676 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1346,6 +1346,30 @@ void irq_domain_free_irqs_parent(struct irq_domain *domain,
>  }
>  EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
>  
> +static void __irq_domain_activate_irq(struct irq_data *irq_data)
> +{
> +	if (irq_data && irq_data->domain) {
> +		struct irq_domain *domain = irq_data->domain;
> +
> +		if (irq_data->parent_data)
> +			__irq_domain_activate_irq(irq_data->parent_data);
> +		if (domain->ops->activate)
> +			domain->ops->activate(domain, irq_data);
> +	}
> +}
> +
> +static void __irq_domain_deactivate_irq(struct irq_data *irq_data)
> +{
> +	if (irq_data && irq_data->domain) {
> +		struct irq_domain *domain = irq_data->domain;
> +
> +		if (domain->ops->deactivate)
> +			domain->ops->deactivate(domain, irq_data);
> +		if (irq_data->parent_data)
> +			__irq_domain_deactivate_irq(irq_data->parent_data);
> +	}
> +}
> +
>  /**
>   * irq_domain_activate_irq - Call domain_ops->activate recursively to activate
>   *			     interrupt
> @@ -1356,13 +1380,9 @@ EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
>   */
>  void irq_domain_activate_irq(struct irq_data *irq_data)
>  {
> -	if (irq_data && irq_data->domain) {
> -		struct irq_domain *domain = irq_data->domain;
> -
> -		if (irq_data->parent_data)
> -			irq_domain_activate_irq(irq_data->parent_data);
> -		if (domain->ops->activate)
> -			domain->ops->activate(domain, irq_data);
> +	if (!irqd_is_activated(irq_data)) {
> +		__irq_domain_activate_irq(irq_data);
> +		irqd_set_activated(irq_data);
>  	}
>  }
>  
> @@ -1376,13 +1396,9 @@ void irq_domain_activate_irq(struct irq_data *irq_data)
>   */
>  void irq_domain_deactivate_irq(struct irq_data *irq_data)
>  {
> -	if (irq_data && irq_data->domain) {
> -		struct irq_domain *domain = irq_data->domain;
> -
> -		if (domain->ops->deactivate)
> -			domain->ops->deactivate(domain, irq_data);
> -		if (irq_data->parent_data)
> -			irq_domain_deactivate_irq(irq_data->parent_data);
> +	if (irqd_is_activated(irq_data)) {
> +		__irq_domain_deactivate_irq(irq_data);
> +		irqd_clr_activated(irq_data);
>  	}
>  }
>  
> 


-- 
Jazz is not dead. It just smells funny...