Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 19 Sep 2001 08:58:52 +1000
From:      Peter Jeremy <peter.jeremy@alcatel.com.au>
To:        gnats-admin@FreeBSD.org, freebsd-bugs@FreeBSD.org
Cc:        Michael Reifenberger <root@nihil.plaut.de>
Subject:   Re: kern/12014: Fix SysV Semaphore handling
Message-ID:  <20010919085852.A4912@gsmx07.alcatel.com.au>
In-Reply-To: <199906032120.OAA40933@freefall.freebsd.org>; from gnats-admin@FreeBSD.org on Fri, Jun 04, 1999 at 07:20:02AM %2B1000
References:  <"99Jun4.065824est.40321"@border.alcanet.com.au> <199906032120.OAA40933@freefall.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
Updated patches following KSE and Giant pushdown.  Note that whilst
these patches compile, I haven't tested them.  As a side-effect they
also reduce the stack frame size in semop()

Index: src/lib/libc/sys/semctl.2
===================================================================
RCS file: /home/CVSROOT/src/lib/libc/sys/semctl.2,v
retrieving revision 1.13
diff -u -r1.13 semctl.2
--- src/lib/libc/sys/semctl.2	2001/07/15 07:53:16	1.13
+++ src/lib/libc/sys/semctl.2	2001/07/21 15:00:04
@@ -107,6 +107,8 @@
 .Fa semnum
 to
 .Fa arg.val .
+Outstanding adjust on exit values for this semaphore in any process
+are cleared.
 .It Dv GETPID
 Return the pid of the last process to perform an operation on
 semaphore number
@@ -127,6 +129,8 @@
 Set the values of all of the semaphores in the set to the values
 in the array pointed to by
 .Fa arg.array .
+Outstanding adjust on exit values for all semaphores in this set,
+in any process are cleared.
 .El
 .Pp
 The
@@ -152,7 +156,10 @@
 .Sh RETURN VALUES
 On success, when
 .Fa cmd
-is one of GETVAL, GETNCNT, or GETZCNT,
+is one of
+.Dv GETVAL , GETNCNT
+or
+.Dv GETZCNT ,
 .Fn semctl
 returns the corresponding value; otherwise, 0 is returned.
 On failure, -1 is returned, and
@@ -174,7 +181,16 @@
 .It Bq Er EACCES
 Permission denied due to mismatch between operation and mode of
 semaphore set.
+.It Bq Er ERANGE
+.Dv SETVAL
+or
+.Dv SETALL
+attempted to set a semaphore outside the allowable range
+.Bq 0 .. Dv SEMVMX .
 .El
 .Sh SEE ALSO
 .Xr semget 2 ,
 .Xr semop 2
+.Sh BUGS
+.Dv SETALL
+may update some semaphore elements before returning an error.
Index: src/lib/libc/sys/semop.2
===================================================================
RCS file: /home/CVSROOT/src/lib/libc/sys/semop.2,v
retrieving revision 1.13
diff -u -r1.13 semop.2
--- src/lib/libc/sys/semop.2	2001/08/09 13:32:12	1.13
+++ src/lib/libc/sys/semop.2	2001/08/10 14:49:18
@@ -70,7 +70,11 @@
 .Fa sem_flg
 determine an operation to be performed on semaphore number
 .Fa sem_num
-in the set.  The values SEM_UNDO and IPC_NOWAIT may be
+in the set.  The values
+.Dv SEM_UNDO
+and
+.Dv IPC_NOWAIT
+may be
 .Em OR Ns 'ed
 into the
 .Fa sem_flg
@@ -80,16 +84,19 @@
 .Fa sem_op :
 .\"
 .\" This section is based on the description of semop() in
-.\" Stevens, _Advanced Programming in the UNIX Environment_.
+.\" Stevens, _Advanced Programming in the UNIX Environment_,
+.\" and the semop(2) description in The Open Group Unix2 specification.
 .\"
 .Bl -bullet
 .It
 When
 .Fa sem_op
-is positive, the semaphore's value is incremented by
+is positive and the process has alter permission,
+the semaphore's value is incremented by
 .Fa sem_op Ns 's
-value.  If SEM_UNDO is specified, the semaphore's adjust on exit
-value is decremented by
+value.  If
+.Dv SEM_UNDO
+is specified, the semaphore's adjust on exit value is decremented by
 .Fa sem_op Ns 's
 value.  A positive value for
 .Fa sem_op
@@ -98,7 +105,8 @@
 .It
 The behavior when
 .Fa sem_op
-is negative depends on the current value of the semaphore:
+is negative and the process has alter permission,
+depends on the current value of the semaphore:
 .Bl -bullet
 .It
 If the current value of the semaphore is greater than or equal to
@@ -106,39 +114,54 @@
 .Fa sem_op ,
 then the value is decremented by the absolute value of
 .Fa sem_op .
-If SEM_UNDO is specified, the semaphore's adjust on exit
+If
+.Dv SEM_UNDO
+is specified, the semaphore's adjust on exit
 value is incremented by the absolute value of
 .Fa sem_op .
 .It
-If the current value of the semaphore is less than
-.Fa sem_op Ns 's
-value, one of the following happens:
+If the current value of the semaphore is less than the absolute value of
+.Fa sem_op ,
+one of the following happens:
 .\" XXX a *second* sublist?
 .Bl -bullet
 .It
-If IPC_NOWAIT was specified, then
+If
+.Dv IPC_NOWAIT
+was specified, then
 .Fn semop
 returns immediately with a return value of
 .Er EAGAIN .
+.It
+Otherwise, the calling process is put to sleep until one of the following
+conditions is satisfied:
+.\" XXX We already have two sublists, why not a third?
+.Bl -bullet
 .It
-If some other process has removed the semaphore with the IPC_RMID
+Some other process removes the semaphore with the
+.Dv IPC_RMID
 option of
-.Fn semctl ,
-then
+.Fn semctl .
+In this case,
 .Fn semop
 returns immediately with a return value of
-.Er EINVAL .
+.Er EIDRM .
+.It
+The process receives a signal that is to be caught.
+In this case, the process will resume execution as defined by
+.Fn sigaction .
 .It
-Otherwise, the calling process is put to sleep until the semaphore's
+The semaphore's
 value is greater than or equal to the absolute value of
 .Fa sem_op .
 When this condition becomes true, the semaphore's value is decremented
 by the absolute value of
 .Fa sem_op ,
-and the semaphore's adjust on exit value is incremented by the
+the semaphore's adjust on exit value is incremented by the
 absolute value of
 .Fa sem_op .
 .El
+.El
 .Pp
 A negative value for
 .Fa sem_op
@@ -149,11 +172,42 @@
 .It
 When
 .Fa sem_op
-is zero, the process waits for the semaphore's value to become zero.
-If it is already zero, the call to
+is zero and the process has read permission,
+one of the following will occur:
+.Bl -bullet
+.It
+If the current value of the semaphore is equal to zero
+then
+.Fn semop
+can return immediately.
+.It
+If
+.Dv IPC_NOWAIT
+was specified, then
+.Fn semop
+returns immediately with a return value of
+.Er EAGAIN .
+.It
+Otherwise, the calling process is put to sleep until one of the following
+conditions is satisfied:
+.\" XXX Another nested sublists
+.Bl -bullet
+.It
+Some other process removes the semaphore with the
+.Dv IPC_RMID
+option of
+.Fn semctl .
+In this case,
 .Fn semop
-can return immediately.  Otherwise, the calling process is put to
-sleep until the semaphore's value becomes zero.
+returns immediately with a return value of
+.Er EIDRM .
+.It
+The process receives a signal that is to be caught.
+In this case, the process will resume execution as defined by
+.Fn sigaction
+.It
+The semaphore's value becomes zero.
+.El
 .El
 .Pp
 For each semaphore a process has in use, the kernel maintains an
@@ -170,16 +224,20 @@
 .Bl -tag -width Er
 .It Bq Er EINVAL
 No semaphore set corresponds to
-.Fa semid .
+.Fa semid ,
+or the process would exceed the system-defined limit for the number of
+per-process SEM_UNDO structures.
 .It Bq Er EACCES
 Permission denied due to mismatch between operation and mode of
 semaphore set.
 .It Bq Er EAGAIN
-The semaphore's value was less than
-.Fa sem_op ,
-and IPC_NOWAIT was specified.
+The semaphore's value would have resulted in the process being put to sleep
+and
+.Dv IPC_NOWAIT
+was specified.
 .It Bq Er E2BIG
 Too many operations were specified.
+.Bq Dv SEMOPM
 .It Bq Er EFBIG
 .\"
 .\" I'd have thought this would be EINVAL, but the source says
@@ -187,7 +245,30 @@
 .\"
 .Fa sem_num
 was not in the range of valid semaphores for the set.
+.It Bq Er EIDRM
+The semaphore set was removed from the system.
+.It Bq Er EINTR
+The
+.Fn semop
+call was interrupted by a signal.
+.It Bq Er ENOSPC
+The system SEM_UNDO pool
+.Bq Dv SEMMNU
+is full.
+.It Bq Er ERANGE
+The requested operation would cause either
+the semaphore's current value
+.Bq Dv SEMVMX
+or its adjust on exit value
+.Bq Dv SEMAEM
+to exceed the system-imposed limits.
 .El
 .Sh SEE ALSO
 .Xr semctl 2 ,
-.Xr semget 2
+.Xr semget 2 ,
+.Xr sigaction 2
+.Sh BUGS
+.Fn Semop
+may block waiting for memory even if
+.Dv IPC_NOWAIT
+is specified.
Index: src/sys/kern/sysv_sem.c
===================================================================
RCS file: /home/CVSROOT/src/sys/kern/sysv_sem.c,v
retrieving revision 1.39
diff -u -r1.39 sysv_sem.c
--- src/sys/kern/sysv_sem.c	2001/09/13 21:06:18	1.39
+++ src/sys/kern/sysv_sem.c	2001/09/17 21:19:01
@@ -406,10 +406,12 @@
 	for (i = 0; i < suptr->un_cnt; i++, sunptr++) {
 		if (sunptr->un_id != semid || sunptr->un_num != semnum)
 			continue;
-		if (adjval == 0)
-			sunptr->un_adjval = 0;
-		else
-			sunptr->un_adjval += adjval;
+		if (adjval != 0) {
+			adjval += sunptr->un_adjval;
+			if (adjval > seminfo.semaem || adjval < -seminfo.semaem)
+				return (ERANGE);
+		}
+		sunptr->un_adjval = adjval;
 		if (sunptr->un_adjval == 0) {
 			suptr->un_cnt--;
 			if (i < suptr->un_cnt)
@@ -422,6 +424,8 @@
 	/* Didn't find the right entry - create it */
 	if (adjval == 0)
 		return(0);
+	if (adjval > seminfo.semaem || adjval < -seminfo.semaem)
+		return (ERANGE);
 	if (suptr->un_cnt != seminfo.semume) {
 		sunptr = &suptr->un_ent[suptr->un_cnt];
 		suptr->un_cnt++;
@@ -489,6 +493,7 @@
 	int i, rval, error;
 	struct semid_ds sbuf;
 	register struct semid_ds *semaptr;
+	u_short usval;
 
 #ifdef SEM_DEBUG
 	printf("call to semctl(%d, %d, %d, 0x%x)\n", semid, semnum, cmd, arg);
@@ -640,6 +645,10 @@
 		}
 		if ((error = copyin(arg, &real_arg, sizeof(real_arg))) != 0)
 			goto done2;
+		if (real_arg.val < 0 || real_arg.val > seminfo.semvmx) {
+			error = ERANGE;
+			goto done2;
+		}
 		semaptr->sem_base[semnum].semval = real_arg.val;
 		semundo_clear(semid, semnum);
 		wakeup((caddr_t)semaptr);
@@ -652,10 +661,14 @@
 			goto done2;
 		for (i = 0; i < semaptr->sem_nsems; i++) {
 			error = copyin(&real_arg.array[i],
-			    (caddr_t)&semaptr->sem_base[i].semval,
-			    sizeof(real_arg.array[0]));
+			    (caddr_t)&usval, sizeof(real_arg.array[0]));
 			if (error != 0)
 				break;
+			if (usval > seminfo.semvmx) {
+				error = ERANGE;
+				break;
+			}
+			semaptr->sem_base[i].semval = usval;
 		}
 		semundo_clear(semid, -1);
 		wakeup((caddr_t)semaptr);
@@ -822,12 +835,12 @@
 {
 	int semid = uap->semid;
 	u_int nsops = uap->nsops;
-	struct sembuf sops[MAX_SOPS];
+	struct sembuf *sops = NULL;
 	register struct semid_ds *semaptr;
 	register struct sembuf *sopptr;
 	register struct sem *semptr;
-	struct sem_undo *suptr = NULL;
-	int i, j, error = 0;
+	struct sem_undo *suptr;
+	int i, j, error;
 	int do_wakeup, do_undos;
 
 #ifdef SEM_DEBUG
@@ -856,26 +869,49 @@
 		error = EINVAL;
 		goto done2;
 	}
-
-	if ((error = ipcperm(td, &semaptr->sem_perm, IPC_W))) {
+	if (nsops > seminfo.semopm) {
 #ifdef SEM_DEBUG
-		printf("error = %d from ipcperm\n", error);
+		printf("too many sops (max=%d, nsops=%d)\n", seminfo.semopm,
+		    nsops);
 #endif
+		error = E2BIG;
 		goto done2;
 	}
 
-	if (nsops > MAX_SOPS) {
+	/* Allocate memory for sem_ops */
+	sops = malloc(nsops * sizeof(sops[0]), M_SEM, M_WAITOK);
+	if (!sops)
+		panic("Failed to allocate %d sem_ops", nsops);
+
+	if ((error = copyin(uap->sops, sops, nsops * sizeof(sops[0]))) != 0) {
 #ifdef SEM_DEBUG
-		printf("too many sops (max=%d, nsops=%u)\n", MAX_SOPS, nsops);
+		printf("error = %d from copyin(%08x, %08x, %d)\n", error,
+		    uap->sops, sops, nsops * sizeof(sops[0]));
 #endif
-		error = E2BIG;
 		goto done2;
 	}
 
-	if ((error = copyin(uap->sops, &sops, nsops * sizeof(sops[0]))) != 0) {
+	/*
+	 * Initial pass thru sops to see what permissions are needed.
+	 * Also perform any checks that don't need repeating on each
+	 * attempt to satisfy the request vector.
+	 */
+	j = 0;		/* permission needed */
+	do_undos = 0;
+	for (i = 0; i < nsops; i++) {
+		sopptr = &sops[i];
+		if (sopptr->sem_num >= semaptr->sem_nsems) {
+			error = EFBIG;
+			goto done2;
+		}
+		if (sopptr->sem_flg & SEM_UNDO && sopptr->sem_op != 0)
+			do_undos = 1;
+		j |= (sopptr->sem_op == 0) ? SEM_R : SEM_A;
+	}
+
+	if ((error = ipcperm(td, &semaptr->sem_perm, j))) {
 #ifdef SEM_DEBUG
-		printf("error = %d from copyin(%08x, %08x, %u)\n", error,
-		    uap->sops, &sops, nsops * sizeof(sops[0]));
+		printf("error = %d from ipaccess\n", error);
 #endif
 		goto done2;
 	}
@@ -889,19 +925,12 @@
 	 * This ensures that from the perspective of other tasks, a set
 	 * of requests is atomic (never partially satisfied).
 	 */
-	do_undos = 0;
-
 	for (;;) {
 		do_wakeup = 0;
+		error = 0;	/* error return if necessary */
 
 		for (i = 0; i < nsops; i++) {
 			sopptr = &sops[i];
-
-			if (sopptr->sem_num >= semaptr->sem_nsems) {
-				error = EFBIG;
-				goto done2;
-			}
-
 			semptr = &semaptr->sem_base[sopptr->sem_num];
 
 #ifdef SEM_DEBUG
@@ -923,21 +952,21 @@
 					    semptr->semzcnt > 0)
 						do_wakeup = 1;
 				}
-				if (sopptr->sem_flg & SEM_UNDO)
-					do_undos = 1;
 			} else if (sopptr->sem_op == 0) {
-				if (semptr->semval > 0) {
+				if (semptr->semval != 0) {
 #ifdef SEM_DEBUG
 					printf("semop:  not zero now\n");
 #endif
 					break;
 				}
+			} else if (semptr->semval + sopptr->sem_op >
+			    seminfo.semvmx) {
+				error = ERANGE;
+				break;
 			} else {
 				if (semptr->semncnt > 0)
 					do_wakeup = 1;
 				semptr->semval += sopptr->sem_op;
-				if (sopptr->sem_flg & SEM_UNDO)
-					do_undos = 1;
 			}
 		}
 
@@ -957,6 +986,10 @@
 			semaptr->sem_base[sops[j].sem_num].semval -=
 			    sops[j].sem_op;
 
+		/* If we detected an error, return it */
+		if (error != 0)
+			goto done2;
+
 		/*
 		 * If the request that we couldn't satisfy has the
 		 * NOWAIT flag set then return with EAGAIN.
@@ -980,8 +1013,6 @@
 		printf("semop:  good morning (error=%d)!\n", error);
 #endif
 
-		suptr = NULL;	/* sem_undo may have been reallocated */
-
 		if (error != 0) {
 			error = EINTR;
 			goto done2;
@@ -1014,6 +1045,7 @@
 	 * Process any SEM_UNDO requests.
 	 */
 	if (do_undos) {
+		suptr = NULL;
 		for (i = 0; i < nsops; i++) {
 			/*
 			 * We only need to deal with SEM_UNDO's for non-zero
@@ -1062,14 +1094,18 @@
 		} /* loop through the sops */
 	} /* if (do_undos) */
 
-	/* We're definitely done - set the sempid's */
+	/* We're definitely done - set the sempid's and time */
 	for (i = 0; i < nsops; i++) {
 		sopptr = &sops[i];
 		semptr = &semaptr->sem_base[sopptr->sem_num];
 		semptr->sempid = td->td_proc->p_pid;
 	}
+	semaptr->sem_otime = time_second;
 
-	/* Do a wakeup if any semaphore was up'd. */
+	/*
+	 * Do a wakeup if any semaphore was up'd whilst something was
+	 * sleeping on it.
+	 */
 	if (do_wakeup) {
 #ifdef SEM_DEBUG
 		printf("semop:  doing wakeup\n");
@@ -1084,6 +1120,8 @@
 #endif
 	td->td_retval[0] = 0;
 done2:
+	if (sops)
+	    free(sops, M_SEM);
 	mtx_unlock(&Giant);
 	return (error);
 }
@@ -1098,9 +1136,6 @@
 {
 	register struct sem_undo *suptr;
 	register struct sem_undo **supptr;
-	int did_something;
-
-	did_something = 0;
 
 	/*
 	 * Go through the chain of undo vectors looking for one
Index: src/sys/sys/sem.h
===================================================================
RCS file: /home/CVSROOT/src/sys/sys/sem.h,v
retrieving revision 1.23
diff -u -r1.23 sem.h
--- src/sys/sys/sem.h	2001/09/13 21:06:41	1.23
+++ src/sys/sys/sem.h	2001/09/15 15:01:43
@@ -37,8 +37,6 @@
 };
 #define SEM_UNDO	010000
 
-#define MAX_SOPS	5	/* maximum # of sembuf's per semop call */
-
 /*
  * semctl's arg parameter structure
  */
@@ -64,8 +62,8 @@
 /*
  * Permissions
  */
-#define SEM_A		0200	/* alter permission */
-#define SEM_R		0400	/* read permission */
+#define SEM_A		IPC_W	/* alter permission */
+#define SEM_R		IPC_R	/* read permission */
 
 #ifdef _KERNEL
 

To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010919085852.A4912>