Re: [RFC] net: Replace del_timer() with del_timer_sync()

[Deepak <deepak_das@mentor.com>]

Thanks for the clarification Eric.
I re-analysed the code and found that sk_stop_timer() is called under 
lock_sock(sk)/bh_lock_sock(sk) so we can not replace del_timer() with 
del_timer_sync() here and will lead to dead-lock as you suggested .

Thanks,
Deepak Das

On Thursday 07 August 2014 10:18 PM, Eric Dumazet wrote:
> On Thu, 2014-08-07 at 15:15 +0000, Das, Deepak wrote:
>
> Please do not top post on netdev, thanks.
>
>> I apologies for not explaining the scenario previously.
>>
>> sk_stop_timer() is used to stop the tcp timers with expiry callback
>> tcp_write_timer(), tcp_delack_timer(), tcp_keepalive_timer(), ...
>> del_timer() is used to stop the the timer in sk_stop_timer(), which
>> might return a non-zero result even if one of these timer handler functions
>> (tcp_write_timer(), tcp_delack_timer(), tcp_keepalive_timer(), ...)
>> is already executing on another processor.
>>
>> Following is the possible scenario :-
>> on CPU #0: sk_stop_timer() decrements the sk->sk_refcnt if del_timer(timer)
>> returns non-zero.
>>
>> on CPU #1: If a timer handler callback runs then it also calls sock_put(sk)
>> which decrements sk->sk_refcnt and if the sk_refcnt becomes zero it frees the
>> structure sock pointed to by sk.
>>
>> if the sk->sk_refcnt decrements twice then that will cause a mismatch in the
>> number of "puts" and "holds" resulting in a malfunction of the sk->sk_refcnt mechanism.
> Not at all.
>
> There is no mismatch, sk_refcnt is decremented once in all cases.
>
> I believe you misunderstood del_timer_sync() / del_timer() behaviors and
> differences.
>
> In the case you describe, del_timer() should return 0, and timer
> function will call sock_put() to decrement socket refcount.
>
> The problem' of del_timer() is the following :
>
> When/If it returns 0, another cpu _might_ be running the timer, we have
> no guarantee timer function is completed.
>
> For sockets, we do not care, because the active timer owns a refcount on
> the socket. When timer is finally completed, refcount will be released.
>
>> The solution is to use del_timer_sync() instead of del_timer()
>> because del_timer_sync() will wait for timer handler functions to
>> complete execution.
> Except that some sk_stop_timer() callers hold the socket lock, so the
> timer will deadlock trying to acquire it.
>
>> yes, we are facing some memory corruption issues due to access of already released
>> struct sock in our environment. Our memory corruption issue looks like memory locations
>> being decremented which could be consistent with a rogue decrement of a reference counter.
> Is 'Your environment' some out of tree module or is it part of standard
> linux kernel ?
>
>> similar suggestion is also made by Dean Jenkins in rfcomm_dlc_clear_timer() and accepted by Marcel.
>> http://www.spinics.net/lists/linux-bluetooth/msg51132.html
> Fix might be good in this case, but the changelog is completely bogus.
>
>
>