[PATCH] uml: fix thread startup race

This fixes a race in the starting of write_sigio_thread. Previously, some of the data needed by the thread was initialized after the clone. If the thread ran immediately, it would see the uninitialized data, including an empty pollfds, which would cause it to hang. We move the data initialization to before the clone, and adjust the error paths and cleanup accordingly. Signed-off-by: Jeff Dike <jdike@addtoit.com> Cc: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>

[PATCH] uml: fix thread startup race
This fixes a race in the starting of write_sigio_thread. Previously, some of the data needed by the thread was initialized after the clone. If the thread ran immediately, it would see the uninitialized data, including an empty pollfds, which would cause it to hang. We move the data initialization to before the clone, and adjust the error paths and cleanup accordingly. Signed-off-by: Jeff Dike <jdike@addtoit.com> Cc: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
Jeff Dike · Linus Torvalds
1 parent 1fbbd6844e
Showing 1 changed file with 27 additions and 28 deletions Side-by-side Diff
arch/um/os-Linux/sigio.c
@@ -29,9 +29,11 @@
  * the descriptors closed after it is killed.  So, it can't see them change.
  * On the UML side, they are changed under the sigio_lock.
  */
-static int write_sigio_fds[2] = { -1, -1 };
-static int sigio_private[2] = { -1, -1 };
+#define SIGIO_FDS_INIT {-1, -1}
  
+static int write_sigio_fds[2] = SIGIO_FDS_INIT;
+static int sigio_private[2] = SIGIO_FDS_INIT;
+
 struct pollfds {
 	struct pollfd *poll;
 	int size;
  
  
  
  
  
  
  
  
  
  
  
@@ -270,49 +272,46 @@
 	/* Did we race? Don't try to optimize this, please, it's not so likely
 	 * to happen, and no more than once at the boot. */
 	if(write_sigio_pid != -1)
-		goto out_unlock;
+		goto out_free;
  
-	write_sigio_pid = run_helper_thread(write_sigio_thread, NULL,
-					    CLONE_FILES | CLONE_VM, &stack, 0);
+	current_poll = ((struct pollfds) { .poll 	= p,
+					   .used 	= 1,
+					   .size 	= 1 });
  
-	if (write_sigio_pid < 0)
-		goto out_clear;
-
 	if (write_sigio_irq(l_write_sigio_fds[0]))
-		goto out_kill;
+		goto out_clear_poll;
  
-	/* Success, finally. */
 	memcpy(write_sigio_fds, l_write_sigio_fds, sizeof(l_write_sigio_fds));
 	memcpy(sigio_private, l_sigio_private, sizeof(l_sigio_private));
  
-	current_poll = ((struct pollfds) { .poll 	= p,
-					   .used 	= 1,
-					   .size 	= 1 });
+	write_sigio_pid = run_helper_thread(write_sigio_thread, NULL,
+					    CLONE_FILES | CLONE_VM, &stack, 0);
  
+	if (write_sigio_pid < 0)
+		goto out_clear;
+
 	sigio_unlock();
 	return;
  
- out_kill:
-	l_write_sigio_pid = write_sigio_pid;
+out_clear:
 	write_sigio_pid = -1;
-	sigio_unlock();
-	/* Going to call waitpid, avoid holding the lock. */
-	os_kill_process(l_write_sigio_pid, 1);
-	goto out_free;
-
- out_clear:
-	write_sigio_pid = -1;
- out_unlock:
-	sigio_unlock();
- out_free:
+	write_sigio_fds[0] = -1;
+	write_sigio_fds[1] = -1;
+	sigio_private[0] = -1;
+	sigio_private[1] = -1;
+out_clear_poll:
+	current_poll = ((struct pollfds) { .poll	= NULL,
+					   .size	= 0,
+					   .used	= 0 });
+out_free:
 	kfree(p);
- out_close2:
+	sigio_unlock();
+out_close2:
 	close(l_sigio_private[0]);
 	close(l_sigio_private[1]);
- out_close1:
+out_close1:
 	close(l_write_sigio_fds[0]);
 	close(l_write_sigio_fds[1]);
-	return;
 }
  
 void sigio_cleanup(void)
...	...	@@ -29,9 +29,11 @@
29	29	* the descriptors closed after it is killed. So, it can't see them change.
30	30	* On the UML side, they are changed under the sigio_lock.
31	31	*/
32		-static int write_sigio_fds[2] = { -1, -1 };
33		-static int sigio_private[2] = { -1, -1 };
	32	+#define SIGIO_FDS_INIT {-1, -1}
34	33
	34	+static int write_sigio_fds[2] = SIGIO_FDS_INIT;
	35	+static int sigio_private[2] = SIGIO_FDS_INIT;
	36	+
35	37	struct pollfds {
36	38	struct pollfd *poll;
37	39	int size;
38	40
39	41
40	42
41	43
42	44
43	45
44	46
45	47
46	48
47	49
48	50
...	...	@@ -270,49 +272,46 @@
270	272	/* Did we race? Don't try to optimize this, please, it's not so likely
271	273	* to happen, and no more than once at the boot. */
272	274	if(write_sigio_pid != -1)
273		- goto out_unlock;
	275	+ goto out_free;
274	276
275		- write_sigio_pid = run_helper_thread(write_sigio_thread, NULL,
276		- CLONE_FILES \| CLONE_VM, &stack, 0);
	277	+ current_poll = ((struct pollfds) { .poll = p,
	278	+ .used = 1,
	279	+ .size = 1 });
277	280
278		- if (write_sigio_pid < 0)
279		- goto out_clear;
280		-
281	281	if (write_sigio_irq(l_write_sigio_fds[0]))
282		- goto out_kill;
	282	+ goto out_clear_poll;
283	283
284		- /* Success, finally. */
285	284	memcpy(write_sigio_fds, l_write_sigio_fds, sizeof(l_write_sigio_fds));
286	285	memcpy(sigio_private, l_sigio_private, sizeof(l_sigio_private));
287	286
288		- current_poll = ((struct pollfds) { .poll = p,
289		- .used = 1,
290		- .size = 1 });
	287	+ write_sigio_pid = run_helper_thread(write_sigio_thread, NULL,
	288	+ CLONE_FILES \| CLONE_VM, &stack, 0);
291	289
	290	+ if (write_sigio_pid < 0)
	291	+ goto out_clear;
	292	+
292	293	sigio_unlock();
293	294	return;
294	295
295		- out_kill:
296		- l_write_sigio_pid = write_sigio_pid;
	296	+out_clear:
297	297	write_sigio_pid = -1;
298		- sigio_unlock();
299		- /* Going to call waitpid, avoid holding the lock. */
300		- os_kill_process(l_write_sigio_pid, 1);
301		- goto out_free;
302		-
303		- out_clear:
304		- write_sigio_pid = -1;
305		- out_unlock:
306		- sigio_unlock();
307		- out_free:
	298	+ write_sigio_fds[0] = -1;
	299	+ write_sigio_fds[1] = -1;
	300	+ sigio_private[0] = -1;
	301	+ sigio_private[1] = -1;
	302	+out_clear_poll:
	303	+ current_poll = ((struct pollfds) { .poll = NULL,
	304	+ .size = 0,
	305	+ .used = 0 });
	306	+out_free:
308	307	kfree(p);
309		- out_close2:
	308	+ sigio_unlock();
	309	+out_close2:
310	310	close(l_sigio_private[0]);
311	311	close(l_sigio_private[1]);
312		- out_close1:
	312	+out_close1:
313	313	close(l_write_sigio_fds[0]);
314	314	close(l_write_sigio_fds[1]);
315		- return;
316	315	}
317	316
318	317	void sigio_cleanup(void)