[LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling

[LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling

[vkabatov]

From: Veronika Kabatova <vkabatov@redhat.com>

Very occasionally, we ran into unexpected failures with exit code 1
when running mtest06/mmap1. This was caused by a race condition during
unmap() - read with SIGSEGV - mmap() loop, where read after unmap was
attempted, but before the actual signal handling occurred the area was
mapped again and signal handler was changed back. Instead of test
continuation when a read after unmap happens, the handler which takes
SIGSEGV as fatal becomes active before it should and kills the test. See
simplified strace output:

[pid  8650] <... munmap resumed> ) ...
[pid  8651] --- SIGSEGV ...
[pid  8650] mmap ...
[pid  8651] write(1, "mmap1    0  TINFO :  created writing thread ...
[pid  8650] <... mmap resumed> ) ...
[pid  8651] <... write resumed> ) ...
[pid  8650] sched_yield ...
[pid  8651] exit_group(1 ...
[pid  8651] +++ exited with 1 +++

Fix the race condition by using a proper locking mechanism for thread
synchronization to prevent mapping changes before the caught signal is
actually handled, and simplify the test by using only a single handler.

Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
---
 testcases/kernel/mem/mtest06/mmap1.c | 86 +++++++++++++-----------------------
 1 file changed, 31 insertions(+), 55 deletions(-)

diff --git a/testcases/kernel/mem/mtest06/mmap1.c b/testcases/kernel/mem/mtest06/mmap1.c
index 8894b0dbf..cd41aebf0 100644
--- a/testcases/kernel/mem/mtest06/mmap1.c
+++ b/testcases/kernel/mem/mtest06/mmap1.c
@@ -7,6 +7,7 @@
 /* Copyright (c) 2007 <rsalveti@linux.vnet.ibm.com>                           */
 /* Copyright (c) 2007 Suzuki K P <suzuki@in.ibm.com>                          */
 /* Copyright (c) 2011 Cyril Hrubis <chrubis@suse.cz>                          */
+/* Copyright (c) 2017 Red Hat, Inc.                                           */
 /*									      */
 /* This program is free software;  you can redistribute it and/or modify      */
 /* it under the terms of the GNU General Public License as published by       */
@@ -65,8 +66,9 @@
 static int verbose_print = 0;
 static char *volatile map_address;
 static jmp_buf jmpbuf;
-static volatile char read_lock = 0;
+static sig_atomic_t volatile active_map;
 static void *distant_area;
+static pthread_mutex_t thread_lock = PTHREAD_MUTEX_INITIALIZER;
 
 char *TCID = "mmap1";
 int TST_TOTAL = 1;
@@ -78,6 +80,11 @@ static void sig_handler(int signal, siginfo_t * info, void *ut)
 		tst_resm(TPASS, "Test ended, success");
 		_exit(TPASS);
 	case SIGSEGV:
+		if (active_map) {
+			tst_resm(TINFO, "[%lu] Unexpected page fault at %p",
+				 pthread_self(), info->si_addr);
+			_exit(TFAIL);
+		}
 		longjmp(jmpbuf, 1);
 		break;
 	default:
@@ -86,27 +93,6 @@ static void sig_handler(int signal, siginfo_t * info, void *ut)
 	}
 }
 
-/*
- * Signal handler that is active, when file is mapped, eg. we do not expect
- * SIGSEGV to be delivered.
- */
-static void sig_handler_mapped(int signal, siginfo_t * info, void *ut)
-{
-	switch (signal) {
-	case SIGALRM:
-		tst_resm(TPASS, "Test ended, success");
-		_exit(TPASS);
-	case SIGSEGV:
-		tst_resm(TINFO, "[%lu] Unexpected page fault at %p",
-			 pthread_self(), info->si_addr);
-		_exit(TFAIL);
-		break;
-	default:
-		fprintf(stderr, "Unexpected signal - %d --- exiting\n", signal);
-		_exit(TBROK);
-	}
-}
-
 int mkfile(int size)
 {
 	char template[] = "/tmp/ashfileXXXXXX";
@@ -132,7 +118,6 @@ int mkfile(int size)
 
 void *map_write_unmap(void *ptr)
 {
-	struct sigaction sa;
 	long *args = ptr;
 	long i;
 	int j;
@@ -148,32 +133,28 @@ void *map_write_unmap(void *ptr)
 			 args[0], args[1], args[2]);
 
 	for (i = 0; i < args[2]; i++) {
+		pthread_mutex_lock(&thread_lock);
 		map_address = mmap(distant_area, (size_t) args[1],
 			PROT_WRITE | PROT_READ, MAP_SHARED, (int)args[0], 0);
 
 		if (map_address == (void *)-1) {
 			perror("map_write_unmap(): mmap()");
+			pthread_mutex_unlock(&thread_lock);
 			pthread_exit((void *)1);
 		}
-
-		while (read_lock)
-			sched_yield();
-
-		sigfillset(&sa.sa_mask);
-		sigdelset(&sa.sa_mask, SIGSEGV);
-		sa.sa_flags = SA_SIGINFO | SA_NODEFER;
-		sa.sa_sigaction = sig_handler_mapped;
-
-		if (sigaction(SIGSEGV, &sa, NULL)) {
-			perror("map_write_unmap(): sigaction()");
-			pthread_exit((void *)1);
-		}
+		active_map = 1;
+		pthread_mutex_unlock(&thread_lock);
 
 		if (verbose_print)
 			tst_resm(TINFO, "map address = %p", map_address);
 
-		for (j = 0; j < args[1]; j++) {
-			map_address[j] = 'a';
+		j = 0;
+		while (j < args[1]) {
+			if (!pthread_mutex_trylock(&thread_lock)) {
+				map_address[j] = 'a';
+				j++;
+				pthread_mutex_unlock(&thread_lock);
+			}
 			if (random() % 2)
 				sched_yield();
 		}
@@ -184,20 +165,14 @@ void *map_write_unmap(void *ptr)
 				 "map_write_unmap():memset() content of memory = %s",
 				 i, args[2], (char *)map_address);
 
-		sigfillset(&sa.sa_mask);
-		sigdelset(&sa.sa_mask, SIGSEGV);
-		sa.sa_flags = SA_SIGINFO | SA_NODEFER;
-		sa.sa_sigaction = sig_handler;
-
-		if (sigaction(SIGSEGV, &sa, NULL)) {
-			perror("map_write_unmap(): sigaction()");
-			pthread_exit((void *)1);
-		}
-
+		pthread_mutex_lock(&thread_lock);
+		active_map = 0;
 		if (munmap(map_address, (size_t) args[1]) == -1) {
 			perror("map_write_unmap(): mmap()");
+			pthread_mutex_unlock(&thread_lock);
 			pthread_exit((void *)1);
 		}
+		pthread_mutex_unlock(&thread_lock);
 	}
 
 	pthread_exit(NULL);
@@ -218,29 +193,30 @@ void *read_mem(void *ptr)
 			 "read from address %p", args[2], map_address);
 
 	for (i = 0; i < args[2]; i++) {
-
 		if (verbose_print)
 			tst_resm(TINFO, "read_mem() in while loop %ld times "
 				 "to go %ld times", i, args[2]);
 
 		if (setjmp(jmpbuf) == 1) {
-			read_lock = 0;
+			pthread_mutex_unlock(&thread_lock);
 			if (verbose_print)
 				tst_resm(TINFO, "page fault occurred due to "
 					 "a read after an unmap");
 		} else {
 			if (verbose_print) {
-				read_lock = 1;
+				pthread_mutex_lock(&thread_lock);
 				tst_resm(TINFO,
 					 "read_mem(): content of memory: %s",
 					 (char *)map_address);
-				read_lock = 0;
+				pthread_mutex_unlock(&thread_lock);
 			}
 			for (j = 0; j < args[1]; j++) {
-				read_lock = 1;
-				if (map_address[j] != 'a')
+				pthread_mutex_lock(&thread_lock);
+				if (map_address[j] != 'a') {
+					pthread_mutex_unlock(&thread_lock);
 					pthread_exit((void *)-1);
-				read_lock = 0;
+				}
+				pthread_mutex_unlock(&thread_lock);
 				if (random() % 2)
 					sched_yield();
 			}
-- 
2.13.6

[LTP] [PATCH 2/2] [mtest06] Rewrite mmap1 to use new library

[vkabatov]

From: Veronika Kabatova <vkabatov@redhat.com>

* Use new tst_test framework
* Use safe macros
* Simplify signal handler && don't exit from it
* Wait for threads to finish before exiting the test function

Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
---
 testcases/kernel/mem/mtest06/mmap1.c | 381 +++++++++++++----------------------
 1 file changed, 144 insertions(+), 237 deletions(-)

diff --git a/testcases/kernel/mem/mtest06/mmap1.c b/testcases/kernel/mem/mtest06/mmap1.c
index 38f9ad793..cf01a3487 100644
--- a/testcases/kernel/mem/mtest06/mmap1.c
+++ b/testcases/kernel/mem/mtest06/mmap1.c
@@ -20,8 +20,7 @@
 /* the GNU General Public License for more details.			      */
 /*									      */
 /* You should have received a copy of the GNU General Public License	      */
-/* along with this program;  if not, write to the Free Software		      */
-/* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA    */
+/* along with this program;  if not, see <http://www.gnu.org/licenses/>.      */
 /*									      */
 /******************************************************************************/
 /******************************************************************************/
@@ -36,82 +35,116 @@
 /*	        read must be a success between map and unmap of the region.   */
 /*									      */
 /******************************************************************************/
-#include <stdio.h>
-#include <sys/types.h>
-#include <sys/stat.h>
-#include <fcntl.h>
-#include <sched.h>
-#include <unistd.h>
-#include <errno.h>
-#include <sys/mman.h>
+#include <pthread.h>
 #include <sched.h>
-#include <stdlib.h>
-#include <signal.h>
-#include <sys/time.h>
-#include <sys/wait.h>
 #include <setjmp.h>
-#include <pthread.h>
-#include <signal.h>
-#include <string.h>
-#include "test.h"
-#include "safe_macros.h"
-
-#define DISTANT_MMAP_SIZE (64*1024*1024)
-#define OPT_MISSING(prog, opt) do { \
-	fprintf(stderr, "%s: option -%c ", prog, opt); \
-        fprintf(stderr, "requires an argument\n"); \
-	usage(prog); \
-} while (0)
-
-static int verbose_print = 0;
+#include <stdlib.h>
+#include "tst_safe_pthread.h"
+#include "tst_test.h"
+
+#define DISTANT_MMAP_SIZE (64 * 1024 * 1024)
+
+static long file_size = 1024;
+static long num_iter = 1000;
+static float exec_time = 24;
+
+static char *opt_verbose_print;
+static char *opt_file_size;
+static char *opt_num_iter;
+static char *opt_exec_time;
+
 static char *volatile map_address;
 static jmp_buf jmpbuf;
 static sig_atomic_t volatile active_map;
+static sig_atomic_t volatile test_end;
 static void *distant_area;
 static pthread_mutex_t thread_lock = PTHREAD_MUTEX_INITIALIZER;
 
-char *TCID = "mmap1";
-int TST_TOTAL = 1;
-
 static void sig_handler(int signal, siginfo_t * info, void *ut)
 {
 	switch (signal) {
-	case SIGALRM:
-		tst_resm(TPASS, "Test ended, success");
-		_exit(TPASS);
 	case SIGSEGV:
 		if (active_map) {
-			tst_resm(TINFO, "[%lu] Unexpected page fault at %p",
+			tst_res(TINFO, "[%lu] Unexpected page fault at %p",
 				 pthread_self(), info->si_addr);
-			_exit(TFAIL);
+			test_end = signal;
+			break;
 		}
 		longjmp(jmpbuf, 1);
 		break;
 	default:
-		fprintf(stderr, "Unexpected signal - %d --- exiting\n", signal);
-		_exit(TBROK);
+		test_end = signal;
+		break;
 	}
 }
 
-int mkfile(int size)
+static struct tst_option mmap1_options[] = {
+	{"l:", &opt_num_iter, "Number of mmap/write/unmap loops, default: 1000"},
+	{"s:", &opt_file_size, "Size of the file to be mapped, default: 1024 bytes"},
+	{"x:", &opt_exec_time, "Test execution time, default: 24 hours"},
+	{"v", &opt_verbose_print, "Verbose output, default: quiet"},
+	{NULL, NULL, NULL}
+};
+
+static void mmap1_setup(void)
 {
-	char template[] = "/tmp/ashfileXXXXXX";
-	int fd, i;
+	int i;
+	int siglist[] = {SIGSEGV, SIGALRM, -1};
+	struct sigaction sigptr;
 
-	if ((fd = mkstemp(template)) == -1)
-		tst_brkm(TBROK | TERRNO, NULL, "mkstemp() failed");
+	if (tst_parse_long(opt_file_size, &file_size, 1, LONG_MAX))
+		tst_brk(TBROK, "Invalid file size: %s", opt_file_size);
+	if (tst_parse_long(opt_num_iter, &num_iter, 1, LONG_MAX))
+		tst_brk(TBROK, "Invalid number of interations: %s",
+				opt_num_iter);
+	if (tst_parse_float(opt_exec_time, &exec_time, 0.0005, INT_MAX))
+		tst_brk(TBROK, "Invalid execution time: %s", opt_exec_time);
+
+	if (opt_verbose_print)
+		tst_res(TINFO, "Input parameters are: File size: %ld; "
+			 "Scheduled to run: %lf hours; "
+			 "Number of mmap/write/read: %ld",
+			 file_size, exec_time, num_iter);
+
+	tst_set_timeout(exec_time * 3600 + 300);
+
+	sigptr.sa_sigaction = sig_handler;
+	sigptr.sa_flags = SA_SIGINFO | SA_NODEFER;
+	sigemptyset(&sigptr.sa_mask);
 
-	unlink(template);
+	for (i = 0; siglist[i] != -1; i++) {
+		if (sigaction(siglist[i], &sigptr, NULL) == -1) {
+			tst_brk(TBROK | TERRNO, "could not set handler for %s",
+					tst_strsig(siglist[i]));
+		}
+	}
 
-	for (i = 0; i < size; i++)
-		if (write(fd, "a", 1) == -1)
-			tst_brkm(TBROK | TERRNO, NULL, "write() failed");
+	/* We don't want other mmap calls to map into same area as is
+	 * used for test (mmap_address). The test expects read to return
+	 * test pattern or read must fail with SIGSEGV. Find an area
+	 * that we can use, which is unlikely to be chosen for other
+	 * mmap calls. */
+	distant_area = SAFE_MMAP(NULL, DISTANT_MMAP_SIZE,
+			PROT_WRITE | PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE,
+			-1, 0);
+	SAFE_MUNMAP(distant_area, (size_t)DISTANT_MMAP_SIZE);
+	distant_area += DISTANT_MMAP_SIZE / 2;
+}
+
+static int mkfile(int size)
+{
+	char template[] = "ashfileXXXXXX";
+	int fd, i;
 
-	if (write(fd, "\0", 1) == -1)
-		tst_brkm(TBROK | TERRNO, NULL, "write() failed");
+	if ((fd = mkstemp(template)) == -1)
+		tst_brk(TBROK | TERRNO, "mkstemp() failed");
+	SAFE_UNLINK(template);
 
-	if (fsync(fd) == -1)
-		tst_brkm(TBROK | TERRNO, NULL, "fsync() failed");
+	for (i = 0; i < size; i++) {
+		SAFE_WRITE(1, fd, "a", 1);
+	}
+	SAFE_WRITE(1, fd, "\0", 1);
+	SAFE_FSYNC(fd);
 
 	return fd;
 }
@@ -119,14 +152,13 @@ int mkfile(int size)
 void *map_write_unmap(void *ptr)
 {
 	long *args = ptr;
-	long i;
-	int j;
+	long i, j;
 
-	tst_resm(TINFO, "[%lu] - map, change contents, unmap files %ld times",
+	tst_res(TINFO, "[%lu] - map, change contents, unmap files %ld times",
 		 pthread_self(), args[2]);
 
-	if (verbose_print)
-		tst_resm(TINFO, "map_write_unmap() arguments are: "
+	if (opt_verbose_print)
+		tst_res(TINFO, "map_write_unmap() arguments are: "
 			 "fd - arg[0]: %ld; "
 			 "size of file - arg[1]: %ld; "
 			 "num of map/write/unmap - arg[2]: %ld",
@@ -134,19 +166,14 @@ void *map_write_unmap(void *ptr)
 
 	for (i = 0; i < args[2]; i++) {
 		pthread_mutex_lock(&thread_lock);
-		map_address = mmap(distant_area, (size_t) args[1],
-			PROT_WRITE | PROT_READ, MAP_SHARED, (int)args[0], 0);
-
-		if (map_address == (void *)-1) {
-			perror("map_write_unmap(): mmap()");
-			pthread_mutex_unlock(&thread_lock);
-			pthread_exit((void *)1);
-		}
+		map_address = SAFE_MMAP(distant_area, (size_t)args[1],
+				PROT_WRITE | PROT_READ, MAP_SHARED,
+				(int)args[0], 0);
 		active_map = 1;
 		pthread_mutex_unlock(&thread_lock);
 
-		if (verbose_print)
-			tst_resm(TINFO, "map address = %p", map_address);
+		if (opt_verbose_print)
+			tst_res(TINFO, "map address = %p", map_address);
 
 		j = 0;
 		while (j < args[1]) {
@@ -159,19 +186,15 @@ void *map_write_unmap(void *ptr)
 				sched_yield();
 		}
 
-		if (verbose_print)
-			tst_resm(TINFO,
-				 "[%ld] times done: of total [%ld] iterations, "
-				 "map_write_unmap():memset() content of memory = %s",
-				 i, args[2], (char *)map_address);
+		if (opt_verbose_print)
+			tst_res(TINFO, "[%ld] times done: of total [%ld] "
+					"iterations, map_write_unmap(), "
+					"contents of memory: %s",
+					i, args[2], map_address);
 
 		pthread_mutex_lock(&thread_lock);
 		active_map = 0;
-		if (munmap(map_address, (size_t) args[1]) == -1) {
-			perror("map_write_unmap(): mmap()");
-			pthread_mutex_unlock(&thread_lock);
-			pthread_exit((void *)1);
-		}
+		SAFE_MUNMAP(map_address, (size_t) args[1]);
 		pthread_mutex_unlock(&thread_lock);
 	}
 
@@ -180,36 +203,35 @@ void *map_write_unmap(void *ptr)
 
 void *read_mem(void *ptr)
 {
-	long i;
 	long *args = ptr;
-	int j;
+	long i, j;
 
-	tst_resm(TINFO, "[%lu] - read contents of memory %p %ld times",
+	tst_res(TINFO, "[%lu] - read contents of memory %p %ld times",
 		 pthread_self(), map_address, args[2]);
 
-	if (verbose_print)
-		tst_resm(TINFO, "read_mem() arguments are: "
+	if (opt_verbose_print)
+		tst_res(TINFO, "read_mem() arguments are: "
 			 "number of reads to be performed - arg[2]: %ld; "
 			 "read from address %p", args[2], map_address);
 
 	for (i = 0; i < args[2]; i++) {
-		if (verbose_print)
-			tst_resm(TINFO, "read_mem() in while loop %ld times "
+		if (opt_verbose_print)
+			tst_res(TINFO, "read_mem() in while loop %ld times "
 				 "to go %ld times", i, args[2]);
 
 		if (setjmp(jmpbuf) == 1) {
 			pthread_mutex_unlock(&thread_lock);
-			if (verbose_print)
-				tst_resm(TINFO, "page fault occurred due to "
+			if (opt_verbose_print)
+				tst_res(TINFO, "page fault occurred due to "
 					 "a read after an unmap");
 		} else {
-			if (verbose_print) {
+			if (opt_verbose_print) {
 				pthread_mutex_lock(&thread_lock);
-				tst_resm(TINFO,
-					 "read_mem(): content of memory: %s",
-					 (char *)map_address);
+				tst_res(TINFO, "read_mem(): contents of "
+						"memory: %s", map_address);
 				pthread_mutex_unlock(&thread_lock);
 			}
+
 			for (j = 0; j < args[1]; j++) {
 				pthread_mutex_lock(&thread_lock);
 				if (map_address[j] != 'a') {
@@ -222,175 +244,60 @@ void *read_mem(void *ptr)
 			}
 		}
 	}
-
 	pthread_exit(NULL);
 }
 
-static void usage(char *progname)
-{
-	fprintf(stderr, "Usage: %s -d -l -s -v -x\n"
-		"\t -h help, usage message.\n"
-		"\t -l number of mmap/write/unmap     default: 1000\n"
-		"\t -s size of the file to be mmapped default: 1024 bytes\n"
-		"\t -v print more info.               default: quiet\n"
-		"\t -x test execution time            default: 24 Hrs\n",
-		progname);
-
-	exit(-1);
-}
-
-struct signal_info {
-	int signum;
-	char *signame;
-};
-
-static struct signal_info sig_info[] = {
-	{SIGHUP, "SIGHUP"},
-	{SIGINT, "SIGINT"},
-	{SIGQUIT, "SIGQUIT"},
-	{SIGABRT, "SIGABRT"},
-	{SIGBUS, "SIGBUS"},
-	{SIGSEGV, "SIGSEGV"},
-	{SIGALRM, "SIGALRM"},
-	{SIGUSR1, "SIGUSR1"},
-	{SIGUSR2, "SIGUSR2"},
-	{-1, "ENDSIG"}
-};
-
-int main(int argc, char **argv)
+static void test_mmap1(void)
 {
-	int c, i;
-	int file_size;
-	int num_iter;
-	double exec_time;
-	int fd;
+	int i, fd;
 	void *status;
 	pthread_t thid[2];
 	long chld_args[3];
-	extern char *optarg;
-	struct sigaction sigptr;
-	int ret;
-
-	/* set up the default values */
-	file_size = 1024;
-	num_iter = 1000;
-	exec_time = 24;
-
-	while ((c = getopt(argc, argv, "hvl:s:x:")) != -1) {
-		switch (c) {
-		case 'h':
-			usage(argv[0]);
-			break;
-		case 'l':
-			if ((num_iter = atoi(optarg)) == 0)
-				OPT_MISSING(argv[0], optopt);
-			else if (num_iter < 0)
-				printf
-				    ("WARNING: bad argument. Using default %d\n",
-				     (num_iter = 1000));
-			break;
-		case 's':
-			if ((file_size = atoi(optarg)) == 0)
-				OPT_MISSING(argv[0], optopt);
-			else if (file_size < 0)
-				printf
-				    ("WARNING: bad argument. Using default %d\n",
-				     (file_size = 1024));
-			break;
-		case 'v':
-			verbose_print = 1;
-			break;
-		case 'x':
-			exec_time = atof(optarg);
-			if (exec_time == 0)
-				OPT_MISSING(argv[0], optopt);
-			else if (exec_time < 0)
-				printf
-				    ("WARNING: bad argument. Using default %.0f\n",
-				     (exec_time = 24));
-			break;
-		default:
-			usage(argv[0]);
-			break;
-		}
-	}
-
-	/* We don't want other mmap calls to map into same area as is
-	 * used for test (mmap_address). The test expects read to return
-	 * test pattern or read must fail with SIGSEGV. Find an area
-	 * that we can use, which is unlikely to be chosen for other
-	 * mmap calls. */
-	distant_area = mmap(0, DISTANT_MMAP_SIZE, PROT_WRITE | PROT_READ,
-		MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
-	if (distant_area == (void *)-1)
-		tst_brkm(TBROK | TERRNO, NULL, "distant_area: mmap()");
-	SAFE_MUNMAP(NULL, distant_area, (size_t)DISTANT_MMAP_SIZE);
-	distant_area += DISTANT_MMAP_SIZE / 2;
-
-	if (verbose_print)
-		tst_resm(TINFO, "Input parameters are: File size:  %d; "
-			 "Scheduled to run:  %lf hours; "
-			 "Number of mmap/write/read:  %d",
-			 file_size, exec_time, num_iter);
 
+	test_end = 0;
 	alarm(exec_time * 3600);
 
-	/* Do not mask SIGSEGV, as we are interested in handling it. */
-	sigptr.sa_sigaction = sig_handler;
-	sigfillset(&sigptr.sa_mask);
-	sigdelset(&sigptr.sa_mask, SIGSEGV);
-	sigptr.sa_flags = SA_SIGINFO | SA_NODEFER;
-
-	for (i = 0; sig_info[i].signum != -1; i++) {
-		if (sigaction(sig_info[i].signum, &sigptr, NULL) == -1) {
-			perror("man(): sigaction()");
-			fprintf(stderr,
-				"could not set handler for %s, errno = %d\n",
-				sig_info[i].signame, errno);
-			exit(-1);
-		}
-	}
-
 	for (;;) {
 		if ((fd = mkfile(file_size)) == -1)
-			tst_brkm(TBROK, NULL,
-				 "main(): mkfile(): Failed to create temp file");
-
-		if (verbose_print)
-			tst_resm(TINFO, "Tmp file created");
+			tst_brk(TBROK, "main(): mkfile(): "
+					"Failed to create temp file");
+		if (opt_verbose_print)
+			tst_res(TINFO, "Tmp file created");
 
 		chld_args[0] = fd;
 		chld_args[1] = file_size;
 		chld_args[2] = num_iter;
 
-		if ((ret =
-		     pthread_create(&thid[0], NULL, map_write_unmap,
-				    chld_args)))
-			tst_brkm(TBROK, NULL, "main(): pthread_create(): %s",
-				 strerror(ret));
-
-		tst_resm(TINFO, "created writing thread[%lu]", thid[0]);
-
-		if ((ret = pthread_create(&thid[1], NULL, read_mem, chld_args)))
-			tst_brkm(TBROK, NULL, "main(): pthread_create(): %s",
-				 strerror(ret));
-
-		tst_resm(TINFO, "created reading thread[%lu]", thid[1]);
+		SAFE_PTHREAD_CREATE(&thid[0], NULL, map_write_unmap, chld_args);
+		tst_res(TINFO, "created writing thread[%lu]", thid[0]);
+		SAFE_PTHREAD_CREATE(&thid[1], NULL, read_mem, chld_args);
+		tst_res(TINFO, "created reading thread[%lu]", thid[1]);
 
 		for (i = 0; i < 2; i++) {
-			if ((ret = pthread_join(thid[i], &status)))
-				tst_brkm(TBROK, NULL,
-					 "main(): pthread_join(): %s",
-					 strerror(ret));
-
+			SAFE_PTHREAD_JOIN(thid[i], &status);
 			if (status)
-				tst_brkm(TFAIL, NULL,
-					 "thread [%lu] - process exited "
+				tst_res(TFAIL, "thread [%lu] - process exited "
 					 "with %ld", thid[i], (long)status);
 		}
-
-		close(fd);
+		SAFE_CLOSE(fd);
+
+		switch (test_end) {
+		case 0:
+			continue;
+		case SIGALRM:
+			tst_res(TPASS, "Test ended, success");
+			return;
+		default:
+			tst_res(TFAIL, "Test failed with unexpected signal %s",
+					tst_strsig(test_end));
+			return;
+		}
 	}
-
-	exit(0);
 }
+
+static struct tst_test test = {
+	.test_all = test_mmap1,
+	.setup = mmap1_setup,
+	.options = mmap1_options,
+	.needs_tmpdir = 1,
+};
-- 
2.13.6

[LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling

[Cyril Hrubis]

Hi!
> Very occasionally, we ran into unexpected failures with exit code 1
> when running mtest06/mmap1. This was caused by a race condition during
> unmap() - read with SIGSEGV - mmap() loop, where read after unmap was
> attempted, but before the actual signal handling occurred the area was
> mapped again and signal handler was changed back. Instead of test
> continuation when a read after unmap happens, the handler which takes
> SIGSEGV as fatal becomes active before it should and kills the test. See
> simplified strace output:
> 
> [pid  8650] <... munmap resumed> ) ...
> [pid  8651] --- SIGSEGV ...
> [pid  8650] mmap ...
> [pid  8651] write(1, "mmap1    0  TINFO :  created writing thread ...
> [pid  8650] <... mmap resumed> ) ...
> [pid  8651] <... write resumed> ) ...
> [pid  8650] sched_yield ...
> [pid  8651] exit_group(1 ...
> [pid  8651] +++ exited with 1 +++

So the problem is that we have SIGSEGV pending in the kernel while the
map_write_unmap() thread changes the signal handler to
sig_handler_mapped() right?

> Fix the race condition by using a proper locking mechanism for thread
> synchronization to prevent mapping changes before the caught signal is
> actually handled, and simplify the test by using only a single handler.

Hmm, how much is the reading loop slowed down with the mutex_lock() and
mutex_unlock() calls?

I would expect that it would make up for the significant part of the
loop which kind of defeats the purpose of stress test.

Maybe we can fix that by making sure that we got different address in
two subsequent mmaps in the map_write_unmap() loop (for instance by
unmaping the old one after we mapped a new one) then we can check the
si_addr in the siginfo_t in the signal handler. If the address is
different from the current one we know that the SEGFAULT has happened
before we mmaped() the file again. If that works we don't have to
implement any locking at all which would increase our chances of hitting
some race condition in the kernel...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling

[Jan Stancek]

----- Original Message -----
> Hi!
> > Very occasionally, we ran into unexpected failures with exit code 1
> > when running mtest06/mmap1. This was caused by a race condition during
> > unmap() - read with SIGSEGV - mmap() loop, where read after unmap was
> > attempted, but before the actual signal handling occurred the area was
> > mapped again and signal handler was changed back. Instead of test
> > continuation when a read after unmap happens, the handler which takes
> > SIGSEGV as fatal becomes active before it should and kills the test. See
> > simplified strace output:
> > 
> > [pid  8650] <... munmap resumed> ) ...
> > [pid  8651] --- SIGSEGV ...
> > [pid  8650] mmap ...
> > [pid  8651] write(1, "mmap1    0  TINFO :  created writing thread ...
> > [pid  8650] <... mmap resumed> ) ...
> > [pid  8651] <... write resumed> ) ...
> > [pid  8650] sched_yield ...
> > [pid  8651] exit_group(1 ...
> > [pid  8651] +++ exited with 1 +++
> 
> So the problem is that we have SIGSEGV pending in the kernel while the
> map_write_unmap() thread changes the signal handler to
> sig_handler_mapped() right?
> 
> > Fix the race condition by using a proper locking mechanism for thread
> > synchronization to prevent mapping changes before the caught signal is
> > actually handled, and simplify the test by using only a single handler.
> 
> Hmm, how much is the reading loop slowed down with the mutex_lock() and
> mutex_unlock() calls?
> 
> I would expect that it would make up for the significant part of the
> loop which kind of defeats the purpose of stress test.
> 

Hi,

> Maybe we can fix that by making sure that we got different address in
> two subsequent mmaps in the map_write_unmap() loop (for instance by
> unmaping the old one after we mapped a new one) then we can check the
> si_addr in the siginfo_t in the signal handler. If the address is
> different from the current one we know that the SEGFAULT has happened
> before we mmaped() the file again. If that works we don't have to
> implement any locking at all which would increase our chances of hitting
> some race condition in the kernel...

This doesn't capture the state of mapped area. When you're in signal
handler you wouldn't know if area was mapped or not when you got SIGSEGV.

I'd modify it in this way:
- use just one singal handler
- write thread will set 'mapped' flag when it maps/unmaps the area
- based on 'mapped' flag read thread tries to read from a specific range of addresses
  e.g. odd vs even bytes or odd vs. even blocks of bytes, such that
  those blocks don't overlap
- when signal handler triggers, it checks si_addr, and based on value
  it can tell if this was an attempt to read mapped or unmapped area
- if we tried to read mapped area and got SIGSEGV -> FAIL
  otherwise keep running

Regards,
Jan

> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
> 

[LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling

[Cyril Hrubis]

Hi!
> > Maybe we can fix that by making sure that we got different address in
> > two subsequent mmaps in the map_write_unmap() loop (for instance by
> > unmaping the old one after we mapped a new one) then we can check the
> > si_addr in the siginfo_t in the signal handler. If the address is
> > different from the current one we know that the SEGFAULT has happened
> > before we mmaped() the file again. If that works we don't have to
> > implement any locking at all which would increase our chances of hitting
> > some race condition in the kernel...
> 
> This doesn't capture the state of mapped area. When you're in signal
> handler you wouldn't know if area was mapped or not when you got SIGSEGV.
>
> I'd modify it in this way:
> - use just one singal handler
> - write thread will set 'mapped' flag when it maps/unmaps the area
> - based on 'mapped' flag read thread tries to read from a specific range of addresses
>   e.g. odd vs even bytes or odd vs. even blocks of bytes, such that
>   those blocks don't overlap
> - when signal handler triggers, it checks si_addr, and based on value
>   it can tell if this was an attempt to read mapped or unmapped area
> - if we tried to read mapped area and got SIGSEGV -> FAIL
>   otherwise keep running

I like the idea of encoding the information about the mapped/unmapped
state into the actuall address we attempt to read, nice trick, let's do
that.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling

[Veronika Kabatova]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: vkabatov@redhat.com, ltp@lists.linux.it
> Sent: Tuesday, November 7, 2017 11:25:37 AM
> Subject: Re: [LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling
> 
> Hi!
> > > Maybe we can fix that by making sure that we got different address in
> > > two subsequent mmaps in the map_write_unmap() loop (for instance by
> > > unmaping the old one after we mapped a new one) then we can check the
> > > si_addr in the siginfo_t in the signal handler. If the address is
> > > different from the current one we know that the SEGFAULT has happened
> > > before we mmaped() the file again. If that works we don't have to
> > > implement any locking at all which would increase our chances of hitting
> > > some race condition in the kernel...
> > 
> > This doesn't capture the state of mapped area. When you're in signal
> > handler you wouldn't know if area was mapped or not when you got SIGSEGV.
> >
> > I'd modify it in this way:
> > - use just one singal handler
> > - write thread will set 'mapped' flag when it maps/unmaps the area
> > - based on 'mapped' flag read thread tries to read from a specific range of
> > addresses
> >   e.g. odd vs even bytes or odd vs. even blocks of bytes, such that
> >   those blocks don't overlap
> > - when signal handler triggers, it checks si_addr, and based on value
> >   it can tell if this was an attempt to read mapped or unmapped area
> > - if we tried to read mapped area and got SIGSEGV -> FAIL
> >   otherwise keep running
> 
> I like the idea of encoding the information about the mapped/unmapped
> state into the actuall address we attempt to read, nice trick, let's do
> that.
> 

Hi, I finally got around to properly check this out.

While this is indeed a nice trick, it doesn't work well with s390x arch
where si_addr is masked and only shows which page the SIGSEGV happened
at, not the actual address. Since we can't mmap() to address that is
not a page boundary and (by default) we are mapping less than a page
worth of memory, no matter how we divide the addresses they would be
treated the same way.

To work around this feature we could use two pages and access one when
the area is mapped and the other one when it's not. This way we can
clearly distinguish between expected and unexpected segfaults on all
architectures.

This method would however deprecate the test option for file size (-s).
I'm not sure how widely this option is used but because of the possible
change of the interface, I'm writing this message to gather feedback
first.

I would also like to mention that we need to use locking at least when
the address computation and actual access to it is happening:

read thread                                          write thread
<compute and load address based on active mapping>
                                                     unmap()
<access address thinking it should be mapped>
<unexpected SIGSEGV>

There should be no issues with leaving out locking for the other parts
of code, but I found no feasible way to get around this one.

I'd be happy to hear any feedback or different ideas how to tackle both
issues if anybody has them.

Thanks,
Veronika Kabatova

> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling

[Jan Stancek]

----- Original Message -----
> 
> 
> ----- Original Message -----
> > From: "Cyril Hrubis" <chrubis@suse.cz>
> > To: "Jan Stancek" <jstancek@redhat.com>
> > Cc: vkabatov@redhat.com, ltp@lists.linux.it
> > Sent: Tuesday, November 7, 2017 11:25:37 AM
> > Subject: Re: [LTP] [PATCH 1/2] [mtest06] Fix race condition with signal
> > handling
> > 
> > Hi!
> > > > Maybe we can fix that by making sure that we got different address in
> > > > two subsequent mmaps in the map_write_unmap() loop (for instance by
> > > > unmaping the old one after we mapped a new one) then we can check the
> > > > si_addr in the siginfo_t in the signal handler. If the address is
> > > > different from the current one we know that the SEGFAULT has happened
> > > > before we mmaped() the file again. If that works we don't have to
> > > > implement any locking at all which would increase our chances of
> > > > hitting
> > > > some race condition in the kernel...
> > > 
> > > This doesn't capture the state of mapped area. When you're in signal
> > > handler you wouldn't know if area was mapped or not when you got SIGSEGV.
> > >
> > > I'd modify it in this way:
> > > - use just one singal handler
> > > - write thread will set 'mapped' flag when it maps/unmaps the area
> > > - based on 'mapped' flag read thread tries to read from a specific range
> > > of
> > > addresses
> > >   e.g. odd vs even bytes or odd vs. even blocks of bytes, such that
> > >   those blocks don't overlap
> > > - when signal handler triggers, it checks si_addr, and based on value
> > >   it can tell if this was an attempt to read mapped or unmapped area
> > > - if we tried to read mapped area and got SIGSEGV -> FAIL
> > >   otherwise keep running
> > 
> > I like the idea of encoding the information about the mapped/unmapped
> > state into the actuall address we attempt to read, nice trick, let's do
> > that.
> > 
> 
> Hi, I finally got around to properly check this out.
> 
> While this is indeed a nice trick, it doesn't work well with s390x arch
> where si_addr is masked and only shows which page the SIGSEGV happened
> at, not the actual address. Since we can't mmap() to address that is
> not a page boundary and (by default) we are mapping less than a page
> worth of memory, no matter how we divide the addresses they would be
> treated the same way.
> 
> To work around this feature we could use two pages and access one when
> the area is mapped and the other one when it's not. This way we can
> clearly distinguish between expected and unexpected segfaults on all
> architectures.
> 
> This method would however deprecate the test option for file size (-s).
> I'm not sure how widely this option is used but because of the possible
> change of the interface, I'm writing this message to gather feedback
> first.

Looking at runtest/ directory, it's not used. If we want to preserve it,
we could impose some minimal value (2 pages).

> 
> I would also like to mention that we need to use locking at least when
> the address computation and actual access to it is happening:
> 
> read thread                                          write thread
> <compute and load address based on active mapping>
>                                                      unmap()
> <access address thinking it should be mapped>
> <unexpected SIGSEGV>
> 
> There should be no issues with leaving out locking for the other parts
> of code, but I found no feasible way to get around this one.

The idea me and Veronika were dicussing off-list was using just
pthread_mutex_trylock() without actual blocking to avoid race above.

> 
> I'd be happy to hear any feedback or different ideas how to tackle both
> issues if anybody has them.

I don't see problem using 2 pages.
I'd drop '-s' parameter until there's need for it.
I'd try some minimal locking and measure how many iterations
are we able to do now vs. before.

---
Thinking loud about alternatives:

Signal handler could be replaced with write() to some temporary
file, which would use the ptr to mapped/unmapped area. If it's
not mapped we get EFAULT. This would also eliminate setjmp()/longjmp().

If we stop looking at mapped status and only consider mapped data,
then this would turn into checking written pattern:
1) create file A with some pattern
2) thread1 maps/unmaps file into memory at X
3) thread2 writes to file B, from memory X, this will either work or fail with EFAULT
4) report PASS if all blocks in file B have same pattern as blocks in file A

To address "read between mmap/munmap must succeed", there would probably also
need to be a write performed by thread1 to file C.

Regards,
Jan

[LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling

[Cyril Hrubis]

Hi!
> > I would also like to mention that we need to use locking at least when
> > the address computation and actual access to it is happening:
> > 
> > read thread                                          write thread
> > <compute and load address based on active mapping>
> >                                                      unmap()
> > <access address thinking it should be mapped>
> > <unexpected SIGSEGV>
> > 
> > There should be no issues with leaving out locking for the other parts
> > of code, but I found no feasible way to get around this one.
> 
> The idea me and Veronika were dicussing off-list was using just
> pthread_mutex_trylock() without actual blocking to avoid race above.
> 
> > 
> > I'd be happy to hear any feedback or different ideas how to tackle both
> > issues if anybody has them.
> 
> I don't see problem using 2 pages.
> I'd drop '-s' parameter until there's need for it.

Agreed.

> I'd try some minimal locking and measure how many iterations
> are we able to do now vs. before.

Sounds reasonable.

I guess that it's impossible to make it 100% correct without any
locking, so we should make it as lightweight as possible, maybe some
spinlocks atomic operations and sched_yield().

I was thinking of doing pthread_kill() to the reading thread with some
other signal that would flip a flag in the signal handler before we
unmap and after we map the memory, so we will use that signal as a some
kind of barrier. But this may be tricky to get working and quite
possibly will not work at all.

> ---
> Thinking loud about alternatives:
> 
> Signal handler could be replaced with write() to some temporary
> file, which would use the ptr to mapped/unmapped area. If it's
> not mapped we get EFAULT. This would also eliminate setjmp()/longjmp().

I wonder if we should actually test both, but yes handling EFAULT would
make this much easier than asynchronous signal delivery. On the other
hand the signal handler code is much more complex so there is probably
bigger room for triggering bugs.

> If we stop looking at mapped status and only consider mapped data,
> then this would turn into checking written pattern:
> 1) create file A with some pattern
> 2) thread1 maps/unmaps file into memory at X
> 3) thread2 writes to file B, from memory X, this will either work or fail with EFAULT
> 4) report PASS if all blocks in file B have same pattern as blocks in file A

And thread2 retries on EFAULT with the same address again, right? This
sounds interesting enough to be implemented as a test.

> To address "read between mmap/munmap must succeed", there would probably also
> need to be a write performed by thread1 to file C.

But that runs synchronously between the mmap() and unmap() right? That
kind of misses the point of the original test, however I wonder how
important it is anyway. If one thread maps/unmaps memory the other
threads has to be synchronized with mutexes anyway. So maybe it makes
sense to test for this assertion with heavy locking in a separate test.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] [mtest06] Fix race condition with signal handling

[Jan Stancek]

----- Original Message -----
> Hi!
> > > I would also like to mention that we need to use locking at least when
> > > the address computation and actual access to it is happening:
> > > 
> > > read thread                                          write thread
> > > <compute and load address based on active mapping>
> > >                                                      unmap()
> > > <access address thinking it should be mapped>
> > > <unexpected SIGSEGV>
> > > 
> > > There should be no issues with leaving out locking for the other parts
> > > of code, but I found no feasible way to get around this one.
> > 
> > The idea me and Veronika were dicussing off-list was using just
> > pthread_mutex_trylock() without actual blocking to avoid race above.
> > 
> > > 
> > > I'd be happy to hear any feedback or different ideas how to tackle both
> > > issues if anybody has them.
> > 
> > I don't see problem using 2 pages.
> > I'd drop '-s' parameter until there's need for it.
> 
> Agreed.
> 
> > I'd try some minimal locking and measure how many iterations
> > are we able to do now vs. before.
> 
> Sounds reasonable.
> 
> I guess that it's impossible to make it 100% correct without any
> locking, so we should make it as lightweight as possible, maybe some
> spinlocks atomic operations and sched_yield().
> 
> I was thinking of doing pthread_kill() to the reading thread with some
> other signal that would flip a flag in the signal handler before we
> unmap and after we map the memory, so we will use that signal as a some
> kind of barrier. But this may be tricky to get working and quite
> possibly will not work at all.
> 
> > ---
> > Thinking loud about alternatives:
> > 
> > Signal handler could be replaced with write() to some temporary
> > file, which would use the ptr to mapped/unmapped area. If it's
> > not mapped we get EFAULT. This would also eliminate setjmp()/longjmp().
> 
> I wonder if we should actually test both, but yes handling EFAULT would
> make this much easier than asynchronous signal delivery. On the other
> hand the signal handler code is much more complex so there is probably
> bigger room for triggering bugs.
> 
> > If we stop looking at mapped status and only consider mapped data,
> > then this would turn into checking written pattern:
> > 1) create file A with some pattern
> > 2) thread1 maps/unmaps file into memory at X
> > 3) thread2 writes to file B, from memory X, this will either work or fail
> > with EFAULT
> > 4) report PASS if all blocks in file B have same pattern as blocks in file
> > A
> 
> And thread2 retries on EFAULT with the same address again, right? This
> sounds interesting enough to be implemented as a test.

OK, I'll put it on my TODO list (as mtest08).

> 
> > To address "read between mmap/munmap must succeed", there would probably
> > also
> > need to be a write performed by thread1 to file C.
> 
> But that runs synchronously between the mmap() and unmap() right? That
> kind of misses the point of the original test, however I wonder how

It does. Likely better to have two tests, mtest06 with some light
locking, and mtest08 doing the EFAULT checks.

Regards,
Jan