[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

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

[Jia-Ju Bai]

I am inexperienced in submitting patches, sorry. I have revised my patch:

1.Check whether pci_pool_create is failed in e100_probe to avoid null
dereference in pci_pool_alloc(in e100_alloc_cbs).
2.Add netif_napi_del to match the call of netif_napi_add.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
diff --git a/drivers/net/ethernet/intel/e100.c
b/drivers/net/ethernet/intel/e100.c
index 781065e..a58ab2e 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -2969,6 +2969,10 @@ 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 +2980,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 +2991,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 +3002,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);


Is it okay?



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




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

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

[Sergei Shtylyov]

On 12/20/2014 5:32 PM, Jia-Ju Bai wrote:

> I am inexperienced in submitting patches, sorry.

    I see. It looks like you're failing to understand my English, too. :-(
    Please put such remarks under the --- line which should be placed after 
sign-off area.

> I have revised my patch:

    You still need to revise it some more.

> 1.Check whether pci_pool_create is failed in e100_probe to avoid null
> dereference in pci_pool_alloc(in e100_alloc_cbs).

    Please fix this issue by one (first) patch.

> 2.Add netif_napi_del to match the call of netif_napi_add.

    And fix this one by another (second) patch.
    Also, you need to insert empty line before sign-off.

> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>

    ... and after sign-off too.

> diff --git a/drivers/net/ethernet/intel/e100.c
> b/drivers/net/ethernet/intel/e100.c
> index 781065e..a58ab2e 100644
[...]

   And finally, please re-post the patches in a new thread, not in reply to 
this (or other) thread.

WBR, Sergei

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

[David Miller]

Your patch submissions are not usable by us.

Instead of immediately sending your patch 10 seconds after you
receive feedback, take your time and make sure you do everything
calmly and cleanly and as tidy as possible.

There is absolutely no rush with these changes.