* [PATCH 2/2] ipv4 igmp: use del_timer_sync instead of del_timer in ip_mc_down
@ 2013-09-05 6:43 Salam Noureddine
2013-09-05 15:43 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Salam Noureddine @ 2013-09-05 6:43 UTC (permalink / raw)
To: Salam Noureddine, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev
Delete timers using del_timer_sync in ip_mc_down. Otherwise, it is
possible for the timer to be the last to release its reference to the
in_device and since __in_dev_put doesn't destroy the in_device we
would end up leaking a reference to the net_device and see messages
like the following,
unregister_netdevice: waiting for eth0 to become free. Usage count = 1
Tested on linux-3.4.43.
Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
---
net/ipv4/igmp.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index cd71190..f8c3bbb 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -1420,10 +1420,10 @@ void ip_mc_down(struct in_device *in_dev)
#ifdef CONFIG_IP_MULTICAST
in_dev->mr_ifc_count = 0;
- if (del_timer(&in_dev->mr_ifc_timer))
+ if (del_timer_sync(&in_dev->mr_ifc_timer))
__in_dev_put(in_dev);
in_dev->mr_gq_running = 0;
- if (del_timer(&in_dev->mr_gq_timer))
+ if (del_timer_sync(&in_dev->mr_gq_timer))
__in_dev_put(in_dev);
igmpv3_clear_delrec(in_dev);
#endif
--
1.7.4.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ipv4 igmp: use del_timer_sync instead of del_timer in ip_mc_down
2013-09-05 6:43 [PATCH 2/2] ipv4 igmp: use del_timer_sync instead of del_timer in ip_mc_down Salam Noureddine
@ 2013-09-05 15:43 ` Stephen Hemminger
2013-09-05 17:22 ` Salam Noureddine
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2013-09-05 15:43 UTC (permalink / raw)
To: Salam Noureddine
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev
On Wed, 4 Sep 2013 23:43:24 -0700
Salam Noureddine <noureddine@aristanetworks.com> wrote:
> Delete timers using del_timer_sync in ip_mc_down. Otherwise, it is
> possible for the timer to be the last to release its reference to the
> in_device and since __in_dev_put doesn't destroy the in_device we
> would end up leaking a reference to the net_device and see messages
> like the following,
>
> unregister_netdevice: waiting for eth0 to become free. Usage count = 1
>
> Tested on linux-3.4.43.
>
> Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
Why not just call in_dev_put instead which just proper cleanup.
It is less risky of deadlock than del_timer_sync.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ipv4 igmp: use del_timer_sync instead of del_timer in ip_mc_down
2013-09-05 15:43 ` Stephen Hemminger
@ 2013-09-05 17:22 ` Salam Noureddine
2013-09-27 23:04 ` Salam Noureddine
0 siblings, 1 reply; 6+ messages in thread
From: Salam Noureddine @ 2013-09-05 17:22 UTC (permalink / raw)
To: Stephen Hemminger
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev
On Thu, Sep 5, 2013 at 8:43 AM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Wed, 4 Sep 2013 23:43:24 -0700
> Salam Noureddine <noureddine@aristanetworks.com> wrote:
>
>> Delete timers using del_timer_sync in ip_mc_down. Otherwise, it is
>> possible for the timer to be the last to release its reference to the
>> in_device and since __in_dev_put doesn't destroy the in_device we
>> would end up leaking a reference to the net_device and see messages
>> like the following,
>>
>> unregister_netdevice: waiting for eth0 to become free. Usage count = 1
>>
>> Tested on linux-3.4.43.
>>
>> Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
>
> Why not just call in_dev_put instead which just proper cleanup.
> It is less risky of deadlock than del_timer_sync.
I was wondering if there was a reason behind using __in_dev_put since
the multicast code
is the only user of that function. I can test using in_dev_put
instead. Should __in_dev_put
be removed altogether in that case?
Thanks,
Salam
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ipv4 igmp: use del_timer_sync instead of del_timer in ip_mc_down
2013-09-05 17:22 ` Salam Noureddine
@ 2013-09-27 23:04 ` Salam Noureddine
2013-09-27 23:15 ` Alexey Kuznetsov
0 siblings, 1 reply; 6+ messages in thread
From: Salam Noureddine @ 2013-09-27 23:04 UTC (permalink / raw)
To: Stephen Hemminger
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev
I tried using in_dev_put instead of __in_dev_put and it seems to work
fine in my testing on linux-3.4. I don't quite understand the reason
for having __in_dev_put decrement the refcnt without destroying the
in_device in case it reaches 0. If the timer handler assumes it cannot
be the last one to hold a reference then that would mean it doesn't
need the reference in the first place.
If this solution is preferable to using del_timer_sync I would go
ahead and submit a new patch.
Thanks,
Salam
On Thu, Sep 5, 2013 at 10:22 AM, Salam Noureddine
<noureddine@aristanetworks.com> wrote:
> On Thu, Sep 5, 2013 at 8:43 AM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>> On Wed, 4 Sep 2013 23:43:24 -0700
>> Salam Noureddine <noureddine@aristanetworks.com> wrote:
>>
>>> Delete timers using del_timer_sync in ip_mc_down. Otherwise, it is
>>> possible for the timer to be the last to release its reference to the
>>> in_device and since __in_dev_put doesn't destroy the in_device we
>>> would end up leaking a reference to the net_device and see messages
>>> like the following,
>>>
>>> unregister_netdevice: waiting for eth0 to become free. Usage count = 1
>>>
>>> Tested on linux-3.4.43.
>>>
>>> Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
>>
>> Why not just call in_dev_put instead which just proper cleanup.
>> It is less risky of deadlock than del_timer_sync.
>
> I was wondering if there was a reason behind using __in_dev_put since
> the multicast code
> is the only user of that function. I can test using in_dev_put
> instead. Should __in_dev_put
> be removed altogether in that case?
>
> Thanks,
>
> Salam
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ipv4 igmp: use del_timer_sync instead of del_timer in ip_mc_down
2013-09-27 23:04 ` Salam Noureddine
@ 2013-09-27 23:15 ` Alexey Kuznetsov
2013-09-27 23:28 ` Salam Noureddine
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Kuznetsov @ 2013-09-27 23:15 UTC (permalink / raw)
To: Salam Noureddine
Cc: Stephen Hemminger, David S. Miller, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev
On Fri, Sep 27, 2013 at 04:04:00PM -0700, Salam Noureddine wrote:
> I don't quite understand the reason
> for having __in_dev_put decrement the refcnt without destroying the
> in_device in case it reaches 0. If the timer handler assumes it cannot
> be the last one to hold a reference then that would mean it doesn't
> need the reference in the first place.
I would like to explain this, because this can result in a big mistake.
Timer takes reference, when it _can_ be the last sometimes.
When we do del_timer() and know for sure that someone holds refcnt, we can just
decrese it. In this cae it is obvious: we sit in context whcih deals with in_dev,
so that reference from timer acannot be the last: caller of the function holds refcnt.
But in another places timer _expires_ and that last reference is dropped by in_dev_put().
Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ipv4 igmp: use del_timer_sync instead of del_timer in ip_mc_down
2013-09-27 23:15 ` Alexey Kuznetsov
@ 2013-09-27 23:28 ` Salam Noureddine
0 siblings, 0 replies; 6+ messages in thread
From: Salam Noureddine @ 2013-09-27 23:28 UTC (permalink / raw)
To: Alexey Kuznetsov
Cc: Stephen Hemminger, David S. Miller, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev
Thanks for the explanation. Currenlty, __in_dev_put is both used in
timer handler function and in ip_mc_down where del_timer is invoked. I
guess we need to change the timer handler so it uses in_dev_put to do
proper cleanup in cases where it runs past the call to ip_mc_down and
has the last reference.
On Fri, Sep 27, 2013 at 4:15 PM, Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> wrote:
> On Fri, Sep 27, 2013 at 04:04:00PM -0700, Salam Noureddine wrote:
>> I don't quite understand the reason
>> for having __in_dev_put decrement the refcnt without destroying the
>> in_device in case it reaches 0. If the timer handler assumes it cannot
>> be the last one to hold a reference then that would mean it doesn't
>> need the reference in the first place.
>
> I would like to explain this, because this can result in a big mistake.
>
> Timer takes reference, when it _can_ be the last sometimes.
>
> When we do del_timer() and know for sure that someone holds refcnt, we can just
> decrese it. In this cae it is obvious: we sit in context whcih deals with in_dev,
> so that reference from timer acannot be the last: caller of the function holds refcnt.
>
> But in another places timer _expires_ and that last reference is dropped by in_dev_put().
>
> Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-28 0:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-05 6:43 [PATCH 2/2] ipv4 igmp: use del_timer_sync instead of del_timer in ip_mc_down Salam Noureddine
2013-09-05 15:43 ` Stephen Hemminger
2013-09-05 17:22 ` Salam Noureddine
2013-09-27 23:04 ` Salam Noureddine
2013-09-27 23:15 ` Alexey Kuznetsov
2013-09-27 23:28 ` Salam Noureddine
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).