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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> 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