Commit 95ed63f7 authored by Arthur Kepner's avatar Arthur Kepner Committed by David S. Miller

[NET] pktgen: Fix races between control/worker threads.

There's a race in pktgen which can lead to a double
free of a pktgen_dev's skb. If a worker thread is in
the midst of doing fill_packet(), and the controlling
thread gets a "stop" message, the already freed skb
can be freed once again in pktgen_stop_device(). This
patch gives all responsibility for cleaning up a
pktgen_dev's skb to the associated worker thread.
Signed-off-by: default avatarArthur Kepner <akepner@sgi.com>
Acked-by: default avatarRobert Olsson <Robert.Olsson@data.slu.se>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 4bf07ef3
...@@ -153,7 +153,7 @@ ...@@ -153,7 +153,7 @@
#include <asm/timex.h> #include <asm/timex.h>
#define VERSION "pktgen v2.63: Packet Generator for packet performance testing.\n" #define VERSION "pktgen v2.64: Packet Generator for packet performance testing.\n"
/* #define PG_DEBUG(a) a */ /* #define PG_DEBUG(a) a */
#define PG_DEBUG(a) #define PG_DEBUG(a)
...@@ -176,7 +176,8 @@ ...@@ -176,7 +176,8 @@
#define T_TERMINATE (1<<0) #define T_TERMINATE (1<<0)
#define T_STOP (1<<1) /* Stop run */ #define T_STOP (1<<1) /* Stop run */
#define T_RUN (1<<2) /* Start run */ #define T_RUN (1<<2) /* Start run */
#define T_REMDEV (1<<3) /* Remove all devs */ #define T_REMDEVALL (1<<3) /* Remove all devs */
#define T_REMDEV (1<<4) /* Remove one dev */
/* Locks */ /* Locks */
#define thread_lock() down(&pktgen_sem) #define thread_lock() down(&pktgen_sem)
...@@ -218,6 +219,8 @@ struct pktgen_dev { ...@@ -218,6 +219,8 @@ struct pktgen_dev {
* we will do a random selection from within the range. * we will do a random selection from within the range.
*/ */
__u32 flags; __u32 flags;
int removal_mark; /* non-zero => the device is marked for
* removal by worker thread */
int min_pkt_size; /* = ETH_ZLEN; */ int min_pkt_size; /* = ETH_ZLEN; */
int max_pkt_size; /* = ETH_ZLEN; */ int max_pkt_size; /* = ETH_ZLEN; */
...@@ -481,7 +484,7 @@ static void pktgen_stop_all_threads_ifs(void); ...@@ -481,7 +484,7 @@ static void pktgen_stop_all_threads_ifs(void);
static int pktgen_stop_device(struct pktgen_dev *pkt_dev); static int pktgen_stop_device(struct pktgen_dev *pkt_dev);
static void pktgen_stop(struct pktgen_thread* t); static void pktgen_stop(struct pktgen_thread* t);
static void pktgen_clear_counters(struct pktgen_dev *pkt_dev); static void pktgen_clear_counters(struct pktgen_dev *pkt_dev);
static struct pktgen_dev *pktgen_NN_threads(const char* dev_name, int remove); static int pktgen_mark_device(const char* ifname);
static unsigned int scan_ip6(const char *s,char ip[16]); static unsigned int scan_ip6(const char *s,char ip[16]);
static unsigned int fmt_ip6(char *s,const char ip[16]); static unsigned int fmt_ip6(char *s,const char ip[16]);
...@@ -1406,7 +1409,7 @@ static ssize_t pktgen_thread_write(struct file *file, ...@@ -1406,7 +1409,7 @@ static ssize_t pktgen_thread_write(struct file *file,
if (!strcmp(name, "rem_device_all")) { if (!strcmp(name, "rem_device_all")) {
thread_lock(); thread_lock();
t->control |= T_REMDEV; t->control |= T_REMDEVALL;
thread_unlock(); thread_unlock();
schedule_timeout_interruptible(msecs_to_jiffies(125)); /* Propagate thread->control */ schedule_timeout_interruptible(msecs_to_jiffies(125)); /* Propagate thread->control */
ret = count; ret = count;
...@@ -1457,7 +1460,8 @@ static struct pktgen_dev *__pktgen_NN_threads(const char* ifname, int remove) ...@@ -1457,7 +1460,8 @@ static struct pktgen_dev *__pktgen_NN_threads(const char* ifname, int remove)
if (pkt_dev) { if (pkt_dev) {
if(remove) { if(remove) {
if_lock(t); if_lock(t);
pktgen_remove_device(t, pkt_dev); pkt_dev->removal_mark = 1;
t->control |= T_REMDEV;
if_unlock(t); if_unlock(t);
} }
break; break;
...@@ -1467,13 +1471,44 @@ static struct pktgen_dev *__pktgen_NN_threads(const char* ifname, int remove) ...@@ -1467,13 +1471,44 @@ static struct pktgen_dev *__pktgen_NN_threads(const char* ifname, int remove)
return pkt_dev; return pkt_dev;
} }
static struct pktgen_dev *pktgen_NN_threads(const char* ifname, int remove) /*
* mark a device for removal
*/
static int pktgen_mark_device(const char* ifname)
{ {
struct pktgen_dev *pkt_dev = NULL; struct pktgen_dev *pkt_dev = NULL;
const int max_tries = 10, msec_per_try = 125;
int i = 0;
int ret = 0;
thread_lock(); thread_lock();
pkt_dev = __pktgen_NN_threads(ifname, remove); PG_DEBUG(printk("pktgen: pktgen_mark_device marking %s for removal\n",
thread_unlock(); ifname));
return pkt_dev;
while(1) {
pkt_dev = __pktgen_NN_threads(ifname, REMOVE);
if (pkt_dev == NULL) break; /* success */
thread_unlock();
PG_DEBUG(printk("pktgen: pktgen_mark_device waiting for %s "
"to disappear....\n", ifname));
schedule_timeout_interruptible(msecs_to_jiffies(msec_per_try));
thread_lock();
if (++i >= max_tries) {
printk("pktgen_mark_device: timed out after waiting "
"%d msec for device %s to be removed\n",
msec_per_try*i, ifname);
ret = 1;
break;
}
}
thread_unlock();
return ret;
} }
static int pktgen_device_event(struct notifier_block *unused, unsigned long event, void *ptr) static int pktgen_device_event(struct notifier_block *unused, unsigned long event, void *ptr)
...@@ -1493,7 +1528,7 @@ static int pktgen_device_event(struct notifier_block *unused, unsigned long even ...@@ -1493,7 +1528,7 @@ static int pktgen_device_event(struct notifier_block *unused, unsigned long even
break; break;
case NETDEV_UNREGISTER: case NETDEV_UNREGISTER:
pktgen_NN_threads(dev->name, REMOVE); pktgen_mark_device(dev->name);
break; break;
}; };
...@@ -2303,11 +2338,11 @@ static void pktgen_stop_all_threads_ifs(void) ...@@ -2303,11 +2338,11 @@ static void pktgen_stop_all_threads_ifs(void)
{ {
struct pktgen_thread *t = pktgen_threads; struct pktgen_thread *t = pktgen_threads;
PG_DEBUG(printk("pktgen: entering pktgen_stop_all_threads.\n")); PG_DEBUG(printk("pktgen: entering pktgen_stop_all_threads_ifs.\n"));
thread_lock(); thread_lock();
while(t) { while(t) {
pktgen_stop(t); t->control |= T_STOP;
t = t->next; t = t->next;
} }
thread_unlock(); thread_unlock();
...@@ -2431,7 +2466,9 @@ static void show_results(struct pktgen_dev *pkt_dev, int nr_frags) ...@@ -2431,7 +2466,9 @@ static void show_results(struct pktgen_dev *pkt_dev, int nr_frags)
static int pktgen_stop_device(struct pktgen_dev *pkt_dev) static int pktgen_stop_device(struct pktgen_dev *pkt_dev)
{ {
int nr_frags = pkt_dev->skb ?
skb_shinfo(pkt_dev->skb)->nr_frags: -1;
if (!pkt_dev->running) { if (!pkt_dev->running) {
printk("pktgen: interface: %s is already stopped\n", pkt_dev->ifname); printk("pktgen: interface: %s is already stopped\n", pkt_dev->ifname);
return -EINVAL; return -EINVAL;
...@@ -2440,13 +2477,8 @@ static int pktgen_stop_device(struct pktgen_dev *pkt_dev) ...@@ -2440,13 +2477,8 @@ static int pktgen_stop_device(struct pktgen_dev *pkt_dev)
pkt_dev->stopped_at = getCurUs(); pkt_dev->stopped_at = getCurUs();
pkt_dev->running = 0; pkt_dev->running = 0;
show_results(pkt_dev, skb_shinfo(pkt_dev->skb)->nr_frags); show_results(pkt_dev, nr_frags);
if (pkt_dev->skb)
kfree_skb(pkt_dev->skb);
pkt_dev->skb = NULL;
return 0; return 0;
} }
...@@ -2469,26 +2501,66 @@ static struct pktgen_dev *next_to_run(struct pktgen_thread *t ) ...@@ -2469,26 +2501,66 @@ static struct pktgen_dev *next_to_run(struct pktgen_thread *t )
static void pktgen_stop(struct pktgen_thread *t) { static void pktgen_stop(struct pktgen_thread *t) {
struct pktgen_dev *next = NULL; struct pktgen_dev *next = NULL;
PG_DEBUG(printk("pktgen: entering pktgen_stop.\n")); PG_DEBUG(printk("pktgen: entering pktgen_stop\n"));
if_lock(t); if_lock(t);
for(next=t->if_list; next; next=next->next) for(next=t->if_list; next; next=next->next) {
pktgen_stop_device(next); pktgen_stop_device(next);
if (next->skb)
kfree_skb(next->skb);
next->skb = NULL;
}
if_unlock(t); if_unlock(t);
} }
/*
* one of our devices needs to be removed - find it
* and remove it
*/
static void pktgen_rem_one_if(struct pktgen_thread *t)
{
struct pktgen_dev *cur, *next = NULL;
PG_DEBUG(printk("pktgen: entering pktgen_rem_one_if\n"));
if_lock(t);
for(cur=t->if_list; cur; cur=next) {
next = cur->next;
if (!cur->removal_mark) continue;
if (cur->skb)
kfree_skb(cur->skb);
cur->skb = NULL;
pktgen_remove_device(t, cur);
break;
}
if_unlock(t);
}
static void pktgen_rem_all_ifs(struct pktgen_thread *t) static void pktgen_rem_all_ifs(struct pktgen_thread *t)
{ {
struct pktgen_dev *cur, *next = NULL; struct pktgen_dev *cur, *next = NULL;
/* Remove all devices, free mem */
/* Remove all devices, free mem */
PG_DEBUG(printk("pktgen: entering pktgen_rem_all_ifs\n"));
if_lock(t); if_lock(t);
for(cur=t->if_list; cur; cur=next) { for(cur=t->if_list; cur; cur=next) {
next = cur->next; next = cur->next;
if (cur->skb)
kfree_skb(cur->skb);
cur->skb = NULL;
pktgen_remove_device(t, cur); pktgen_remove_device(t, cur);
} }
...@@ -2550,6 +2622,9 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev) ...@@ -2550,6 +2622,9 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev)
if (!netif_running(odev)) { if (!netif_running(odev)) {
pktgen_stop_device(pkt_dev); pktgen_stop_device(pkt_dev);
if (pkt_dev->skb)
kfree_skb(pkt_dev->skb);
pkt_dev->skb = NULL;
goto out; goto out;
} }
if (need_resched()) if (need_resched())
...@@ -2581,7 +2656,7 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev) ...@@ -2581,7 +2656,7 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev)
pkt_dev->clone_count = 0; /* reset counter */ pkt_dev->clone_count = 0; /* reset counter */
} }
} }
spin_lock_bh(&odev->xmit_lock); spin_lock_bh(&odev->xmit_lock);
if (!netif_queue_stopped(odev)) { if (!netif_queue_stopped(odev)) {
...@@ -2644,6 +2719,9 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev) ...@@ -2644,6 +2719,9 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev)
/* Done with this */ /* Done with this */
pktgen_stop_device(pkt_dev); pktgen_stop_device(pkt_dev);
if (pkt_dev->skb)
kfree_skb(pkt_dev->skb);
pkt_dev->skb = NULL;
} }
out:; out:;
} }
...@@ -2685,6 +2763,7 @@ static void pktgen_thread_worker(struct pktgen_thread *t) ...@@ -2685,6 +2763,7 @@ static void pktgen_thread_worker(struct pktgen_thread *t)
t->control &= ~(T_TERMINATE); t->control &= ~(T_TERMINATE);
t->control &= ~(T_RUN); t->control &= ~(T_RUN);
t->control &= ~(T_STOP); t->control &= ~(T_STOP);
t->control &= ~(T_REMDEVALL);
t->control &= ~(T_REMDEV); t->control &= ~(T_REMDEV);
t->pid = current->pid; t->pid = current->pid;
...@@ -2748,8 +2827,13 @@ static void pktgen_thread_worker(struct pktgen_thread *t) ...@@ -2748,8 +2827,13 @@ static void pktgen_thread_worker(struct pktgen_thread *t)
t->control &= ~(T_RUN); t->control &= ~(T_RUN);
} }
if(t->control & T_REMDEV) { if(t->control & T_REMDEVALL) {
pktgen_rem_all_ifs(t); pktgen_rem_all_ifs(t);
t->control &= ~(T_REMDEVALL);
}
if(t->control & T_REMDEV) {
pktgen_rem_one_if(t);
t->control &= ~(T_REMDEV); t->control &= ~(T_REMDEV);
} }
...@@ -2833,6 +2917,7 @@ static int pktgen_add_device(struct pktgen_thread *t, const char* ifname) ...@@ -2833,6 +2917,7 @@ static int pktgen_add_device(struct pktgen_thread *t, const char* ifname)
} }
memset(pkt_dev->flows, 0, MAX_CFLOWS*sizeof(struct flow_state)); memset(pkt_dev->flows, 0, MAX_CFLOWS*sizeof(struct flow_state));
pkt_dev->removal_mark = 0;
pkt_dev->min_pkt_size = ETH_ZLEN; pkt_dev->min_pkt_size = ETH_ZLEN;
pkt_dev->max_pkt_size = ETH_ZLEN; pkt_dev->max_pkt_size = ETH_ZLEN;
pkt_dev->nfrags = 0; pkt_dev->nfrags = 0;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment