[PATCH] igb in linux-3.18.0: some potential bugs

[PATCH] igb in linux-3.18.0: some potential bugs

[Jia-Ju Bai]

I have actually tested igb driver on the real hardware(Intel 82575EB PCI-E
Gigabit Ethernet Controller), and find some potential bugs:
The target file is drivers/net/ethernet/intel/igb/igb_main.c

(1) In the normal process of igb, pci_enable_pcie_error_reporting and
pci_disable_pcie_error_reporting is called in pairs in igb_probe and
igb_remove. However, when pci_enable_pcie_error_reporting has been called
and alloc_etherdev_mqs in igb_probe is failed, "err_alloc_etherdev" segment
in igb_probe is executed immediately to exit, but
pci_disable_pcie_error_reporting is not called.
(2) The same situation happens when pci_iomap in igb_probe is failed.
(3) The same situation happens when igb_sw_init in igb_probe is failed.
(4) The same situation happens when register_netdev in igb_probe is failed.
(5) The same situation happens when igb_init_i2c in igb_probe is failed.

(6) The function kcalloc is called by igb_sw_init when initializing the
ethernet card driver, but kfree is not called when register_netdev in
igb_probe is failed, which may cause memory leak.
(7) The same situation happens when igb_init_i2c in igb_probe is failed.
(8) The same situation happens when kzalloc in igb_alloc_q_vector is failed.
(9) The same situation happens when igb_alloc_q_vector in
igb_alloc_q_vectors is failed.

(10) When igb_init_i2c in igb_probe is failed, igb_enable_sriov is called in
igb_probe_vfs, but igb_disable_sriov is not called.
(11) The same situation with [10] happens when register_netdev in igb_probe
is failed.

Meanwhile, I also write the patch to fix the bugs. I have run the patch on
the hardware, it can work normally and fix the above bugs.

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
b/drivers/net/ethernet/intel/igb/igb_main.c
index 487cd9c..cd9364a 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -179,6 +179,7 @@ static void igb_check_vf_rate_limit(struct igb_adapter
*);
 #ifdef CONFIG_PCI_IOV
 static int igb_vf_configure(struct igb_adapter *adapter, int vf);
 static int igb_pci_enable_sriov(struct pci_dev *dev, int num_vfs);
+static int igb_disable_sriov(struct pci_dev *pdev);
 #endif
 
 #ifdef CONFIG_PM
@@ -2653,17 +2654,22 @@ err_register:
 	igb_release_hw_control(adapter);
 	memset(&adapter->i2c_adap, 0, sizeof(adapter->i2c_adap));
 err_eeprom:
+#ifdef CONFIG_PCI_IOV
+	igb_disable_sriov(pdev);
+#endif
 	if (!igb_check_reset_block(hw))
 		igb_reset_phy(hw);
 
 	if (hw->flash_address)
 		iounmap(hw->flash_address);
 err_sw_init:
+	kfree(adapter->shadow_vfta);
 	igb_clear_interrupt_scheme(adapter);
 	pci_iounmap(pdev, hw->hw_addr);
 err_ioremap:
 	free_netdev(netdev);
 err_alloc_etherdev:
+	pci_disable_pcie_error_reporting(pdev);
 	pci_release_selected_regions(pdev,
 				     pci_select_bars(pdev, IORESOURCE_MEM));
 err_pci_reg:


Thanks!

Re: [PATCH] igb in linux-3.18.0: some potential bugs

[Jia-Ju Bai]

Thank for the reply!

For the first reply:
I let some functions fail on purpose to test error handling code, and then run the driver in reality as well as monitor the function calls in runtime.
The results are in my report. 

For the second reply:
I admit you are right, and my code style need to be improved.

On Sat, 2014-12-20 at 16:11 +0800, Jia-Ju Bai wrote:
> I have actually tested igb driver on the real hardware(Intel 82575EB 
> PCI-E Gigabit Ethernet Controller), and find some potential bugs:
> The target file is drivers/net/ethernet/intel/igb/igb_main.c
> 
> (1) In the normal process of igb, pci_enable_pcie_error_reporting and 
> pci_disable_pcie_error_reporting is called in pairs in igb_probe and 
> igb_remove. However, when pci_enable_pcie_error_reporting has been 
> called and alloc_etherdev_mqs in igb_probe is failed, 
> "err_alloc_etherdev"
> segment
> in igb_probe is executed immediately to exit, but 
> pci_disable_pcie_error_reporting is not called.
> (2) The same situation happens when pci_iomap in igb_probe is failed.
> (3) The same situation happens when igb_sw_init in igb_probe is 
> failed.
> (4) The same situation happens when register_netdev in igb_probe is 
> failed.
> (5) The same situation happens when igb_init_i2c in igb_probe is 
> failed.
> 
> (6) The function kcalloc is called by igb_sw_init when initializing 
> the ethernet card driver, but kfree is not called when register_netdev 
> in igb_probe is failed, which may cause memory leak.
> (7) The same situation happens when igb_init_i2c in igb_probe is 
> failed.
> (8) The same situation happens when kzalloc in igb_alloc_q_vector is 
> failed.
> (9) The same situation happens when igb_alloc_q_vector in 
> igb_alloc_q_vectors is failed.
> 
> (10) When igb_init_i2c in igb_probe is failed, igb_enable_sriov is 
> called in igb_probe_vfs, but igb_disable_sriov is not called.
> (11) The same situation with [10] happens when register_netdev in 
> igb_probe is failed.
> 
> Meanwhile, I also write the patch to fix the bugs. I have run the 
> patch on the hardware, it can work normally and fix the above bugs.

>Was this a bug you actually saw?  Or a theoretical bug based on code review?

>I do not mind adding this to my queue so that we can review and test the patch, although this will cause a fair amount of regression testing.



------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] igb in linux-3.18.0: some potential bugs

[David Miller]

From: "Jia-Ju Bai" <baijiaju1990@163.com>
Date: Sat, 20 Dec 2014 20:59:18 +0800

> Thank for the reply!

Please do not top-post.

Etiquette on these mailing lists is that you quote a minimal
amount of material from the email you are replying to to give
enough context to the reader, than you give your response
_after_ the quoted material, not before.

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired