Re: [Xen-devel] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL

[Igor Mammedov <imammedo@xxxxxxxxxx>]

On 09/13/2011 01:45 PM, Jan Beulich wrote:
> > > > On 13.09.11 at 11:08, Igor Mammedov<imammedo@xxxxxxxxxx>  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<imammedo@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxx
> > http://lists.xensource.com/xen-devel
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
--
Thanks,
 Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel