* [PATCH] moxa: drop free_irq of devm_request_irq allocated irq
@ 2013-09-25 7:33 Wei Yongjun
2013-09-26 0:47 ` Jingoo Han
2013-09-30 18:55 ` David Miller
0 siblings, 2 replies; 7+ messages in thread
From: Wei Yongjun @ 2013-09-25 7:33 UTC (permalink / raw)
To: grant.likely, rob.herring, davem, jg1.han, jonas.jensen
Cc: yongjun_wei, netdev
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);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] moxa: drop free_irq of devm_request_irq allocated irq
2013-09-25 7:33 [PATCH] moxa: drop free_irq of devm_request_irq allocated irq Wei Yongjun
@ 2013-09-26 0:47 ` Jingoo Han
2013-09-26 2:12 ` Wei Yongjun
2013-09-30 18:55 ` David Miller
1 sibling, 1 reply; 7+ messages in thread
From: Jingoo Han @ 2013-09-26 0:47 UTC (permalink / raw)
To: 'Wei Yongjun', 'Jonas Jensen'
Cc: davem, grant.likely, rob.herring, yongjun_wei, netdev,
'Jingoo Han', 'Sachin Kamat'
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] moxa: drop free_irq of devm_request_irq allocated irq
2013-09-26 0:47 ` Jingoo Han
@ 2013-09-26 2:12 ` Wei Yongjun
2013-09-26 2:36 ` Jingoo Han
2013-09-26 14:02 ` Ben Hutchings
0 siblings, 2 replies; 7+ messages in thread
From: Wei Yongjun @ 2013-09-26 2:12 UTC (permalink / raw)
To: jg1.han
Cc: jonas.jensen, davem, grant.likely, rob.herring, yongjun_wei,
netdev, sachin.kamat
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] moxa: drop free_irq of devm_request_irq allocated irq
2013-09-26 2:12 ` Wei Yongjun
@ 2013-09-26 2:36 ` Jingoo Han
2013-09-26 14:02 ` Ben Hutchings
1 sibling, 0 replies; 7+ messages in thread
From: Jingoo Han @ 2013-09-26 2:36 UTC (permalink / raw)
To: 'Wei Yongjun'
Cc: 'Jonas Jensen', 'David S. Miller',
'Grant Likely', rob.herring, yongjun_wei, netdev,
'Sachin Kamat', 'Jingoo Han'
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] moxa: drop free_irq of devm_request_irq allocated irq
@ 2013-09-26 3:23 Wei Yongjun
0 siblings, 0 replies; 7+ messages in thread
From: Wei Yongjun @ 2013-09-26 3:23 UTC (permalink / raw)
To: jg1.han
Cc: jonas.jensen, davem, grant.likely, rob.herring, yongjun_wei,
netdev, sachin.kamat
On 09/26/2013 10:36 AM, Jingoo Han wrote:
> 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.
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] moxa: drop free_irq of devm_request_irq allocated irq
2013-09-26 2:12 ` Wei Yongjun
2013-09-26 2:36 ` Jingoo Han
@ 2013-09-26 14:02 ` Ben Hutchings
1 sibling, 0 replies; 7+ messages in thread
From: Ben Hutchings @ 2013-09-26 14:02 UTC (permalink / raw)
To: Wei Yongjun
Cc: jg1.han, jonas.jensen, davem, grant.likely, rob.herring,
yongjun_wei, netdev, sachin.kamat
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.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] moxa: drop free_irq of devm_request_irq allocated irq
2013-09-25 7:33 [PATCH] moxa: drop free_irq of devm_request_irq allocated irq Wei Yongjun
2013-09-26 0:47 ` Jingoo Han
@ 2013-09-30 18:55 ` David Miller
1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2013-09-30 18:55 UTC (permalink / raw)
To: weiyj.lk
Cc: grant.likely, rob.herring, jg1.han, jonas.jensen, yongjun_wei,
netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-09-30 18:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-25 7:33 [PATCH] moxa: drop free_irq of devm_request_irq allocated irq Wei Yongjun
2013-09-26 0:47 ` Jingoo Han
2013-09-26 2:12 ` Wei Yongjun
2013-09-26 2:36 ` Jingoo Han
2013-09-26 14:02 ` Ben Hutchings
2013-09-30 18:55 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2013-09-26 3:23 Wei Yongjun
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).