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/
This archive was generated by hypermail 2b29 : Wed Feb 23 2000 - 21:00:27 EST