linux-kernel - Re: [Xen-devel] Re: [patch] xenfb: fix xenfb suspend/resume race

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: <1294303673.3831.3562.camel@zakaz.uk.xensource.com>
Date:	Thu, 6 Jan 2011 08:47:53 +0000
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	Joe Jin <joe.jin@...cle.com>
CC:	"jeremy@...p.org" <jeremy@...p.org>,
	"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
	"gurudas.pai@...cle.com" <gurudas.pai@...cle.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	"guru.anbalagane@...cle.com" <guru.anbalagane@...cle.com>,
	"greg.marsden@...cle.com" <greg.marsden@...cle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [Xen-devel] Re: [patch] xenfb: fix xenfb suspend/resume race

On Thu, 2011-01-06 at 08:02 +0000, Ian Campbell wrote:
> 
> > Check if irq is valid will fix this issue.
> 
> No, it papers over the issue, the code should never have been allowed
> to get this far if the connection to the backend is not yet fully
> resumed (i.e. when irq == -1). 

On the other hand changing a kernel crash into a WARN_ON has merit in
its own right, I just think it is a stretch to say the xenfb issue is
fixed by it! You'd need to also make the warning go away to be able to
say that.

> @@ -175,6 +175,10 @@ static struct irq_info *info_for_irq(unsigned irq)
>  
>  static unsigned int evtchn_from_irq(unsigned irq)
>  {
> +     if (unlikely(irq < 0 || irq >= nr_irqs)) {
> +             WARN_ON(1, "[%s]: Invalid irq(%d)!\n", __func__, irq);

Did this compile without warnings? That isn't the correct syntax for a
WARN_ON since it takes only a condition and not the additional message.

I think this can simplified as:
	if (WARN(irq < 0 || irq >= nr_irqs, "invalid irq %d!\n", irq))
             return 0;

I think WARN includes the file name and line number as well as a stack
trace so including the function name is unnecessary.

I'm not sure the condition used is totally valid for a modern kernel
with sparse IRQs (it probably needs to do a irq_to_desc lookup) but I
think we can defer that until events.c moves to the more dynamic scheme
this implies (I should resurrect those patches now that thiongs have
settled down after the dom0 merge).

Ian.

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