From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Mammedov Subject: Re: xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL Date: Tue, 13 Sep 2011 14:31:59 +0200 Message-ID: <4E6F4D3F.7060904@redhat.com> References: <1315904920-12533-1-git-send-email-imammedo@redhat.com> <4E6F5E830200007800055D49@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4E6F5E830200007800055D49@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On 09/13/2011 01:45 PM, Jan Beulich wrote: >>>> On 13.09.11 at 11:08, Igor Mammedov wrote: >> On a system with Intel C600 series Patsburg SAS controller >> if following command are executed: >> >> rmmod isci >> modprobe isci >> >> the host will crash in pirq_guest_bind in attempt to dereference >> NULL action pointer. >> >> This is caused by isci driver which does not cleanup irq properly, >> removing device first and then os tries to unbind its irqs afterwards. >> >> c/s 20093 and 20844 fixed host crashes when removing isci module. >> >> However in dynamic_irq_cleanup 'action' field of irq_desc is set to >> NULL but IRQ_GUEST flag in 'status' field is not cleared. So on next > > So why don't you clear the bit there? > >> attempt to bind pirq (modprobe isci) with IRQ_GUEST flag set, branch >> if ( !(desc->status& IRQ_GUEST) ) >> is skipped and host ends up with dereferencing NULL pointer 'action'. >> >> Second hunk is a bit of code cleanup, removing duplicate code and keeps >> IRQ_GUEST flag reset at one place. > > This is not just cleanup - till now, when action == NULL, the function > would return 0, while with your patch it would return 1 (which is wrong > afaict), so you'll minimally need to move down the unbind: label by one > line. But the cleanup here would better be a separate patch anyway. > > Jan Thanks for review. I'll fix it and re-post it as 2 patches. > >> Please review. >> >> Signed-off-by: Igor Mammedov >> >> diff -r 0312575dc35e xen/arch/x86/irq.c >> --- a/xen/arch/x86/irq.c Thu Sep 08 15:13:06 2011 +0100 >> +++ b/xen/arch/x86/irq.c Tue Sep 13 09:27:12 2011 +0200 >> @@ -1472,6 +1472,7 @@ static irq_guest_action_t *__pirq_guest_ >> >> if ( unlikely(action == NULL) ) >> { >> + desc->status&= ~IRQ_GUEST; >> dprintk(XENLOG_G_WARNING, "dom%d: pirq %d: desc->action is NULL!\n", >> d->domain_id, pirq->pirq); >> return NULL; >> @@ -1598,17 +1599,14 @@ static int pirq_guest_force_unbind(struc >> >> action = (irq_guest_action_t *)desc->action; >> if ( unlikely(action == NULL) ) >> - { >> - dprintk(XENLOG_G_WARNING, "dom%d: pirq %d: desc->action is NULL!\n", >> - d->domain_id, pirq->pirq); >> - goto out; >> - } >> + goto unbind; >> >> for ( i = 0; (i< action->nr_guests)&& (action->guest[i] != d); i++ ) >> continue; >> if ( i == action->nr_guests ) >> goto out; >> >> + unbind: >> bound = 1; >> oldaction = __pirq_guest_unbind(d, pirq, desc); >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel -- Thanks, Igor