Re: [PATCH] moxa: drop free_irq of devm_request_irq allocated irq

On 09/26/2013 10:36 AM, Jingoo Han wrote:

On Thursday, September 26, 2013 11:13 AM, Wei Yongjun wrote:

quoted

On 09/26/2013 08:47 AM, Jingoo Han wrote:

quoted

On Wednesday, September 25, 2013 4:33 PM, Wei Yongjun wrote:

quoted

From: Wei Yongjun <redacted>

irq allocated with devm_request_irq should not be freed using
free_irq, because doing so causes a dangling pointer, and a
subsequent double free.

Signed-off-by: Wei Yongjun <redacted>
---
 drivers/net/ethernet/moxa/moxart_ether.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/moxa/moxart_ether.c b/drivers/net/ethernet/moxa/moxart_ether.c
index 83c2091..9a7fcb5 100644
--- a/drivers/net/ethernet/moxa/moxart_ether.c
+++ b/drivers/net/ethernet/moxa/moxart_ether.c

@@ -531,7 +531,6 @@ static int moxart_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);

 	unregister_netdev(ndev);
-	free_irq(ndev->irq, ndev);
 	moxart_mac_free_memory(ndev);
 	free_netdev(ndev);

CC'ed Sachin Kamat,

In this case, the free_irq() will be called, after calling
free_netdev(). 'ndev' is freed by free_netdev(). Then, 'ndev->irq'
is used by free_irq(). Is it right?

In my humble opinion, it seems to make the problem.

devm_request_irq() has recorded the irq and dev_id, so free_irq() by devm_*
will not touch 'ndev' which has been freed by free_netdev().
So, if we not need to call free_irq() before free_netdev(), there will be
no problem.

However, 'dev_id' is a pointer, not a value.
It seems to make the problem that references the invalid pointer.

free_irq using dev_id as a raw data, so you mean the irq handle?

----
 free_irq(...)
 {
   ...
     if (action->dev_id == dev_id)
   ...
 }
---

Regards,
Yongjun Wei