From: Frank Ch. Eigler <fche@elastic.org>
Subject: PR10651 / RHBZ653286: mod_timer vs. del_timer_sync races
References: bnc#664105
Patch-Mainline: yes
Git-commit: f2b610b6295628ae1fef744421c293883f1e4298
Git-repo: git://sources.redhat.com/git/systemtap.git

Signed-off-by: Thomas Renninger <trenn@suse.de>

It appears possible for del_timer_sync (from outside) and mod_timer
(from within a timer callback) to race.  Defeat this race by ensuring
that the timer callback checks an atomic_t flag before rescheduling
itself with mod_timer.

* runtime/transport/relay_v2.c (transport_state): Turn into an atomic_t.
  Update users.
  (__stp_relay_wakeup_timer): Observe flag.
  (_stp_transport_data_fs_{start,stop}): Update flag before timer manipulations.
* runtime/transport/ring_buffer.c: Ditto for corresponding functions.

diff --git a/runtime/transport/relay_v2.c b/runtime/transport/relay_v2.c
index 974941d..33e198e 100644
--- a/runtime/transport/relay_v2.c
+++ b/runtime/transport/relay_v2.c
@@ -4,7 +4,7 @@
  * code started as a proposed relayfs interface called 'utt'.  It has
  * been modified and simplified for systemtap.
  *
- * Changes Copyright (C) 2009 Red Hat Inc.
+ * Changes Copyright (C) 2009-2010 Red Hat Inc.
  *
  * Original utt code by:
  *   Copyright (C) 2006 Jens Axboe <axboe@suse.de>
@@ -42,7 +42,7 @@
  * to be changed. */
 struct _stp_relay_data_type {
 	struct rchan *rchan;
-	enum _stp_transport_state transport_state;
+	atomic_t /* enum _stp_transport_state */ transport_state;
 	struct dentry *dropped_file;
 	atomic_t dropped;
 	atomic_t wakeup;
@@ -133,7 +133,10 @@ static void __stp_relay_wakeup_timer(unsigned long val)
 #endif
 	}
 
- 	mod_timer(&_stp_relay_data.timer, jiffies + STP_RELAY_TIMER_INTERVAL);
+	if (atomic_read(&_stp_relay_data.transport_state) == STP_TRANSPORT_RUNNING)
+        	mod_timer(&_stp_relay_data.timer, jiffies + STP_RELAY_TIMER_INTERVAL);
+        else
+		dbug_trans(0, "relay_v2 wakeup timer expiry\n");
 }
 
 static void __stp_relay_timer_init(void)
@@ -149,7 +152,7 @@ static void __stp_relay_timer_init(void)
 
 static enum _stp_transport_state _stp_transport_get_state(void)
 {
-	return _stp_relay_data.transport_state;
+	return atomic_read (&_stp_relay_data.transport_state);
 }
 
 static void _stp_transport_data_fs_overwrite(int overwrite)
@@ -242,19 +245,19 @@ static struct rchan_callbacks __stp_relay_callbacks = {
 
 static void _stp_transport_data_fs_start(void)
 {
-	if (_stp_relay_data.transport_state == STP_TRANSPORT_INITIALIZED) {
+	if (atomic_read (&_stp_relay_data.transport_state) == STP_TRANSPORT_INITIALIZED) {
+		atomic_set (&_stp_relay_data.transport_state, STP_TRANSPORT_RUNNING);
 		/* We're initialized.  Now start the timer. */
 		__stp_relay_timer_init();
-		_stp_relay_data.transport_state = STP_TRANSPORT_RUNNING;
 	}
 }
 
 static void _stp_transport_data_fs_stop(void)
 {
-	if (_stp_relay_data.transport_state == STP_TRANSPORT_RUNNING) {
+	if (atomic_read (&_stp_relay_data.transport_state) == STP_TRANSPORT_RUNNING) {
+		atomic_set (&_stp_relay_data.transport_state, STP_TRANSPORT_STOPPED);
 		del_timer_sync(&_stp_relay_data.timer);
 		dbug_trans(0, "flushing...\n");
-		_stp_relay_data.transport_state = STP_TRANSPORT_STOPPED;
 		if (_stp_relay_data.rchan)
 			relay_flush(_stp_relay_data.rchan);
 	}
@@ -277,7 +280,7 @@ static int _stp_transport_data_fs_init(void)
 	u64 npages;
 	struct sysinfo si;
 
-	_stp_relay_data.transport_state = STP_TRANSPORT_STOPPED;
+	atomic_set(&_stp_relay_data.transport_state, STP_TRANSPORT_STOPPED);
 	_stp_relay_data.overwrite_flag = 0;
 	atomic_set(&_stp_relay_data.dropped, 0);
 	_stp_relay_data.dropped_file = NULL;
@@ -337,7 +340,7 @@ static int _stp_transport_data_fs_init(void)
 		goto err;
 	}
 	dbug_trans(1, "returning 0...\n");
-	_stp_relay_data.transport_state = STP_TRANSPORT_INITIALIZED;
+	atomic_set (&_stp_relay_data.transport_state, STP_TRANSPORT_INITIALIZED);
 
 	return 0;
 
diff --git a/runtime/transport/ring_buffer.c b/runtime/transport/ring_buffer.c
index 6c327c0..232f099 100644
--- a/runtime/transport/ring_buffer.c
+++ b/runtime/transport/ring_buffer.c
@@ -55,7 +55,7 @@ struct _stp_iterator {
 #endif
 
 struct _stp_relay_data_type {
-	enum _stp_transport_state transport_state;
+        atomic_t /* enum _stp_transport_state */ transport_state;
 	struct ring_buffer *rb;
 	struct _stp_iterator iter[NR_ITERS];
 	cpumask_var_t trace_reader_cpumask;
@@ -649,10 +649,12 @@ static int _stp_data_write_commit(void *entry)
 
 static void __stp_relay_wakeup_timer(unsigned long val)
 {
-	if (waitqueue_active(&_stp_poll_wait)
-	    && ! _stp_ring_buffer_empty())
+	if (waitqueue_active(&_stp_poll_wait) && ! _stp_ring_buffer_empty())
 		wake_up_interruptible(&_stp_poll_wait);
- 	mod_timer(&_stp_relay_data.timer, jiffies + STP_RELAY_TIMER_INTERVAL);
+	if (atomic_read(&_stp_relay_data.transport_state) == STP_TRANSPORT_RUNNING)
+        	mod_timer(&_stp_relay_data.timer, jiffies + STP_RELAY_TIMER_INTERVAL);
+        else
+		dbug_trans(0, "ring_buffer wakeup timer expiry\n");
 }
 
 static void __stp_relay_timer_start(void)
@@ -677,7 +679,7 @@ static int _stp_transport_data_fs_init(void)
 	int rc;
 	int cpu, cpu2;
 
-	_stp_relay_data.transport_state = STP_TRANSPORT_STOPPED;
+	atomic_set (&_stp_relay_data.transport_state, STP_TRANSPORT_STOPPED);
 	_stp_relay_data.rb = NULL;
 
 	// allocate buffer
@@ -738,23 +740,23 @@ static int _stp_transport_data_fs_init(void)
 	}
 
 	dbug_trans(1, "returning 0...\n");
-	_stp_relay_data.transport_state = STP_TRANSPORT_INITIALIZED;
+	atomic_set (&_stp_relay_data.transport_state, STP_TRANSPORT_INITIALIZED);
 	return 0;
 }
 
 static void _stp_transport_data_fs_start(void)
 {
-	if (_stp_relay_data.transport_state == STP_TRANSPORT_INITIALIZED) {
-		__stp_relay_timer_start();
-		_stp_relay_data.transport_state = STP_TRANSPORT_RUNNING;
+	if (atomic_read(&_stp_relay_data.transport_state) == STP_TRANSPORT_INITIALIZED) {
+        	atomic_set(&_stp_relay_data.transport_state, STP_TRANSPORT_RUNNING);
+                __stp_relay_timer_start();
 	}
 }
 
 static void _stp_transport_data_fs_stop(void)
 {
-	if (_stp_relay_data.transport_state == STP_TRANSPORT_RUNNING) {
-		__stp_relay_timer_stop();
-		_stp_relay_data.transport_state = STP_TRANSPORT_STOPPED;
+	if (atomic_read(&_stp_relay_data.transport_state) == STP_TRANSPORT_RUNNING) {
+        	atomic_set(&_stp_relay_data.transport_state, STP_TRANSPORT_STOPPED);
+                __stp_relay_timer_stop();
 	}
 }
 
@@ -773,7 +775,7 @@ static void _stp_transport_data_fs_close(void)
 
 static enum _stp_transport_state _stp_transport_get_state(void)
 {
-	return _stp_relay_data.transport_state;
+	return atomic_read (&_stp_relay_data.transport_state);
 }
 
 static void _stp_transport_data_fs_overwrite(int overwrite)