Message ID | 1378363359-37716-1-git-send-email-noureddine@aristanetworks.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, 2013-09-04 at 23:42 -0700, Salam Noureddine wrote: > Delete timers using del_timer_sync in ipv6_mc_down. Otherwise, it is > possible for the timer to be the last to release its reference to the > inet6_dev and since __in6_dev_put doesn't destroy the inet6_dev 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/ipv6/mcast.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c > index 99cd65c..5c8d49d 100644 > --- a/net/ipv6/mcast.c > +++ b/net/ipv6/mcast.c > @@ -2277,12 +2277,12 @@ void ipv6_mc_down(struct inet6_dev *idev) > > read_lock_bh(&idev->lock); > idev->mc_ifc_count = 0; > - if (del_timer(&idev->mc_ifc_timer)) > + if (del_timer_sync(&idev->mc_ifc_timer)) Are you sure this doesn't introduce a potential deadlock? Have you tested this with lockdep enabled? Ben. > __in6_dev_put(idev); > idev->mc_gq_running = 0; > - if (del_timer(&idev->mc_gq_timer)) > + if (del_timer_sync(&idev->mc_gq_timer)) > __in6_dev_put(idev); > - if (del_timer(&idev->mc_dad_timer)) > + if (del_timer_sync(&idev->mc_dad_timer)) > __in6_dev_put(idev); > > for (i = idev->mc_list; i; i=i->next)
On Thu, Sep 5, 2013 at 7:30 AM, Ben Hutchings <bhutchings@solarflare.com> wrote: >> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c >> index 99cd65c..5c8d49d 100644 >> --- a/net/ipv6/mcast.c >> +++ b/net/ipv6/mcast.c >> @@ -2277,12 +2277,12 @@ void ipv6_mc_down(struct inet6_dev *idev) >> >> read_lock_bh(&idev->lock); >> idev->mc_ifc_count = 0; >> - if (del_timer(&idev->mc_ifc_timer)) >> + if (del_timer_sync(&idev->mc_ifc_timer)) > > Are you sure this doesn't introduce a potential deadlock? Have you > tested this with lockdep enabled? > > Ben. I will test with lockdep enabled and get back to you. 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 ran our usual workload with lockdep enabled and didn't see any lockdep complaints. Another approach to solve this would be to use in6_dev_put instead of __in6_dev_put int the multicast code. I am not sure though why __in6_dev_put was originally used. This part of the code seems to not have been changed since the initial git check-in. Thanks, Salam On Thu, Sep 5, 2013 at 10:20 AM, Salam Noureddine <noureddine@aristanetworks.com> wrote: > On Thu, Sep 5, 2013 at 7:30 AM, Ben Hutchings <bhutchings@solarflare.com> wrote: >>> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c >>> index 99cd65c..5c8d49d 100644 >>> --- a/net/ipv6/mcast.c >>> +++ b/net/ipv6/mcast.c >>> @@ -2277,12 +2277,12 @@ void ipv6_mc_down(struct inet6_dev *idev) >>> >>> read_lock_bh(&idev->lock); >>> idev->mc_ifc_count = 0; >>> - if (del_timer(&idev->mc_ifc_timer)) >>> + if (del_timer_sync(&idev->mc_ifc_timer)) >> >> Are you sure this doesn't introduce a potential deadlock? Have you >> tested this with lockdep enabled? >> >> Ben. > > I will test with lockdep enabled and get back to you. > > 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
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c index 99cd65c..5c8d49d 100644 --- a/net/ipv6/mcast.c +++ b/net/ipv6/mcast.c @@ -2277,12 +2277,12 @@ void ipv6_mc_down(struct inet6_dev *idev) read_lock_bh(&idev->lock); idev->mc_ifc_count = 0; - if (del_timer(&idev->mc_ifc_timer)) + if (del_timer_sync(&idev->mc_ifc_timer)) __in6_dev_put(idev); idev->mc_gq_running = 0; - if (del_timer(&idev->mc_gq_timer)) + if (del_timer_sync(&idev->mc_gq_timer)) __in6_dev_put(idev); - if (del_timer(&idev->mc_dad_timer)) + if (del_timer_sync(&idev->mc_dad_timer)) __in6_dev_put(idev); for (i = idev->mc_list; i; i=i->next)
Delete timers using del_timer_sync in ipv6_mc_down. Otherwise, it is possible for the timer to be the last to release its reference to the inet6_dev and since __in6_dev_put doesn't destroy the inet6_dev 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/ipv6/mcast.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)