xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL

xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL

[Igor Mammedov]

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

Re: xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL

[Jan Beulich]

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

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 

Re: xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL

[Igor Mammedov]

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

Thanks 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

Re: xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL

[Igor Mammedov]

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

Re: xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL

[Jan Beulich]

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

Makes sense - could you say so in the description when re-submitting?

Jan