On Thu, 28 Feb 2013 15:02:07 +0800 xunleer <xunleer.li@xxxxxxxxxx> wrote: > This patch fix an oops issue caused by NULL pointer. The call stack > may be like this: > [<ffffffffa04dae0b>] ixgbevf_down+0x10b/0x300 [ixgbevf] > [<ffffffffa04dc6bd>] ixgbevf_open+0x27d/0x2b0 [ixgbevf] > [<ffffffff813866b7>] __dev_open+0xa7/0x100 > [<ffffffff81386745>] dev_open+0x35/0x60 > > When opening the net device, we are trying to request msix irqs, > if failed, the error procedure released the msix entries and disabled > the MSI-X interrupts in pci level. However, if the request is not > succeeded, the ixgbevf_down is called, it synchronizes the irq by > referring to the msix_entries, it may cause oops as to NULL pointer > references. > > The msix_entries may be allocated in probe procedure and be released > in remove routine may be the best way. So we removed this error > processing action. Then we may try to reopen the device, all we must > reload the driver. Li, we appreciate the patch you sent and after review have concluded that it appears to be the right thing to do. However we would like to modify the description of the patch to improve clarity. How about the following description? ---------- When the ixgbevf driver is opened the request to allocate MSIX irq vectors may fail. In that case the driver will call ixgbevf_down() which will call ixgbevf_irq_disable() to clear the HW interrupt registers and calls synchronize_irq() using the msix_entries pointer in the adapter structure. However, when the function to request the MSIX irq vectors failed it had already freed the msix_entries which causes an OOPs from using the NULL pointer in synchronize_irq(). The calls to pci_disable_msix() and to free the msix_entries memory should not occur if device open fails. Instead they should be called during device driver removal to balance with the call to pci_enable_msix() and the call to allocate msix_entries memory during the device probe and driver load. ---------- If that description works for you then please resubmit the patch and we will add it to our queue for validation and testing. Or if you would like to change the description in some other way let me know and we'll work on it. Thanks, - Greg Rose Networking Division Intel Corp. > > Signed-off-by: Li Xun <xunleer.li@xxxxxxxxxx> > --- > drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c > b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c index > c3db6cd..f176208 100644 --- > a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +++ > b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c @@ -944,9 +944,6 > @@ free_queue_irqs: free_irq(adapter->msix_entries[vector].vector, > adapter->q_vector[vector]); > } > - pci_disable_msix(adapter->pdev); > - kfree(adapter->msix_entries); > - adapter->msix_entries = NULL; > return err; > } > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html