Messages in this thread Patch in this message | | | Date | Mon, 21 Feb 2000 16:57:29 +0100 | From | Jan Kasprzak <> | Subject | Patch: syncppp.c locking |
| |
Hello all,
I have found a potential race condition in the syncppp.c code: The spppq list is modified without any lock, causing possible troubles when two devices call sppp_attach() and/or sppp_detach() simultaneously. I have added the spppq_lock spinlock to protect the modifications of spppq.
I am not sure about the sppp_keepalive() function, which walks through the spppq list and protects itself using a global cli(). I think it would be better to change this to use the spinlock as well.
Is it OK to add another spinlock, or should I use some existing spinlock instead?
Patch is relative to 2.3.46.
-Yenya
--- syncppp.c.orig Mon Feb 21 16:27:19 2000 +++ syncppp.c Mon Feb 21 16:53:45 2000 @@ -50,6 +50,7 @@ #include <linux/random.h> #include <linux/pkt_sched.h> #include <asm/byteorder.h> +#include <linux/spinlock.h> #include "syncppp.h" #define MAXALIVECNT 6 /* max. alive packets */ @@ -126,6 +127,7 @@ static struct sppp *spppq; static struct timer_list sppp_keepalive_timer; +static spinlock_t spppq_lock; static void sppp_keepalive (unsigned long dummy); static void sppp_cp_send (struct sppp *sp, u16 proto, u8 type, @@ -359,8 +361,8 @@ { struct sppp *sp; unsigned long flags; - save_flags(flags); - cli(); + + spin_lock_irqsave(&spppq_lock, flags); for (sp=spppq; sp; sp=sp->pp_next) { @@ -402,7 +404,7 @@ sp->lcp.echoid, 4, &nmagic); } } - restore_flags(flags); + spin_unlock_irqrestore(&spppq_lock, flags); sppp_keepalive_timer.expires=jiffies+10*HZ; add_timer(&sppp_keepalive_timer); } @@ -915,7 +917,9 @@ { struct net_device *dev = pd->dev; struct sppp *sp = &pd->sppp; - + unsigned long flags; + + spin_lock_irqsave(&spppq_lock, flags); /* Initialize keepalive handler. */ if (! spppq) { @@ -927,6 +931,7 @@ /* Insert new entry into the keepalive list. */ sp->pp_next = spppq; spppq = sp; + spin_unlock_irqrestore(&spppq_lock, flags); sp->pp_loopcnt = 0; sp->pp_alivecnt = 0; @@ -971,7 +976,9 @@ void sppp_detach (struct net_device *dev) { struct sppp **q, *p, *sp = (struct sppp *)sppp_of(dev); + unsigned long flags; + spin_lock_irqsave(&spppq_lock, flags); /* Remove the entry from the keepalive list. */ for (q = &spppq; (p = *q); q = &p->pp_next) if (p == sp) { @@ -983,6 +990,7 @@ if (! spppq) del_timer(&sppp_keepalive_timer); sppp_clear_timeout (sp); + spin_unlock_irqrestore(&spppq_lock, flags); } EXPORT_SYMBOL(sppp_detach); @@ -1292,6 +1300,7 @@ { printk(KERN_INFO "Cronyx Ltd, Synchronous PPP and CISCO HDLC (c) 1994\n"); printk(KERN_INFO "Linux port (c) 1998 Building Number Three Ltd & Jan \"Yenya\" Kasprzak.\n"); + spin_lock_init(&spppq_lock); sppp_packet_type.type=htons(ETH_P_WAN_PPP); dev_add_pack(&sppp_packet_type); } -- \ Jan "Yenya" Kasprzak <kas at fi.muni.cz> http://www.fi.muni.cz/~kas/ \\ PGP: finger kas at aisa.fi.muni.cz 0D99A7FB206605D7 8B35FCDE05B18A5E // \\\ Czech Linux Homepage: http://www.linux.cz/ /// Its purely bandwidth. If it was 40 instances of Miguel reading web pages flat out over 100baseT you would definitely be right. But its not... (Alan) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |