Messages in this thread | | | Date | Mon, 11 Aug 2014 15:55:58 +0530 | From | Deepak <> | Subject | Re: [RFC] net: Replace del_timer() with del_timer_sync() |
| |
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. > > >
| |