Re: [PATCH] 2.5.53 : net/atm/lec.c

[romieu@fr.zoreil.com]

Greetings,

Frank Davis <fdavis@si.rr.com> :
[locking in net/atm/lec.c]

net/atm/lec.c::lec_arp_destroy()
-> spin_lock_irqsave(&lec_lock, flags);         
-> lec_arp_remove(priv->lec_arp_tables, entry);  
   -> spin_lock_irqsave(&lec_lock, flags); 
      -> good bye Charlie !

Both lec_arp_check_expire() (timer function) and lec_arp_destroy() are
walking the lec_arp_table from the 'lec_priv' they were attached to and if
the intent of lec_arp_{lock/unlock} is to protect the timer function
lec_arp_check_expire() by virtue of refcounting, it seems slightly racy.
If you are wondering who the callers of lec_arp_remove() are:
lec_arp_remove
<- lec_atm_send
   <- lecdev_ops.send = lec_atm_send
<- lec_arp_destroy
   <- lec_atm_close
      <- lecdev_ops.close = lec_atm_close
<- lec_arp_check_expire
   <- priv->lec_arp_timer.function = lec_arp_check_expire;
<- lec_addr_delete
   <- lec_atm_send
<- lec_vcc_close
   <- lec_push
      <- vcc->push = lec_push;
<- lec_arp_check_empties
   <- lec_push

Removing the lock from lec_arp_remove() and asking callers to do themselves
the locking (liek lec_arp_destroy) doesn't seem too unsane.

Btw, del_timer_sync() in lec_arp_destroy() shouldn't hurt imho.

--
Ueimor