Re: [PATCH net-next 4/6] 6lowpan: rework fragment-deleting routine

[Eric Dumazet <eric.dumazet@gmail.com>]

On Mon, 2012-07-09 at 14:22 +0400, Alexander Smirnov wrote:
> 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 <alex.bluesman.smirnov@gmail.com>
> ---
>  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.
 */