Re: [PATCH net-next 4/6] 6lowpan: rework fragment-deleting routine
From: Eric Dumazet <hidden>
Date: 2012-07-09 10:53:41
On Mon, 2012-07-09 at 14:22 +0400, Alexander Smirnov wrote:
quoted hunk ↗ jump to hunk
6lowpan module starts collecting incomming frames and fragments right after lowpan_module_init() therefor it will be better to clean unfinished fragments in lowpan_cleanup_module() function instead of doing it when link goes down. Changed spinlocks type to prevent deadlock with expired timer event and removed unused one. Signed-off-by: Alexander Smirnov <redacted> --- net/ieee802154/6lowpan.c | 28 ++++++++++++++++------------ 1 files changed, 16 insertions(+), 12 deletions(-)diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c index b872515..e7de085 100644 --- a/net/ieee802154/6lowpan.c +++ b/net/ieee802154/6lowpan.c@@ -113,7 +113,6 @@ struct lowpan_dev_record { struct lowpan_fragment { struct sk_buff *skb; /* skb to be assembled */ - spinlock_t lock; /* concurency lock */ u16 length; /* length to be assemled */ u32 bytes_rcv; /* bytes received */ u16 tag; /* current fragment tag */@@ -761,7 +760,7 @@ lowpan_process_data(struct sk_buff *skb) if ((frame->bytes_rcv == frame->length) && frame->timer.expires > jiffies) { /* if timer haven't expired - first of all delete it */ - del_timer(&frame->timer); + del_timer_sync(&frame->timer); list_del(&frame->list); spin_unlock(&flist_lock);@@ -1196,19 +1195,9 @@ static void lowpan_dellink(struct net_device *dev, struct list_head *head) struct lowpan_dev_info *lowpan_dev = lowpan_dev_info(dev); struct net_device *real_dev = lowpan_dev->real_dev; struct lowpan_dev_record *entry, *tmp; - struct lowpan_fragment *frame, *tframe; ASSERT_RTNL(); - spin_lock(&flist_lock); - list_for_each_entry_safe(frame, tframe, &lowpan_fragments, list) { - del_timer(&frame->timer); - list_del(&frame->list); - dev_kfree_skb(frame->skb); - kfree(frame); - } - spin_unlock(&flist_lock); - mutex_lock(&lowpan_dev_info(dev)->dev_list_mtx); list_for_each_entry_safe(entry, tmp, &lowpan_devices, list) { if (entry->ldev == dev) {@@ -1264,9 +1253,24 @@ out: static void __exit lowpan_cleanup_module(void) { + struct lowpan_fragment *frame, *tframe; + lowpan_netlink_fini(); dev_remove_pack(&lowpan_packet_type); + + /* Now 6lowpan packet_type is removed, so no new fragments are + * expected on RX, therefore that's the time to clean incomplete + * fragments. + */ + spin_lock_bh(&flist_lock); + list_for_each_entry_safe(frame, tframe, &lowpan_fragments, list) { + del_timer_sync(&frame->timer); + list_del(&frame->list); + dev_kfree_skb(frame->skb); + kfree(frame); + } + spin_unlock_bh(&flist_lock); } module_init(lowpan_init_module);
Problem is lowpan_fragment_timer_expired() can race with this code. del_timer_sync() might block here if lowpan_fragment_timer_expired() is waiting/spinning for spin_lock(&flist_lock) You cant call del_timer_sync() holding flist_lock, you should find another way to solve the problem. Its explained in kernel/timer.c : #ifdef CONFIG_SMP /** * 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. 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: 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. */