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

On Sat, 2014-12-20 at 20:40 +0800, 白家驹 wrote:

Thank for the reply!
 
For the first reply:
I let pci_pool_create fail on purpose to simulate insufficient memory,
and then run the driver in reality, but the driver crashes.
 
For the second reply:
I admit you are right, and my code style need to be improved.

Added in netdev and e1000-devel back onto the CC, since Jia-Ju Bai
removed them in his reply

At 2014-12-20 18:18:09,"Jeff Kirsher" [off-list ref] wrote:

quoted

On Sat, 2014-12-20 at 15:40 +0800, Jia-Ju Bai wrote:

quoted

I have actually tested e100 driver on the real hardware(Intel 82559
PCI
Ethernet Controller), and find some bugs:
The target file is drivers/net/ethernet/intel/e100.c, which is used to
build
e100.ko.

(1) The function pci_pool_create is called by e100_probe when
initializing
the ethernet card driver. But when pci_pool_create is failed, which
means
that it returns NULL to nic->cbs_pool, the system crash will happen.
Because
pci_pool_alloc (in e100_alloc_cbs in e100_up in e100_open) need to use
nic->cbs_pool to allocate the resource, but it is NULL. I suggest that
a
check can be added in the code to detect whether pci_pool_create
returns
NULL.
(2) In the normal process, netif_napi_add is called in e100_probe, but
netif_napi_del is not called in e100_remove. However, many other
ethernet
card drivers call them in pairs, even in the error handling paths,
such as
r8169 and igb.

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.

Did you actually experience an issue?  Or is this a theoretical issue
that was never actually seen?

quoted

diff --git a/drivers/net/ethernet/intel/e100.c

b/drivers/net/ethernet/intel/e100.c
index 781065e..2631d3f 100644

--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c

@@ -2969,6 +2969,11 @@ static int e100_probe(struct pci_dev *pdev,

const
struct pci_device_id *ent)
                           nic->params.cbs.max * sizeof(struct cb),
                           sizeof(u32),
                           0);
+       if(!(nic->cbs_pool))
+       {
+               err = -ENOMEM;
+               goto err_out_pool;
+       }
        netif_info(nic, probe, nic->netdev,

Minor nit-pick but your open bracket needs to be on the same line as the
if statement AND you need a space between the 'if' and ().  So the above
code should look like:
if (!nic->cbs_pool) {
	err = -ENOMEM;
	goto err_out_pool;
}