Message ID | CAPgLHd8YRP7kL_jNHi3J9jFn86f=Fv-AWWUY_aRGZQOgVTNPgg@mail.gmail.com |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
On Wednesday, September 25, 2013 4:33 PM, Wei Yongjun wrote: > > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > 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 <yongjun_wei@trendmicro.com.cn> > --- > 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. Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/26/2013 08:47 AM, Jingoo Han wrote: > On Wednesday, September 25, 2013 4:33 PM, Wei Yongjun wrote: >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >> >> 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 <yongjun_wei@trendmicro.com.cn> >> --- >> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, September 26, 2013 11:13 AM, Wei Yongjun wrote: > On 09/26/2013 08:47 AM, Jingoo Han wrote: > > On Wednesday, September 25, 2013 4:33 PM, Wei Yongjun wrote: > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > >> > >> 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 <yongjun_wei@trendmicro.com.cn> > >> --- > >> 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. Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-09-26 at 10:12 +0800, Wei Yongjun wrote: > On 09/26/2013 08:47 AM, Jingoo Han wrote: > > On Wednesday, September 25, 2013 4:33 PM, Wei Yongjun wrote: > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > >> > >> 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 <yongjun_wei@trendmicro.com.cn> > >> --- > >> 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. What if this is a shared IRQ? Then if free_irq() is not called here: - The IRQ handler might still be called after free_netdev() - The memory containing ndev could also be reallocated to another device sharing the IRQ, so that it it uses the same dev_id for its IRQ handler Maybe you can be sure that this device will never share an IRQ. But it still doesn't look like good practice to rely on this. Perhaps there should be devm functions for netdevs too, so it wouldn't be necessary to free either IRQ or netdev explicitly. Ben.
From: Wei Yongjun <weiyj.lk@gmail.com> Date: Wed, 25 Sep 2013 15:33:29 +0800 > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > 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 <yongjun_wei@trendmicro.com.cn> I think this is a dangerous change, if the IRQ fires after the point of the existing free_irq() call it will try to dereference the net device struct which will be free by the time the devm release code runes. I'm not applying this patch, sorry. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);