[PATCH 0/2] drmtest: Fix hanging child processes

[PATCH 0/2] drmtest: Fix hanging child processes

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

It was observed both on Linux and Android that tests which
fork can sometimes hang failing to terminate child
processes.

This was eventually tracked down to race conditions in C
library implementations, all with regards to caching of
PID/TGID and TID values.

Solution is to revert the previous workaround, which closed
the race under glibc a bit, and ensure current values from
the kernel are used during timing sensitive periods.

This was tested under Android only but should be a generic fix.

Tvrtko Ursulin (2):
  Revert "lib/drmtest: ducttape over fork race"
  drmtest: Avoid wrong PID/TID after clone races

 lib/drmtest.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

-- 
1.8.4.3

[PATCH 1/2] Revert "lib/drmtest: ducttape over fork race"

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This reverts commit a031a1bf93b828585e7147f06145fc5030814547.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Conflicts:
	lib/drmtest.c
---
 lib/drmtest.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 94fa686..dcacd3b 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -1127,7 +1127,6 @@ static void fork_helper_exit_handler(int sig)
 bool __igt_fork_helper(struct igt_helper_process *proc)
 {
 	pid_t pid;
-	sighandler_t oldsig;
 	int id;
 
 	assert(!proc->running);
@@ -1138,13 +1137,6 @@ bool __igt_fork_helper(struct igt_helper_process *proc)
 
 	igt_install_exit_handler(fork_helper_exit_handler);
 
-	/*
-	 * XXX: There's a race between fork and the subsequent kill in
-	 * igt_stop_signal_helper if we don't ovewrite the SIGQUIT handler. Note
-	 * that inserting sufficient amounts of printf or other delays makes
-	 * this unnecessary.
-	 */
-	oldsig = signal(SIGQUIT, SIG_DFL);
 	switch (pid = fork()) {
 	case -1:
 		igt_assert(0);
@@ -1155,8 +1147,6 @@ bool __igt_fork_helper(struct igt_helper_process *proc)
 
 		return true;
 	default:
-		signal(SIGQUIT, oldsig);
-
 		proc->running = true;
 		proc->pid = pid;
 		proc->id = id;
-- 
1.8.4.3

[PATCH 2/2] drmtest: Avoid wrong PID/TID after clone races

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Various C library implementations have various races with regards
to caching getpid() or TID inside pthread_kill() implementations.

For example see clone(2) glibc man page and pthread_kill
Bionic C library source.

Work around that by making sure correct PID/TGID and TID values
are retrieved from the kernel when re-raising the signal. It
can be delivered immediately after the clone system call while C
library cached copies have not yet been updated.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/drmtest.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index dcacd3b..139eb82 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -43,6 +43,8 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <sys/wait.h>
+#include <sys/types.h>
+#include <sys/syscall.h>
 
 #include "drmtest.h"
 #include "i915_drm.h"
@@ -1473,6 +1475,8 @@ static void igt_atexit_handler(void)
 
 static void fatal_sig_handler(int sig)
 {
+	pid_t pid, tid;
+
 	restore_all_sig_handler();
 
 	/*
@@ -1481,7 +1485,11 @@ static void fatal_sig_handler(int sig)
 	 */
 	call_exit_handlers(sig);
 
-	raise(sig);
+	/* Workaround cached PID and TID races on glibc and Bionic libc. */
+	pid = syscall(SYS_getpid);
+	tid = syscall(SYS_gettid);
+
+	syscall(SYS_tgkill, pid, tid, sig);
 }
 
 /*
-- 
1.8.4.3

Re: [PATCH 0/2] drmtest: Fix hanging child processes

[Daniel Vetter]

On Tue, Dec 03, 2013 at 04:44:53PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> It was observed both on Linux and Android that tests which
> fork can sometimes hang failing to terminate child
> processes.
> 
> This was eventually tracked down to race conditions in C
> library implementations, all with regards to caching of
> PID/TGID and TID values.
> 
> Solution is to revert the previous workaround, which closed
> the race under glibc a bit, and ensure current values from
> the kernel are used during timing sensitive periods.
> 
> This was tested under Android only but should be a generic fix.
> 
> Tvrtko Ursulin (2):
>   Revert "lib/drmtest: ducttape over fork race"
>   drmtest: Avoid wrong PID/TID after clone races

Both merged, thanks for the patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch