diff mbox

igb in linux-3.18.0: some potential bugs

Message ID 001101d01c2c$8dcc0040$a96400c0$@163.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Jia-Ju Bai Dec. 20, 2014, 8:11 a.m. UTC 
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.

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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Kirsher, Jeffrey T Dec. 20, 2014, 10:35 a.m. UTC | #1 
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.
Jia-Ju Bai Dec. 20, 2014, 12:56 p.m. UTC | #2 
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.
【来自网易邮箱的超大附件】
邮件带有附件预览链接，若您转发或回复此邮件时不希望对方预览附件，建议您手动删除链接。

signature.asc
下载: http://u.163.com/t/HnVMNHn21

预览: http://u.163.com/t/g1ju64



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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