pipeio: prevent race between SIGCHLD and open()

PATCH Superseded Jan Stancek 3 participants 3 messages Dec 14, 2011 – Dec 15, 2011

lore

Share & tools

lore

Jan Stancek

author

2011-12-14 11:06 UTC #0 lore raw

PATCH 1/1 pipeio: prevent race between SIGCHLD and open()

This test occasionally hangs on some machines. The hang has been observed on some single CPU ones.

pipeio code is using signal(2), setting by default SA_RESTART flag, which is also the case for SIGCHLD.

If last child manages to exit while parent is still at open(), parent gets SIGCHLD and open() is restarted. At this point test hangs.

Here's strace output from parent point of view:

=== snip ===

brk(0)                                  = 0x11bb000
brk(0x11dd000)                          = 0x11dd000
getpid()                                = 18826
stat("tpipe.18826", 0x7fff89e1d410)     = -1 ENOENT (No such file or

directory)
mknod("tpipe.18826", S_IFIFO|0777) = 0
rt_sigaction(SIGCHLD, {0x400a54, [CHLD], SA_RESTORER|SA_RESTART,
0x354aa32a20}, {SIG_DFL, [], 0}, 8) = 0
clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|
CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f53b9ddd9d0) = 18827
fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 3), ...}) = 0 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0)
= 0x7f53b9de5000
open("tpipe.18826", O_RDONLY
) = ? ERESTARTSYS (To be restarted)
--- SIGCHLD (Child exited) @ 0 (0) ---
wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 18827
rt_sigreturn(0xffffffffffffffff) = 2
open("tpipe.18826", O_RDONLY
=== /snip ===

This patch is introducing semaphore, which prevents children from exiting until parent completes open(). It also adds timed wait, so parent waits for children to exit before it deletes pipe and semaphore.

Signed-off-by Jan Stancek <[email protected]>

---
 testcases/kernel/ipc/pipeio/pipeio.c |   37 
++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 2 deletions(-)

testcases/kernel/ipc/pipeio/pipeio.c +35 -2

index 1c28c9b..451c094 100644
@@ -158,6 +158,8 @@ char *av[];
 	  struct semid_ds *buf;
 	  unsigned short int *array;
 	} u;
+	unsigned int uwait_iter = 1000;
+	unsigned int uwait_total = 5000000;
 
 	u.val = 0;
 	format = HEX;

@@ -443,13 +445,17 @@ char *av[];
 
 	writebuf[size-1] = 'A';	/* to detect partial read/write problem */
 
-	if ((sem_id = semget(IPC_PRIVATE, 1, IPC_CREAT|S_IRWXU)) == -1) {
+	if ((sem_id = semget(IPC_PRIVATE, 2, IPC_CREAT|S_IRWXU)) == -1) {
 		tst_brkm(TBROK, NULL, "Couldn't allocate semaphore: %s", strerror(errno));
 	}
 
 	if (semctl(sem_id, 0, SETVAL, u) == -1)
 		tst_brkm(TBROK, NULL, "Couldn't initialize semaphore value: %s", strerror(errno));
 
+	/* semaphore to hold off children from exiting until open() completes */
+	if (semctl(sem_id, 1, SETVAL, u) == -1)
+		tst_brkm(TBROK, NULL, "Couldn't initialize semaphore value: %s", strerror(errno));
+
 	if (background) {
 	    if ((n=fork()) == -1) {
 		tst_resm (TFAIL, "fork() failed: %s", strerror(errno));

@@ -586,6 +592,15 @@ printf("child after fork pid = %d\n", getpid());
 			}
 			fflush(stderr);
 		}
+
+		/* child waits until parent completes open() */
+		sem_op = (struct sembuf) {
+			.sem_num = 1,
+			.sem_op = -1,
+			.sem_flg = 0
+		};
+		if (semop(sem_id, &sem_op, 1) == -1)
+			tst_brkm(TBROK, NULL, "Couldn't lower the semaphore: %s", strerror(errno));
 	}
 	if (c > 0) {	/***** if parent *****/

@@ -602,6 +617,15 @@ printf("child after fork pid = %d\n", getpid());
 			close(write_fd);
 		}
 
+		/* raise semaphore so children can exit */
+		sem_op = (struct sembuf) {
+			.sem_num = 1,
+			.sem_op = num_wrters,
+			.sem_flg = 0
+		};
+		if (semop(sem_id, &sem_op, 1) == -1)
+			tst_brkm(TBROK, NULL, "Couldn't raise the semaphore: %s", strerror(errno));
+
 		sem_op = (struct sembuf) {
 			.sem_num = 0,
 			.sem_op = -num_wrters,

@@ -694,6 +718,15 @@ output:
 				tst_resm(TPASS, "1 PASS %d pipe reads complete, read size = %d, %s %s",
 				          count+1,size,pipe_type,blk_type);
 
+		/* wait for all children to finish, timeout after uwait_total
+		   semtimedop might not be available everywhere */
+		for (i=0; i<uwait_total; i+=uwait_iter) {
+			if (semctl(sem_id, 1, GETVAL) == 0) {
+				break;
+			}
+			usleep(uwait_iter);
+		}
+
 		semctl(sem_id, 0, IPC_RMID);
 
 		if (!unpipe)

@@ -884,4 +917,4 @@ sig_handler(int sig)
     fflush(stdout);
     exit(3);
 
-}
\ No newline at end of file
+}

------------------------------------------------------------------------------
Cloud Computing - Latest Buzzword or a Glimpse of the Future? This paper surveys cloud computing today: What are the benefits? Why are businesses embracing it? What are its payoffs and pitfalls?
http://www.accelacomm.com/jaw/sdnl/114/51425149/

_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Caspar Zhang

Reviewed-by on v1 1/1

2011-12-15 03:04 UTC #1 lore raw

Quoted 35 lines · Jan Stancek

This test occasionally hangs on some machines. The hang has been observed on some single CPU ones.
pipeio code is using signal(2), setting by default SA_RESTART flag, which is also the case for SIGCHLD.
If last child manages to exit while parent is still at open(), parent gets SIGCHLD and open() is restarted. At this point test hangs.
Here's strace output from parent point of view:
=== snip ===
brk(0)                                  = 0x11bb000
brk(0x11dd000)                          = 0x11dd000
getpid()                                = 18826
stat("tpipe.18826", 0x7fff89e1d410)     = -1 ENOENT (No such file or
directory)
mknod("tpipe.18826", S_IFIFO|0777) = 0
rt_sigaction(SIGCHLD, {0x400a54, [CHLD], SA_RESTORER|SA_RESTART,
0x354aa32a20}, {SIG_DFL, [], 0}, 8) = 0
clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|
CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f53b9ddd9d0) = 18827
fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 3), ...}) = 0 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0)
= 0x7f53b9de5000
open("tpipe.18826", O_RDONLY
) = ? ERESTARTSYS (To be restarted)
--- SIGCHLD (Child exited) @ 0 (0) ---
wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 18827
rt_sigreturn(0xffffffffffffffff) = 2
open("tpipe.18826", O_RDONLY
=== /snip ===

Quoted 6 lines

This patch is introducing semaphore, which prevents children from exiting until parent completes open(). It also adds timed wait, so parent waits for children to exit before it deletes pipe and semaphore.
Signed-off-by Jan Stancek <[email protected]>

looks good.

Reviewed-by Caspar Zhang <[email protected]>

Quoted 5 lines

---
  testcases/kernel/ipc/pipeio/pipeio.c |   37 
++++++++++++++++++++++++++++++++-
  1 files changed, 35 insertions(+), 2 deletions(-)

------------------------------------------------------------------------------
10 Tips for Better Server Consolidation
Server virtualization is being driven by many needs. But none more important than the need to reduce IT complexity while improving strategic productivity. Learn More!
http://www.accelacomm.com/jaw/sdnl/114/51507609/
_______________________________________________
Ltp-list mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Garrett Cooper

2011-12-15 05:46 UTC #2 lore raw

In reply to Jan Stancek

Quoted 40 lines · Jan Stancek

This test occasionally hangs on some machines. The hang has been observed on some single CPU ones.
pipeio code is using signal(2), setting by default SA_RESTART flag, which is also the case for SIGCHLD.
If last child manages to exit while parent is still at open(), parent gets SIGCHLD and open() is restarted. At this point test hangs.
Here's strace output from parent point of view:
=== snip ===
brk(0) = 0x11bb000 brk(0x11dd000) = 0x11dd000
getpid() = 18826 stat("tpipe.18826", 0x7fff89e1d410) = -1 ENOENT (No such file or directory)
mknod("tpipe.18826", S_IFIFO|0777) = 0 rt_sigaction(SIGCHLD, {0x400a54, [CHLD], SA_RESTORER|SA_RESTART, 0x354aa32a20}, {SIG_DFL, [], 0}, 8) = 0 clone(child_stack=0, flags=CLONE_CHILD_CLEARTID| CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f53b9ddd9d0) = 18827 fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 3), ...}) = 0 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f53b9de5000
open("tpipe.18826", O_RDONLY
) = ? ERESTARTSYS (To be restarted)
--- SIGCHLD (Child exited) @ 0 (0) ---
wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 18827 rt_sigreturn(0xffffffffffffffff) = 2 open("tpipe.18826", O_RDONLY
=== /snip ===
This patch is introducing semaphore, which prevents children from exiting until parent completes open(). It also adds timed wait, so parent waits for children to exit before it deletes pipe and semaphore.

Quoted 5 lines

Signed-off-by Jan Stancek <[email protected]>
---
testcases/kernel/ipc/pipeio/pipeio.c | 37
++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 2 deletions(-)

Please use:

tst_brkm(TBROK|TERRNO,

instead of:

tst_brkm(TBROK, "... : %s", strerror(errno));

Thanks!
-Garrett