linux-kernel - Re: [PATCH 02/11] irqdomain: Warn if we fail to set the IRQ type

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]

Date:	Tue, 10 May 2016 19:00:44 +0100
From:	Jon Hunter <jonathanh@...dia.com>
To:	Marc Zyngier <marc.zyngier@....com>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	<linux-kernel@...r.kernel.org>, <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH 02/11] irqdomain: Warn if we fail to set the IRQ type


On 10/05/16 18:25, Marc Zyngier wrote:
> On 10/05/16 16:14, Jon Hunter wrote:
>> When setting the IRQ type we don't check the return value to see if it
>> is set correctly. Due to this, failures to set the IRQ type have gone
>> unnoticed and because these failures were not catastrophic have not had
>> an impact on the system.
>>
>> Ideally, we should return an error if we fail to set the type, however,
>> this could cause non-catastrophic failures to prevent devices from
>> working. Therefore, for now add a warning so that any bad interrupt
>> configurations can be corrected.
>>
>> Signed-off-by: Jon Hunter <jonathanh@...dia.com>
>> ---
>>  kernel/irq/irqdomain.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index 8798b6c9e945..09060072cc28 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -610,7 +610,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>  	/* Set type if specified and different than the current one */
>>  	if (type != IRQ_TYPE_NONE &&
>>  	    type != irq_get_trigger_type(virq))
>> -		irq_set_irq_type(virq, type);
>> +		if (irq_set_irq_type(virq, type))
>> +			pr_warn("failed to set type for irq %d\n", virq);
> 
> This warning triggers on all per-cpu interrupts, because
> irq_set_irq_type() uses IRQ_GET_DESC_CHECK_GLOBAL and not
> IRQ_GET_DESC_CHECK_PERCPU. Which sort of makes sense because the trigger
> is per-cpu and not global. We'd need some similar check in
> enable_percpu_irq, but at that stage, we've already lost the context
> coming from the firmware.
> 
> Which only proves one thing: per-cpu interrupts have never been
> configured on the allocation path, and we've been living pretty
> dangerously so far. They do work (at least on ARM) because of the
> following reasons:
> 
> 1) the triggers are already configured (firmware, read-only...)
> 2) the handle_percpu_devid_irq handler doesn't distinguish between flows
> 
> It is probably broken on all other architectures, which kind of sucks.
> At this point, I'm really tempted to drop this patch and to aim towards
> something similar to what you had in patches 5 and 6 in your previous
> series. I'll have a think tonight.

OK. I will hold off on posting the other patches for the minute.

Cheers
Jon