netdev - Re: [PATCH net] inet: fix potential deadlock in reqsk_queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1439905003.6443.2.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Tue, 18 Aug 2015 06:36:43 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	David Laight <David.Laight@...LAB.COM>
Cc:	David Miller <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] inet: fix potential deadlock in reqsk_queue_unlink()

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html