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

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

[Jia-Ju Bai]

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

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.

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,
 		   "addr 0x%llx, irq %d, MAC addr %pM\n",
 		   (unsigned long long)pci_resource_start(pdev, use_io ? 1 :
0),
@@ -2976,6 +2981,8 @@ static int e100_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
 
 	return 0;
 
+err_out_pool:
+	unregister_netdev(netdev);
 err_out_free:
 	e100_free(nic);
 err_out_iounmap:
@@ -2985,6 +2992,7 @@ err_out_free_res:
 err_out_disable_pdev:
 	pci_disable_device(pdev);
 err_out_free_dev:
+	netif_napi_del(&nic->napi);
 	free_netdev(netdev);
 	return err;
 }
@@ -2995,6 +3003,7 @@ static void e100_remove(struct pci_dev *pdev)
 
 	if (netdev) {
 		struct nic *nic = netdev_priv(netdev);
+		netif_napi_del(&nic->napi);
 		unregister_netdev(netdev);
 		e100_free(nic);
 		pci_iounmap(pdev, nic->csr);

[-- Attachment #2: patch_e100 --]
[-- Type: application/octet-stream, Size: 1265 bytes --]

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,
 		   "addr 0x%llx, irq %d, MAC addr %pM\n",
 		   (unsigned long long)pci_resource_start(pdev, use_io ? 1 : 0),
@@ -2976,6 +2981,8 @@ static int e100_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	return 0;
 
+err_out_pool:
+	unregister_netdev(netdev);
 err_out_free:
 	e100_free(nic);
 err_out_iounmap:
@@ -2985,6 +2992,7 @@ err_out_free_res:
 err_out_disable_pdev:
 	pci_disable_device(pdev);
 err_out_free_dev:
+	netif_napi_del(&nic->napi);
 	free_netdev(netdev);
 	return err;
 }
@@ -2995,6 +3003,7 @@ static void e100_remove(struct pci_dev *pdev)
 
 	if (netdev) {
 		struct nic *nic = netdev_priv(netdev);
+		netif_napi_del(&nic->napi);
 		unregister_netdev(netdev);
 		e100_free(nic);
 		pci_iounmap(pdev, nic->csr);

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

[Jeff Kirsher]

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

On Sat, 2014-12-20 at 15:40 +0800, Jia-Ju Bai wrote:
> 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?

> 
> 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;
	}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jeff Kirsher]

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

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" <jeffrey.t.kirsher@intel.com> wrote:
> >On Sat, 2014-12-20 at 15:40 +0800, Jia-Ju Bai wrote:
> >> 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?
> >
> >> 
> >> 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;
> >	}
> 
> 



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Sergei Shtylyov]

On 12/20/2014 10:40 AM, Jia-Ju Bai wrote:

> 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.

    Fixing one issue per patch is the rule of thumb.

> 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.

    Again, your sign-off is required. See Documentation/SubmittingPatches.

> 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))

    Space needed after *if*. Please run your patches thru scripts/checkpatch.pl.

[...]

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 12/20/2014 10:40 AM, Jia-Ju Bai wrote:

> 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.

> 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))

    Oh, and the inner () not needed.

WBR, Sergei