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

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.

[PATCH V2] e1000: Add netif_napi_del in the normal path and error path to match netif_napi_add

[Jia-Ju Bai]

Th driver lacks netif_napi_del in the normal path and error path to match the call of netif_napi_add in e1000_probe.

This patch fixes this problem, and it has been tested in runtime.

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

---

  drivers/net/ethernet/intel/e1000/e1000_main.c    |    6 +-

  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c

index 24f3986..f6def7b 100644

--- a/drivers/net/ethernet/intel/e1000/e1000_main.c

+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c

@@ -1004,7 +1004,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

  	/* make ready for any if (hw->...) below */

  	err = e1000_init_hw_struct(adapter, hw);

  	if (err)

-		goto err_sw_init;

+		goto err_dma;

  	/* there is a workaround being applied below that limits

  	* 64-bit DMA addresses to 64-bit hardware.  There are some

@@ -1239,8 +1239,9 @@ err_eeprom:

  		iounmap(hw->flash_address);

  	kfree(adapter->tx_ring);

  	kfree(adapter->rx_ring);

-err_dma:

  err_sw_init:

+	netif_napi_del(&adapter->napi);

+err_dma:

  err_mdio_ioremap:

  	iounmap(hw->ce4100_gbe_mdio_base_virt);

  	iounmap(hw->hw_addr);

@@ -1271,6 +1272,7 @@ static void e1000_remove(struct pci_dev *pdev)

  	e1000_down_and_stop(adapter);

  	e1000_release_manageability(adapter);

+	netif_napi_del(&adapter->napi);

  	unregister_netdev(netdev);

  	e1000_phy_hw_reset(hw);



------------------------------------------------------------------------------
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 V2] e1000: Add netif_napi_del in the normal path and error path to match netif_napi_add

[Sergei Shtylyov]

Hello.

On 12/21/2014 3:52 AM, Jia-Ju Bai wrote:

> Th driver lacks netif_napi_del in the normal path and error path to match the
> call of netif_napi_add in e1000_probe.

    Please wrap your change log lines at 80 columns (or less).

> This patch fixes this problem, and it has been tested in runtime.

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

> ---

>   drivers/net/ethernet/intel/e1000/e1000_main.c    |    6 +-

>   1 file changed, 4 insertions(+), 2 deletions(-)

> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c
> b/drivers/net/ethernet/intel/e1000/e1000_main.c
>
> index 24f3986..f6def7b 100644
>
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
>
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
>
> @@ -1004,7 +1004,7 @@ static int e1000_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>
>       /* make ready for any if (hw->...) below */
>
>       err = e1000_init_hw_struct(adapter, hw);
>
>       if (err)
>
> -        goto err_sw_init;
>
> +        goto err_dma;
>
>       /* there is a workaround being applied below that limits
>
>       * 64-bit DMA addresses to 64-bit hardware.  There are some

    Somehow your mailer inserted empty lines after each actual line of the 
patch. :-(

WBR, Sergei

[PATCH V2 1/2] e100 in linux-3.18.0: Fix null pointer deference in e100_probe

[Jia-Ju Bai]

[-- Attachment #1.1: Type: text/plain, Size: 1280 bytes --]

The driver lacks the check of nic->cbs_pool after pci_pool_create in e100_probe. So when this function is failed, the null pointer dereference occurs when pci_pool_alloc uses nic->cbs_pool in e100_alloc_cbs.
This patch fix this problem, and it has been tested in runtime.

Signed-off-by: Jia-Ju Bai<baijiaju1990@163.com>
---
  drivers/net/ethernet/intel/e100.c     |   6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index 781065e..ba1813f 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:



[-- Attachment #2: Type: text/plain, Size: 441 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]

_______________________________________________
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 V2 1/2] e100 in linux-3.18.0: Fix null pointer deference in e100_probe

[Sergei Shtylyov]

On 12/21/2014 4:19 AM, Jia-Ju Bai wrote:

    Please don't send HTML to this mailing list -- your mail may be ignored by 
the list server.

> The driver lacks the check of nic->cbs_pool after pci_pool_create in e100_probe. So when this function is failed, the null pointer dereference occurs when pci_pool_alloc uses nic->cbs_pool in e100_alloc_cbs.

    Same comment as for the previous patch about wrapping at 80 columns.

> This patch fix this problem, and it has been tested in runtime.

> Signed-off-by: Jia-Ju Bai<baijiaju1990@163.com>
> ---
>   drivers/net/ethernet/intel/e100.c     |   6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
> index 781065e..ba1813f 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;
> +   }

    Looks like tabs got converted to spaces by your mailer, thus the patch 
can't be applied. Consider using 'git send-email' instead.

[...]

WBR, Sergei

[PATCH V2 2/2] e100 in linux-3.18.0: Add netif_napi_del in the normal path and error path to match netif_napi_add

[Jia-Ju Bai]

[-- Attachment #1.1: Type: text/plain, Size: 901 bytes --]

The driver lacks netif_napi_del in the normal path and error path to 
match the call of netif_napi_add in e100_probe.
This patch fixes this problem, and it has been tested in runtime.

Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
drivers/net/ethernet/intel/e100.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/e100.c 
b/drivers/net/ethernet/intel/e100.c
index 781065e..21c4d0f 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -2985,6 +2985,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 +2996,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: Type: text/plain, Size: 441 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]

_______________________________________________
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