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

Jia-Ju Bai

author

2014-12-20 14:32 UTC #0 lore raw

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 <[email protected]>

(quoted diff) +0 -0

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

drivers/net/ethernet/intel/e100.c +0 -0

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

Quoted 5 lines · Jia-Ju Bai

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.

Quoted 14 lines

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

Quoted 2 lines

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.

drivers/net/ethernet/intel/e100.c static int e100_probe(struct pci_dev *pdev, · line 2973

Quoted 11 lines

drivers/net/ethernet/intel/e100.c +1 -0

index 781065e..2631d3f 100644
@@ -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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Sergei Shtylyov

2014-12-20 15:07 UTC #1 lore raw

Quoted 1 line · Jia-Ju Bai

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.

Quoted 1 line

I have revised my patch:

You still need to revise it some more.

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 1 line

Signed-off-by Jia-Ju Bai <[email protected]>

... and after sign-off too.

drivers/net/ethernet/intel/e100.c

Quoted 3 lines

drivers/net/ethernet/intel/e100.c +0 -0
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

David Miller

maintainer

2014-12-20 19:30 UTC #2 lore raw

In reply to Jia-Ju Bai

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.

Jia-Ju Bai

author

2014-12-21 00:52 UTC #3 lore raw

PATCH v2 1/1 e1000: Add netif_napi_del in the normal path and error path to match netif_napi_add

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<[email protected]>

---

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

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

drivers/net/ethernet/intel/e1000/e1000_main.c +0 -0

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Sergei Shtylyov

2014-12-21 13:58 UTC #4 lore raw

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

Hello.

Quoted 2 lines · 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.

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

Quoted 25 lines

This patch fixes this problem, and it has been tested in runtime.
Signed-off-by Jia-Ju Bai<[email protected]>
---
  drivers/net/ethernet/intel/e1000/e1000_main.c    |    6 +-
  1 file changed, 4 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/e1000/e1000_main.c +0 -0
index 24f3986..f6def7b 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
drivers/net/ethernet/intel/e1000/e1000_main.c +0 -0
@@ -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);

drivers/net/ethernet/intel/e1000/e1000_main.c static int e1000_probe(struct pci_dev *pdev, const

Quoted 10 lines

drivers/net/ethernet/intel/e1000/e1000_main.c +1 -1

 
      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

Jia-Ju Bai

author

2014-12-21 01:19 UTC #5 lore raw

PATCH v2 1/2 e100 in linux-3.18.0: Fix null pointer deference in e100_probe

In reply to David Miller

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<[email protected]>

---
 drivers/net/ethernet/intel/e100.c     |   6 ++++++
 1 file changed, 6 insertions(+)

drivers/net/ethernet/intel/e100.c +6 -0

index 781065e..ba1813f 100644
@@ -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:

------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Sergei Shtylyov

2014-12-21 14:02 UTC #6 lore raw

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

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.

Quoted 1 line

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.

Quoted 12 lines

This patch fix this problem, and it has been tested in runtime.
Signed-off-by Jia-Ju Bai<[email protected]>
---
  drivers/net/ethernet/intel/e100.c     |   6 ++++++
  1 file changed, 6 insertions(+)
drivers/net/ethernet/intel/e100.c +0 -0
index 781065e..ba1813f 100644

drivers/net/ethernet/intel/e100.c static int e100_probe(struct pci_dev *pdev, const struct pci_device_id *ent) · line 2975

Quoted 7 lines

drivers/net/ethernet/intel/e100.c +4 -0

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

Jia-Ju Bai

author

2014-12-21 01:51 UTC #7 lore raw

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

In reply to David Miller

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 <[email protected]>

---
drivers/net/ethernet/intel/e100.c | 2 ++

1 file changed, 2 insertions(+)

drivers/net/ethernet/intel/e100.c +0 -0

index 781065e..21c4d0f 100644
@@ -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);

------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired