From owner-freebsd-bugs Tue Sep 18 15:59:25 2001 Delivered-To: freebsd-bugs@freebsd.org Received: from netau1.alcanet.com.au (ntp.alcanet.com.au [203.62.196.27]) by hub.freebsd.org (Postfix) with ESMTP id 62EBE37B408; Tue, 18 Sep 2001 15:59:01 -0700 (PDT) Received: from mfg1.cim.alcatel.com.au (mfg1.cim.alcatel.com.au [139.188.23.1]) by netau1.alcanet.com.au (8.9.3 (PHNE_22672)/8.9.3) with ESMTP id IAA17214; Wed, 19 Sep 2001 08:58:58 +1000 (EST) Received: from gsmx07.alcatel.com.au by cim.alcatel.com.au (PMDF V5.2-32 #37645) with ESMTP id <01K8IKFQTT1C22X8GT@cim.alcatel.com.au>; Wed, 19 Sep 2001 08:58:56 +1000 Received: (from jeremyp@localhost) by gsmx07.alcatel.com.au (8.11.1/8.11.1) id f8IMwqW05014; Wed, 19 Sep 2001 08:58:52 +1000 (EST envelope-from jeremyp) Content-return: prohibited Date: Wed, 19 Sep 2001 08:58:52 +1000 From: Peter Jeremy Subject: Re: kern/12014: Fix SysV Semaphore handling In-reply-to: <199906032120.OAA40933@freefall.freebsd.org>; from gnats-admin@FreeBSD.org on Fri, Jun 04, 1999 at 07:20:02AM +1000 To: gnats-admin@FreeBSD.org, freebsd-bugs@FreeBSD.org Cc: Michael Reifenberger Message-id: <20010919085852.A4912@gsmx07.alcatel.com.au> MIME-version: 1.0 Content-type: text/plain; charset=us-ascii Content-disposition: inline User-Agent: Mutt/1.2.5i References: <"99Jun4.065824est.40321"@border.alcanet.com.au> <199906032120.OAA40933@freefall.freebsd.org> Sender: owner-freebsd-bugs@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org 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