| Message ID | 1439505891.7960.38.camel@edumazet-glaptop2.roam.corp.google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 13 Aug 2015 15:44:51 -0700 > From: Eric Dumazet <edumazet@google.com> > > When replacing del_timer() with del_timer_sync(), I introduced > a deadlock condition : > > reqsk_queue_unlink() is called from inet_csk_reqsk_queue_drop() > > inet_csk_reqsk_queue_drop() can be called from many contexts, > one being the timer handler itself (reqsk_timer_handler()). > > In this case, del_timer_sync() loops forever. > > Simple fix is to test if timer is pending. > > Fixes: 2235f2ac75fd ("inet: fix races with reqsk timers") > Signed-off-by: Eric Dumazet <edumazet@google.com> Applied, thanks Eric. -- 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 Tue, 2015-08-18 at 11:04 +0000, David Laight wrote: > From: Eric Dumazet > > Sent: 13 August 2015 23:45 > > When replacing del_timer() with del_timer_sync(), I introduced > > a deadlock condition : > > > > reqsk_queue_unlink() is called from inet_csk_reqsk_queue_drop() > > > > inet_csk_reqsk_queue_drop() can be called from many contexts, > > one being the timer handler itself (reqsk_timer_handler()). > > > > In this case, del_timer_sync() loops forever. > > > > Simple fix is to test if timer is pending. > > Doesn't that mean you fail to wait for the timer function > to finish if you are calling from a different context? No, this is the purpose of del_timer_sync() > > What you need to know is whether the current context > is that running the timer itself. > In which case you need to mark the timer 'to be deleted' > and actually delete it when the timer function returns. > (so other code can still wait for completion.) Please read del_timer_sync() documentation and/or implementation if you have doubts. /** * del_timer_sync - deactivate a timer and wait for the handler to finish. * @timer: the timer to be deactivated * * This function only differs from del_timer() on SMP: besides deactivating * the timer it also makes sure the handler has finished executing on other * CPUs. * * Synchronization rules: Callers must prevent restarting of the timer, * otherwise this function is meaningless. It must not be called from * interrupt contexts unless the timer is an irqsafe one. The caller must * not hold locks which would prevent completion of the timer's * handler. The timer's handler must not call add_timer_on(). Upon exit the * timer is not queued and the handler is not running on any CPU. * * Note: For !irqsafe timers, you must not hold locks that are held in * interrupt context while calling this function. Even if the lock has * nothing to do with the timer in question. Here's why: * * CPU0 CPU1 * ---- ---- * <SOFTIRQ> * call_timer_fn(); * base->running_timer = mytimer; * spin_lock_irq(somelock); * <IRQ> * spin_lock(somelock); * del_timer_sync(mytimer); * while (base->running_timer == mytimer); * * Now del_timer_sync() will never return and never release somelock. * The interrupt on the other CPU is waiting to grab somelock but * it has interrupted the softirq that CPU0 is waiting to finish. * * The function returns whether it has deactivated a pending timer or not. */ -- 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
From: Eric Dumazet > Sent: 18 August 2015 14:37 > On Tue, 2015-08-18 at 11:04 +0000, David Laight wrote: > > From: Eric Dumazet > > > Sent: 13 August 2015 23:45 > > > When replacing del_timer() with del_timer_sync(), I introduced > > > a deadlock condition : > > > > > > reqsk_queue_unlink() is called from inet_csk_reqsk_queue_drop() > > > > > > inet_csk_reqsk_queue_drop() can be called from many contexts, > > > one being the timer handler itself (reqsk_timer_handler()). > > > > > > In this case, del_timer_sync() loops forever. > > > > > > Simple fix is to test if timer is pending. > > > > Doesn't that mean you fail to wait for the timer function > > to finish if you are calling from a different context? > > No, this is the purpose of del_timer_sync() I wasn't worried about del_timer_sync(). The problem is the call to timer_pending(). If the timer has just expired, and the timeout function is running (on another cpu) then timer_pending() will return false. So any tidyup path (apart from that called by the timeout function itself) will fail to wait for the function to finish. So, in effect, you've converted the code back into a call to del_timer(). timer_pending() should probably never be called without the relevant timer lock help - because the result is stale. David
On Tue, 2015-08-18 at 13:57 +0000, David Laight wrote: > s the purpose of del_timer_sync() > > I wasn't worried about del_timer_sync(). > The problem is the call to timer_pending(). > > If the timer has just expired, and the timeout function is running > (on another cpu) then timer_pending() will return false. This case is not a problem. refcount will be properly handled. No leak, no reuse, no crash. > So any tidyup path (apart from that called by the timeout function itself) > will fail to wait for the function to finish. > > So, in effect, you've converted the code back into a call to del_timer(). This is fine. > > timer_pending() should probably never be called without the relevant > timer lock help - because the result is stale. Thats absolutely not true. Please read again timer code and linux code, you'll see lot of timer uses without a lock being held (unserialized use of timers) This is why we have mod_timer_pending() for example. -- 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/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index 05e3145f7dc3..134957159c27 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -593,7 +593,7 @@ static bool reqsk_queue_unlink(struct request_sock_queue *queue, } spin_unlock(&queue->syn_wait_lock); - if (del_timer_sync(&req->rsk_timer)) + if (timer_pending(&req->rsk_timer) && del_timer_sync(&req->rsk_timer)) reqsk_put(req); return found; }