Igor Mammedov
2011-Sep-13 09:08 UTC
[Xen-devel] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL
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
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.
Please review.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
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
Jan Beulich
2011-Sep-13 11:45 UTC
Re: [Xen-devel] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL
>>> On 13.09.11 at 11:08, Igor Mammedov <imammedo@redhat.com> 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 nextSo 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> Please review. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > 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
Igor Mammedov
2011-Sep-13 12:31 UTC
Re: [Xen-devel] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL
On 09/13/2011 01:45 PM, Jan Beulich wrote:>>>> On 13.09.11 at 11:08, Igor Mammedov<imammedo@redhat.com> 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. > > JanThanks for review. I''ll fix it and re-post it as 2 patches.> >> Please review. >> >> Signed-off-by: Igor Mammedov<imammedo@redhat.com> >> >> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Igor Mammedov
2011-Sep-13 12:36 UTC
Re: [Xen-devel] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL
On 09/13/2011 01:45 PM, Jan Beulich wrote:>>>> On 13.09.11 at 11:08, Igor Mammedov<imammedo@redhat.com> 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?then we may hit BUG_ON(!(desc->status & IRQ_GUEST)); in pirq_guest_unbind -> __pirq_guest_unbind It seams safer for me to clear bit in __pirq_guest_unbind _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-13 13:10 UTC
Re: [Xen-devel] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL
>>> On 13.09.11 at 14:36, Igor Mammedov <imammedo@redhat.com> wrote: > On 09/13/2011 01:45 PM, Jan Beulich wrote: >>>>> On 13.09.11 at 11:08, Igor Mammedov<imammedo@redhat.com> 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? > > then we may hit > > BUG_ON(!(desc->status & IRQ_GUEST)); > > in pirq_guest_unbind -> __pirq_guest_unbind > > It seams safer for me to clear bit in __pirq_guest_unbindMakes sense - could you say so in the description when re-submitting? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel