linux-kernel - Re: [patch 5/5] irqchip: armanda: Sanitize set_irq

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: <5319ED86.2050707@free-electrons.com>
Date:	Fri, 07 Mar 2014 17:02:14 +0100
From:	Gregory CLEMENT <gregory.clement@...e-electrons.com>
To:	Jason Cooper <jason@...edaemon.net>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [patch 5/5] irqchip: armanda: Sanitize set_irq_affinity()

On 06/03/2014 20:05, Jason Cooper wrote:
> Thomas,
> 
> nit: s/armanda/armada/ in the patch subject.
> 
> Gregory,
> 
> Mind providing an Ack on this?

Well sorry but with this patch the kernel doesn't
work anymore.

I am investigating to find if some part could be kept.

> 
> thx,
> 
> Jason.
> 
> On Tue, Mar 04, 2014 at 08:43:41PM -0000, Thomas Gleixner wrote:
>> The set_irq_affinity() function has two issues:
>>
>> 1) It has no protection against selecting an offline cpu from the
>>    given mask.
>>
>> 2) It pointlessly restricts the affinity masks to have a single cpu
>>    set. This collides with the irq migration code of arm.
>>
>>    irq affinity is set to core 3
>>    core 3 goes offline
>>
>>    migration code sets mask to cpu_online_mask and calls the
>>    irq_set_affinity() callback of the irq_chip which fails due to bit
>>    0,1,2 set.
>>
>> So instead of doing silly for_each_cpu() loops just pick any bit of
>> the mask which intersects with the online mask.
>>
>> The read back of the routing register is pointless as well. We can
>> simply write the new mask as the bits in this register reflect one
>> core.
>>
>> Get rid of fiddling with the default_irq_affinity as well.
>>
>> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Gregory CLEMENT <gregory.clement@...e-electrons.com>
>> Cc: Jason Cooper <jason@...edaemon.net>
>>
>> ---
>>  drivers/irqchip/irq-armada-370-xp.c |   38 ++++--------------------------------
>>  1 file changed, 5 insertions(+), 33 deletions(-)
>>
>> Index: tip/drivers/irqchip/irq-armada-370-xp.c
>> ===================================================================
>> --- tip.orig/drivers/irqchip/irq-armada-370-xp.c
>> +++ tip/drivers/irqchip/irq-armada-370-xp.c
>> @@ -244,35 +244,16 @@ static DEFINE_RAW_SPINLOCK(irq_controlle
>>  static int armada_xp_set_affinity(struct irq_data *d,
>>  				  const struct cpumask *mask_val, bool force)
>>  {
>> -	unsigned long reg;
>> -	unsigned long new_mask = 0;
>> -	unsigned long online_mask = 0;
>> -	unsigned long count = 0;
>>  	irq_hw_number_t hwirq = irqd_to_hwirq(d);
>> +	unsigned long mask;
>>  	int cpu;
>>  
>> -	for_each_cpu(cpu, mask_val) {
>> -		new_mask |= 1 << cpu_logical_map(cpu);
>> -		count++;
>> -	}
>> -
>> -	/*
>> -	 * Forbid mutlicore interrupt affinity
>> -	 * This is required since the MPIC HW doesn't limit
>> -	 * several CPUs from acknowledging the same interrupt.
>> -	 */
>> -	if (count > 1)
>> -		return -EINVAL;
>> -
>> -	for_each_cpu(cpu, cpu_online_mask)
>> -		online_mask |= 1 << cpu_logical_map(cpu);
>> +	/* Select a single core from the affinity mask which is online */
>> +	cpu = cpumask_any_and(mask_val, cpu_online_mask);
>> +	mask = 1UL << cpu_logical_map(cpu);
>>  
>>  	raw_spin_lock(&irq_controller_lock);
>> -
>> -	reg = readl(main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
>> -	reg = (reg & (~online_mask)) | new_mask;
>> -	writel(reg, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
>> -
>> +	writel(mask, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
>>  	raw_spin_unlock(&irq_controller_lock);
>>  
>>  	return 0;
>> @@ -494,15 +475,6 @@ static int __init armada_370_xp_mpic_of_
>>  
>>  #ifdef CONFIG_SMP
>>  	armada_xp_mpic_smp_cpu_init();
>> -
>> -	/*
>> -	 * Set the default affinity from all CPUs to the boot cpu.
>> -	 * This is required since the MPIC doesn't limit several CPUs
>> -	 * from acknowledging the same interrupt.
>> -	 */
>> -	cpumask_clear(irq_default_affinity);
>> -	cpumask_set_cpu(smp_processor_id(), irq_default_affinity);
>> -
>>  #endif
>>  
>>  	armada_370_xp_msi_init(node, main_int_res.start);
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/