[PATCH] x86: check if desc->action is NULL when unbinding guest pirq

[PATCH] x86: check if desc->action is NULL when unbinding guest pirq

[Cui, Dexuan]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

Before igb PF driver is unloaded, dom0 doesn't unload igbvf driver
automatically. When igb drver is unloaded, it invokes the
PHYSDEVOP_manage_pci_remove hypercall to remove the VFs and xen frees the msi
irqs by pci_cleanup_msi() -> ... -> dynamic_irq_cleanup() and sets the
desc->action to NULL.
igbvf driver knows the VF is disappearing via a hook ndo_stop() in dev_close()
and tries to unbind the pirq and xen would crash as the desc->action
is NULL now.
The patch adds the checking for this.

Thanks,
-- Dexuan
 

[-- Attachment #2: check_desc_action.patch --]
[-- Type: application/octet-stream, Size: 1667 bytes --]

x86: check if desc->action is NULL when unbinding guest pirq

Before igb PF driver is unloaded, dom0 doesn't unload igbvf driver
automatically. When igb drver is unloaded, it invokes the
PHYSDEVOP_manage_pci_remove hypercall to remove the VFs and xen frees the msi
irqs by pci_cleanup_msi() -> ... -> dynamic_irq_cleanup() and sets the
desc->action to NULL.
igbvf driver knows the VF is disappearing via a hook ndo_stop() in dev_close()
and tries to unbind the pirq and xen would crash as the desc->action
is NULL now.
The patch adds the checking for this.

Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>

diff -r c06732ac2392 xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c	Thu Jan 21 15:13:00 2010 +0000
+++ b/xen/arch/x86/irq.c	Fri Jan 22 15:34:54 2010 +0800
@@ -1229,6 +1229,13 @@ static irq_guest_action_t *__pirq_guest_
 
     BUG_ON(!(desc->status & IRQ_GUEST));
 
+    if ( unlikely((desc->status | IRQ_DISABLED) && (desc->action == NULL)) )
+    {
+        dprintk(XENLOG_G_WARNING, "dom%d: pirq %d: desc->action is NULL!\n",
+            d->domain_id, pirq);
+        return NULL;
+    }
+
     action = (irq_guest_action_t *)desc->action;
     irq = desc - irq_desc;
 
@@ -1353,6 +1360,13 @@ static int pirq_guest_force_unbind(struc
         goto out;
 
     action = (irq_guest_action_t *)desc->action;
+    if ( unlikely((desc->status | IRQ_DISABLED) && (desc->action == NULL)) )
+    {
+        dprintk(XENLOG_G_WARNING, "dom%d: pirq %d: desc->action is NULL!\n",
+            d->domain_id, irq);
+        goto out;
+    }
+
     for ( i = 0; (i < action->nr_guests) && (action->guest[i] != d); i++ )
         continue;
     if ( i == action->nr_guests )

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] x86: check if desc->action is NULL when unbinding guest pirq

[Keir Fraser]

On 22/01/2010 08:01, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:

> Before igb PF driver is unloaded, dom0 doesn't unload igbvf driver
> automatically. When igb drver is unloaded, it invokes the
> PHYSDEVOP_manage_pci_remove hypercall to remove the VFs and xen frees the msi
> irqs by pci_cleanup_msi() -> ... -> dynamic_irq_cleanup() and sets the
> desc->action to NULL.
> igbvf driver knows the VF is disappearing via a hook ndo_stop() in dev_close()
> and tries to unbind the pirq and xen would crash as the desc->action
> is NULL now.
> The patch adds the checking for this.

Although I checked this in as c/s 20844, I now wonder what the
'desc->status|IRQ_DISABLED' is included for? (e.g., see below extract:)

+ if (unlikely((desc->status | IRQ_DISABLED) && (desc->action == NULL)))

Looks pointless: should this just be 'if(desc->action==NULL)'?

 -- Keir

RE: [PATCH] x86: check if desc->action is NULL when unbinding guest pirq

[Cui, Dexuan]

Keir Fraser wrote:
> On 22/01/2010 08:01, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:
> 
>> Before igb PF driver is unloaded, dom0 doesn't unload igbvf driver
>> automatically. When igb drver is unloaded, it invokes the
>> PHYSDEVOP_manage_pci_remove hypercall to remove the VFs and xen
>> frees the msi irqs by pci_cleanup_msi() -> ... ->
>> dynamic_irq_cleanup() and sets the desc->action to NULL. igbvf
>> driver knows the VF is disappearing via a hook ndo_stop() in
>> dev_close() and tries to unbind the pirq and xen would crash as the
>> desc->action  
>> is NULL now.
>> The patch adds the checking for this.
> 
> Although I checked this in as c/s 20844, I now wonder what the
> 'desc->status|IRQ_DISABLED' is included for? (e.g., see below
> extract:) 
> 
> + if (unlikely((desc->status | IRQ_DISABLED) && (desc->action ==
> NULL))) 
> 
> Looks pointless: should this just be 'if(desc->action==NULL)'?
(Sorry for the late reply as I was out for several days.)
Yes, I think so. Please improve it. :-)

Thanks,
-- Dexuan