| Message ID | 1378363404-37749-1-git-send-email-noureddine@aristanetworks.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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, 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 -- 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
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 -- 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 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 -- 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
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 -- 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/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
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(-)