LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] e1000: check the return of pci_get_drvdata() in e1000_remove()
@ 2018-05-23  0:17 Bo Chen
  2018-05-23  1:47 ` Stephen Hemminger
  0 siblings, 1 reply; 3+ messages in thread
From: Bo Chen @ 2018-05-23  0:17 UTC (permalink / raw)
  To: jeffrey.t.kirsher; +Cc: davem, intel-wired-lan, netdev, linux-kernel, Bo Chen

This check on pci_get_drvdata() prevents potential invalid pointer dereferences,
and is a common practice in *_remove() functions from other drivers, such as
'intel/e100.c', 'amd/pcnet32.c', 'realtek/8139too.c', and 'broadcom/tg3.c'.

Signed-off-by: Bo Chen <chenbo@pdx.edu>
---
 drivers/net/ethernet/intel/e1000/e1000_main.c | 41 ++++++++++---------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index d5eb19b86a0a..7f63360dd221 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1283,32 +1283,35 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 static void e1000_remove(struct pci_dev *pdev)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
-	struct e1000_adapter *adapter = netdev_priv(netdev);
-	struct e1000_hw *hw = &adapter->hw;
-	bool disable_dev;
 
-	e1000_down_and_stop(adapter);
-	e1000_release_manageability(adapter);
+	if (netdev) {
+		struct e1000_adapter *adapter = netdev_priv(netdev);
+		struct e1000_hw *hw = &adapter->hw;
+		bool disable_dev;
 
-	unregister_netdev(netdev);
+		e1000_down_and_stop(adapter);
+		e1000_release_manageability(adapter);
 
-	e1000_phy_hw_reset(hw);
+		unregister_netdev(netdev);
 
-	kfree(adapter->tx_ring);
-	kfree(adapter->rx_ring);
+		e1000_phy_hw_reset(hw);
 
-	if (hw->mac_type == e1000_ce4100)
-		iounmap(hw->ce4100_gbe_mdio_base_virt);
-	iounmap(hw->hw_addr);
-	if (hw->flash_address)
-		iounmap(hw->flash_address);
-	pci_release_selected_regions(pdev, adapter->bars);
+		kfree(adapter->tx_ring);
+		kfree(adapter->rx_ring);
 
-	disable_dev = !test_and_set_bit(__E1000_DISABLED, &adapter->flags);
-	free_netdev(netdev);
+		if (hw->mac_type == e1000_ce4100)
+			iounmap(hw->ce4100_gbe_mdio_base_virt);
+		iounmap(hw->hw_addr);
+		if (hw->flash_address)
+			iounmap(hw->flash_address);
+		pci_release_selected_regions(pdev, adapter->bars);
 
-	if (disable_dev)
-		pci_disable_device(pdev);
+		disable_dev = !test_and_set_bit(__E1000_DISABLED, &adapter->flags);
+		free_netdev(netdev);
+
+		if (disable_dev)
+			pci_disable_device(pdev);
+	}
 }
 
 /**
-- 
2.17.0

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] e1000: check the return of pci_get_drvdata() in e1000_remove()
  2018-05-23  0:17 [PATCH] e1000: check the return of pci_get_drvdata() in e1000_remove() Bo Chen
@ 2018-05-23  1:47 ` Stephen Hemminger
  2018-05-23  3:31   ` Bo Chen
  0 siblings, 1 reply; 3+ messages in thread
From: Stephen Hemminger @ 2018-05-23  1:47 UTC (permalink / raw)
  To: Bo Chen; +Cc: jeffrey.t.kirsher, davem, intel-wired-lan, netdev, linux-kernel

On Tue, 22 May 2018 17:17:43 -0700
Bo Chen <chenbo@pdx.edu> wrote:

> This check on pci_get_drvdata() prevents potential invalid pointer dereferences,
> and is a common practice in *_remove() functions from other drivers, such as
> 'intel/e100.c', 'amd/pcnet32.c', 'realtek/8139too.c', and 'broadcom/tg3.c'.
> 
> Signed-off-by: Bo Chen <chenbo@pdx.edu>

Why check for something that can never be true.
You are creating dead code paths that can never be exercised.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] e1000: check the return of pci_get_drvdata() in e1000_remove()
  2018-05-23  1:47 ` Stephen Hemminger
@ 2018-05-23  3:31   ` Bo Chen
  0 siblings, 0 replies; 3+ messages in thread
From: Bo Chen @ 2018-05-23  3:31 UTC (permalink / raw)
  Cc: netdev, linux-kernel

Re-send to mailing lists as the previous email was rejected because of
not using plain text.

-----

Hi Stephen,

Thanks for the quick reply and your comments. I am new to network
drivers, and certainly trust your assessment.

I was assuming 'pdev->dev.p' can be NULL with a 'kzalloc()' failure in
'pci_set_drvdata()' invoked by 'e1000_probe()', in which case the
"pci_get_drvdata()" will return a NULL pointer. But I did not trace
back to confirm whether 'pdev->dev.p' has to be valid before
'e1000_probe()' is invoked. Maybe this is what I am missing? Please
excuse my newbie questions and mistakes.

Here is some context about why I started this patch. I am building a
tool to perform "grey-box" fault injection on linux-kernel-module
binaries. In my tool, there is a set of kernel API functions that
faults can be generated, for example making the return of
'__kmalloc()' be NULL. My tool fired an alarm (a kernel panic), when a
fault on ''dev_get_drvdata()" is injected to 'e1000.ko' (and other
drivers were fine, such as e100.ko and pcnet32.ko). It seems that I
was wrong to assume 'dev_get_drvdata()' can return NULL and inject
faults from it. Do you have any suggestions about how I can avoid such
wrong assumptions?

Thanks again for your time and attention. I hope this patch is not
wasting too much effort from the netdev community. Any other comments
or suggestions will be appreciated.

Best Regards,
Bo Chen

On Tue, May 22, 2018 at 6:47 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 22 May 2018 17:17:43 -0700
> Bo Chen <chenbo@pdx.edu> wrote:
>
> > This check on pci_get_drvdata() prevents potential invalid pointer dereferences,
> > and is a common practice in *_remove() functions from other drivers, such as
> > 'intel/e100.c', 'amd/pcnet32.c', 'realtek/8139too.c', and 'broadcom/tg3.c'.
> >
> > Signed-off-by: Bo Chen <chenbo@pdx.edu>
>
> Why check for something that can never be true.
> You are creating dead code paths that can never be exercised.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-05-23  3:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-23  0:17 [PATCH] e1000: check the return of pci_get_drvdata() in e1000_remove() Bo Chen
2018-05-23  1:47 ` Stephen Hemminger
2018-05-23  3:31   ` Bo Chen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).