linux-kernel - Re: Re: [PATCH] IRQ: Add a check for uninitialized irqs.

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] [day] [month] [year] [list]

Message-ID: <20120915025502.GB58426@Archie.local>
Date:	Sat, 15 Sep 2012 08:25:02 +0530
From:	Raghavendra D Prabhu <raghu.prabhu13@...il.com>
To:	Paul Gortmaker <paul.gortmaker@...driver.com>
Cc:	jeremy@...p.org, mingo@...hat.com, x86@...nel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	liu chuansheng <chuansheng.liu@...el.com>,
	Fernando Luis Vazquez Cao <fernando@....ntt.co.jp>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re:  Re: [PATCH] IRQ: Add a check for uninitialized irqs.

Hi,


* On Mon, Sep 10, 2012 at 09:41:49AM -0400, Paul Gortmaker <paul.gortmaker@...driver.com> wrote:
>On 12-09-09 03:25 PM, raghu.prabhu13@...il.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@...hang.net>
>>
>> Errors like "do_IRQ: 0.84 No Irq handler for vector (irq -1)" have been reported
>> earlier filling up the console/logs. So this adds a condition to check for
>
>Reported by who, and where, and on what hardware, and under what
>circumstances?

I have seen it reported in many places like 
https://bugzilla.redhat.com/show_bug.cgi?id=225399 and 
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/480997

>
>You are adding another if statement to the hot path of do_IRQ just
>to mask some random unknown symptom. instead of trying to understand
>(and then solve) what the real breakage is.  Which is not the right
>approach.

I just realized that handler_irq checks for < NR_IRQS where the 
underflow will fail and will return. So this patch is not required.

However,  I think we need to fix the unsigned / int mixup by 
assigning vector_irq[..] to NR_IRQS+1  as guard value and 
changing unsigned to signed as in patch.
>
>Paul.
>--
>
>> uninitialized irqs so that it exits early and doesn't proceed further. Also, irq
>> is made a signed integer, since if it is not mapped to a vector, it will be
>> assigned -1 which will be UINT_MAX otherwise.
>>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@...hang.net>
>> ---
>>  arch/x86/kernel/irq.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
>> index d44f782..ddba63a 100644
>> --- a/arch/x86/kernel/irq.c
>> +++ b/arch/x86/kernel/irq.c
>> @@ -184,21 +184,24 @@ unsigned int __irq_entry do_IRQ(struct pt_regs *regs)
>>
>>  	/* high bit used in ret_from_ code  */
>>  	unsigned vector = ~regs->orig_ax;
>> -	unsigned irq;
>> +	int irq;
>>
>>  	irq_enter();
>>  	exit_idle();
>>
>> -	irq = __this_cpu_read(vector_irq[vector]);
>> +	if (unlikely((irq = __this_cpu_read(vector_irq[vector])) == -1)) {
>> +		pr_emerg_ratelimited("IRQ handler not setup for vector %u", vector);
>> +		goto err_exit;
>> +	}
>>
>>  	if (!handle_irq(irq, regs)) {
>>  		ack_APIC_irq();
>>
>> -		if (printk_ratelimit())
>> -			pr_emerg("%s: %d.%d No irq handler for vector (irq %d)\n",
>> -				__func__, smp_processor_id(), vector, irq);
>> +		pr_emerg_ratelimited("%s: %d.%d No irq handler for vector (irq %u)\n",
>> +			__func__, smp_processor_id(), vector, irq);
>>  	}
>>
>> +err_exit:
>>  	irq_exit();
>>
>>  	set_irq_regs(old_regs);
>>
>




Regards,
-- 
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

Content of type "application/pgp-signature" skipped