[PATCH 1/2] ipv6 mcast: use del_timer_sync instead of del_timer in ipv6_mc_down

[PATCH 1/2] ipv6 mcast: use del_timer_sync instead of del_timer in ipv6_mc_down

[Salam Noureddine]

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))
 		__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)
-- 
1.7.4.4

Re: [PATCH 1/2] ipv6 mcast: use del_timer_sync instead of del_timer in ipv6_mc_down

[Ben Hutchings]

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)

-- 
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.

Re: [PATCH 1/2] ipv6 mcast: use del_timer_sync instead of del_timer in ipv6_mc_down

[Salam Noureddine]

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

Re: [PATCH 1/2] ipv6 mcast: use del_timer_sync instead of del_timer in ipv6_mc_down

[Salam Noureddine]

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