diff mbox

[tip/core/rcu,5/5] rcu: Prevent initialization race in rcutorture kthreads

Message ID 1346352312-31987-5-git-send-email-paulmck@linux.vnet.ibm.com
State Accepted
Commit 60f53782c51f27c695840ce90c6c432284319eef
Headers show

Commit Message

Paul E. McKenney Aug. 30, 2012, 6:45 p.m. UTC 
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

When you do something like "t = kthread_run(...)", it is possible that
the kthread will start running before the assignment to "t" happens.
If the child kthread expects to find a pointer to its task_struct in "t",
it will then be fatally disappointed.  This commit therefore switches
such cases to kthread_create() followed by wake_up_process(), guaranteeing
that the assignment happens before the child kthread starts running.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutorture.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

Comments

Josh Triplett Aug. 30, 2012, 7:15 p.m. UTC | #1 
On Thu, Aug 30, 2012 at 11:45:12AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> 
> When you do something like "t = kthread_run(...)", it is possible that
> the kthread will start running before the assignment to "t" happens.
> If the child kthread expects to find a pointer to its task_struct in "t",
> it will then be fatally disappointed.  This commit therefore switches
> such cases to kthread_create() followed by wake_up_process(), guaranteeing
> that the assignment happens before the child kthread starts running.
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Seems like you should go ahead and make this change for all the threads,
not just two of them.  A simple wrapper around kthread_run, taking a
struct task_struct ** to write to, would make this much simpler.  Such a
wrapper could also return an error code directly (for use in firsterr),
write NULL to the pointer on error, and perhaps print an error message,
which would remove most of the boilerplate currently duplicated for
every thread creation.

Arguably, all of those except the error message printing would make
sense as changes to kthread_run itself, but that's another patch. :)

- Josh Triplett
diff mbox

Patch

diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
index 8ea2726..3e2adae 100644
--- a/kernel/rcutorture.c
+++ b/kernel/rcutorture.c
@@ -2028,14 +2028,15 @@  rcu_torture_init(void)
 	/* Start up the kthreads. */
 
 	VERBOSE_PRINTK_STRING("Creating rcu_torture_writer task");
-	writer_task = kthread_run(rcu_torture_writer, NULL,
-				  "rcu_torture_writer");
+	writer_task = kthread_create(rcu_torture_writer, NULL,
+				     "rcu_torture_writer");
 	if (IS_ERR(writer_task)) {
 		firsterr = PTR_ERR(writer_task);
 		VERBOSE_PRINTK_ERRSTRING("Failed to create writer");
 		writer_task = NULL;
 		goto unwind;
 	}
+	wake_up_process(writer_task);
 	fakewriter_tasks = kzalloc(nfakewriters * sizeof(fakewriter_tasks[0]),
 				   GFP_KERNEL);
 	if (fakewriter_tasks == NULL) {
@@ -2150,14 +2151,15 @@  rcu_torture_init(void)
 	}
 	if (shutdown_secs > 0) {
 		shutdown_time = jiffies + shutdown_secs * HZ;
-		shutdown_task = kthread_run(rcu_torture_shutdown, NULL,
-					    "rcu_torture_shutdown");
+		shutdown_task = kthread_create(rcu_torture_shutdown, NULL,
+					       "rcu_torture_shutdown");
 		if (IS_ERR(shutdown_task)) {
 			firsterr = PTR_ERR(shutdown_task);
 			VERBOSE_PRINTK_ERRSTRING("Failed to create shutdown");
 			shutdown_task = NULL;
 			goto unwind;
 		}
+		wake_up_process(shutdown_task);
 	}
 	i = rcu_torture_onoff_init();
 	if (i != 0) {