Date: Wed, 5 Jul 2006 18:58:31 GMT From: John Baldwin <jhb@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 100646 for review Message-ID: <200607051858.k65IwVC4038486@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=100646 Change 100646 by jhb@jhb_mutex on 2006/07/05 18:58:08 - Change kern_semctl() to always assume UIO_SYSSPACE and just fix __semctl() to do the copyout's aside from GETALL and SETALL. Also, don't set td->td_retval[0] directly in kern_semctl() so we only set it in ABIs on success now. - Remove the race loop from SETALL with notes about why. Affected files ... .. //depot/projects/smpng/sys/compat/linux/linux_ipc.c#26 edit .. //depot/projects/smpng/sys/compat/svr4/svr4_ipc.c#17 edit .. //depot/projects/smpng/sys/i386/ibcs2/ibcs2_ipc.c#12 edit .. //depot/projects/smpng/sys/kern/sysv_sem.c#39 edit .. //depot/projects/smpng/sys/sys/syscallsubr.h#39 edit Differences ... ==== //depot/projects/smpng/sys/compat/linux/linux_ipc.c#26 (text+ko) ==== @@ -494,6 +494,7 @@ struct l_seminfo linux_seminfo; struct semid_ds semid; union semun semun; + register_t rval; int cmd, error; switch (args->cmd & ~LINUX_IPC_64) { @@ -524,25 +525,25 @@ return (error); linux_to_bsd_semid_ds(&linux_semid, &semid); semun.buf = &semid; - return kern_semctl(td, args->semid, args->semnum, cmd, &semun, - UIO_SYSSPACE); + return (kern_semctl(td, args->semid, args->semnum, cmd, &semun, + td->td_retval)); case LINUX_IPC_STAT: case LINUX_SEM_STAT: - if((args->cmd & ~LINUX_IPC_64) == LINUX_IPC_STAT) + if ((args->cmd & ~LINUX_IPC_64) == LINUX_IPC_STAT) cmd = IPC_STAT; else cmd = SEM_STAT; semun.buf = &semid; error = kern_semctl(td, args->semid, args->semnum, cmd, &semun, - UIO_SYSSPACE); + &rval); if (error) return (error); - td->td_retval[0] = (cmd == SEM_STAT) ? - IXSEQ_TO_IPCID(args->semid, semid.sem_perm) : - 0; bsd_to_linux_semid_ds(&semid, &linux_semid); - return (linux_semid_pushdown(args->cmd & LINUX_IPC_64, - &linux_semid, (caddr_t)PTRIN(args->arg.buf))); + error = linux_semid_pushdown(args->cmd & LINUX_IPC_64, + &linux_semid, (caddr_t)PTRIN(args->arg.buf)); + if (error == 0) + td->td_retval[0] = (cmd == SEM_STAT) ? rval : 0; + return (error); case LINUX_IPC_INFO: case LINUX_SEM_INFO: bcopy(&seminfo, &linux_seminfo, sizeof(linux_seminfo) ); @@ -567,8 +568,8 @@ args->cmd & ~LINUX_IPC_64); return EINVAL; } - return kern_semctl(td, args->semid, args->semnum, cmd, &semun, - UIO_USERSPACE); + return (kern_semctl(td, args->semid, args->semnum, cmd, &semun, + td->td_retval)); } int ==== //depot/projects/smpng/sys/compat/svr4/svr4_ipc.c#17 (text+ko) ==== @@ -209,6 +209,7 @@ struct svr4_semid_ds ss; struct semid_ds bs; union semun semun; + register_t rval; int cmd, error; switch (uap->cmd) { @@ -244,21 +245,24 @@ cmd = IPC_STAT; semun.buf = &bs; error = kern_semctl(td, uap->semid, uap->semnum, cmd, &semun, - UIO_SYSSPACE); + &rval); if (error) - return error; + return (error); bsd_to_svr4_semid_ds(&bs, &ss); - return copyout(&ss, uap->arg.buf, sizeof(ss)); + error = copyout(&ss, uap->arg.buf, sizeof(ss)); + if (error == 0) + td->td_retval[0] = rval; + return (error); case SVR4_IPC_SET: cmd = IPC_SET; error = copyin(uap->arg.buf, (caddr_t) &ss, sizeof ss); if (error) - return error; + return (error); svr4_to_bsd_semid_ds(&ss, &bs); semun.buf = &bs; - return kern_semctl(td, uap->semid, uap->semnum, cmd, &semun, - UIO_SYSSPACE); + return (kern_semctl(td, uap->semid, uap->semnum, cmd, &semun, + td->td_retval)); case SVR4_IPC_RMID: cmd = IPC_RMID; @@ -268,8 +272,8 @@ return EINVAL; } - return kern_semctl(td, uap->semid, uap->semnum, cmd, &uap->arg, - UIO_USERSPACE); + return (kern_semctl(td, uap->semid, uap->semnum, cmd, &uap->arg, + td->td_retval)); } struct svr4_sys_semget_args { ==== //depot/projects/smpng/sys/i386/ibcs2/ibcs2_ipc.c#12 (text+ko) ==== @@ -316,17 +316,21 @@ struct ibcs2_semid_ds is; struct semid_ds bs; union semun semun; + register_t rval; int error; switch(uap->cmd) { case IBCS2_IPC_STAT: semun.buf = &bs; error = kern_semctl(td, uap->semid, uap->semnum, IPC_STAT, - &semun, UIO_SYSSPACE); + &semun, &rval); if (error) return (error); cvt_semid2isemid(&bs, &is); - return (copyout(&is, uap->arg.buf, sizeof(is))); + error = copyout(&is, uap->arg.buf, sizeof(is)); + if (error == 0) + td->td_retval[0] = rval; + return (error); case IBCS2_IPC_SET: error = copyin(uap->arg.buf, &is, sizeof(is)); @@ -335,11 +339,11 @@ cvt_isemid2semid(&is, &bs); semun.buf = &bs; return (kern_semctl(td, uap->semid, uap->semnum, IPC_SET, - &semun, UIO_SYSSPACE)); + &semun, td->td_retval)); } return (kern_semctl(td, uap->semid, uap->semnum, uap->cmd, &uap->arg, - UIO_USERSPACE)); + td->td_retval)); } struct ibcs2_semget_args { ==== //depot/projects/smpng/sys/kern/sysv_sem.c#39 (text+ko) ==== @@ -556,8 +556,9 @@ struct thread *td; struct __semctl_args *uap; { - union semun real_arg; - union semun *arg; + struct semid_ds dsbuf; + union semun arg, semun; + register_t rval; int error; switch (uap->cmd) { @@ -567,27 +568,57 @@ case GETALL: case SETVAL: case SETALL: - error = copyin(uap->arg, &real_arg, sizeof(real_arg)); + error = copyin(uap->arg, &arg, sizeof(arg)); + if (error) + return (error); + break; + } + + switch (uap->cmd) { + case SEM_STAT: + case IPC_STAT: + semun.buf = &dsbuf; + break; + case IPC_SET: + error = copyin(arg.buf, &dsbuf, sizeof(dsbuf)); if (error) return (error); - arg = &real_arg; + semun.buf = &dsbuf; + break; + case GETALL: + case SETALL: + semun.array = arg.array; break; - default: - arg = NULL; + case SETVAL: + semun.val = arg.val; + break; + } + + error = kern_semctl(td, uap->semid, uap->semnum, uap->cmd, &semun, + &rval); + if (error) + return (error); + + switch (uap->cmd) { + case SEM_STAT: + case IPC_STAT: + error = copyout(&dsbuf, arg.buf, sizeof(dsbuf)); break; } - return (kern_semctl(td, uap->semid, uap->semnum, uap->cmd, arg, - UIO_USERSPACE)); + + if (error == 0) + td->td_retval[0] = rval; + return (error); } int -kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg, - enum uio_seg bufseg) +kern_semctl(struct thread *td, int semid, int semnum, int cmd, + union semun *arg, register_t *rval) { u_short *array; struct ucred *cred = td->td_ucred; - int i, rval, error; - struct semid_ds sbuf; + int i, error; + struct semid_ds *sbuf; struct semid_kernel *semakptr; struct mtx *sema_mtxp; u_short usval, count; @@ -625,16 +656,10 @@ goto done2; } #endif + bcopy(&semakptr->u, arg->buf, sizeof(struct semid_ds)); + *rval = IXSEQ_TO_IPCID(semid, semakptr->u.sem_perm); mtx_unlock(sema_mtxp); - if (bufseg == UIO_USERSPACE) - error = copyout(&semakptr->u, arg->buf, - sizeof(struct semid_ds)); - else - bcopy(&semakptr->u, arg->buf, sizeof(struct semid_ds)); - rval = IXSEQ_TO_IPCID(semid, semakptr->u.sem_perm); - if (error == 0) - td->td_retval[0] = rval; - return (error); + return (0); } semidx = IPCID_TO_IX(semid); @@ -643,15 +668,13 @@ semakptr = &sema[semidx]; sema_mtxp = &sema_mtx[semidx]; + mtx_lock(sema_mtxp); #ifdef MAC - mtx_lock(sema_mtxp); error = mac_check_sysv_semctl(cred, semakptr, cmd); if (error != 0) { MPRINTF(("mac_check_sysv_semctl returned %d\n", error)); - mtx_unlock(sema_mtxp); - return (error); + goto done2; } - mtx_unlock(sema_mtxp); #endif error = 0; @@ -659,7 +682,6 @@ switch (cmd) { case IPC_RMID: - mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_M))) @@ -685,41 +707,27 @@ break; case IPC_SET: - if (bufseg == UIO_USERSPACE) { - error = copyin(arg->buf, &sbuf, sizeof(sbuf)); - if (error) - goto done2; - } else - bcopy(arg->buf, &sbuf, sizeof(sbuf)); - mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_M))) goto done2; - semakptr->u.sem_perm.uid = sbuf.sem_perm.uid; - semakptr->u.sem_perm.gid = sbuf.sem_perm.gid; + sbuf = arg->buf; + semakptr->u.sem_perm.uid = sbuf->sem_perm.uid; + semakptr->u.sem_perm.gid = sbuf->sem_perm.gid; semakptr->u.sem_perm.mode = (semakptr->u.sem_perm.mode & - ~0777) | (sbuf.sem_perm.mode & 0777); + ~0777) | (sbuf->sem_perm.mode & 0777); semakptr->u.sem_ctime = time_second; break; case IPC_STAT: - mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_R))) goto done2; - sbuf = semakptr->u; - mtx_unlock(sema_mtxp); - if (bufseg == UIO_USERSPACE) - error = copyout(&semakptr->u, arg->buf, - sizeof(struct semid_ds)); - else - bcopy(&semakptr->u, arg->buf, sizeof(struct semid_ds)); + bcopy(&semakptr->u, arg->buf, sizeof(struct semid_ds)); break; case GETNCNT: - mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_R))) @@ -728,11 +736,10 @@ error = EINVAL; goto done2; } - rval = semakptr->u.sem_base[semnum].semncnt; + *rval = semakptr->u.sem_base[semnum].semncnt; break; case GETPID: - mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_R))) @@ -741,11 +748,10 @@ error = EINVAL; goto done2; } - rval = semakptr->u.sem_base[semnum].sempid; + *rval = semakptr->u.sem_base[semnum].sempid; break; case GETVAL: - mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_R))) @@ -754,34 +760,47 @@ error = EINVAL; goto done2; } - rval = semakptr->u.sem_base[semnum].semval; + *rval = semakptr->u.sem_base[semnum].semval; break; case GETALL: /* - * Given the fact that this copies out a variable amount - * of data into userland, I don't see any feasible way of - * doing this with a kmem copy of the userland buffer. + * Unfortunately, callers of this function don't know + * in advance how many semaphores are in this set. + * While we could just allocate the maximum size array + * and pass the actual size back to the caller, that + * won't work for SETALL since we can't copyin() more + * data than the user specified as we may return a + * spurious EFAULT. + * + * Note that the number of semaphores in a set is + * fixed for the life of that set. The only way that + * the 'count' could change while are blocked in + * malloc() is if this semaphore set were destroyed + * and a new one created with the same index. + * However, semvalid() will catch that due to the + * sequence number unless exactly 0x8000 (or a + * multiple thereof) semaphore sets for the same index + * are created and destroyed while we are in malloc! + * */ - if (bufseg == UIO_SYSSPACE) - return (EINVAL); - - array = malloc(sizeof(*array) * semakptr->u.sem_nsems, M_TEMP, - M_WAITOK); + count = semakptr->u.sem_nsems; + mtx_unlock(sema_mtxp); + array = malloc(sizeof(*array) * count, M_TEMP, M_WAITOK); mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; + KASSERT(count == semakptr->u.sem_nsems, ("nsems changed")); if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_R))) goto done2; for (i = 0; i < semakptr->u.sem_nsems; i++) array[i] = semakptr->u.sem_base[i].semval; mtx_unlock(sema_mtxp); - error = copyout(array, arg->array, - i * sizeof(arg->array[0])); + error = copyout(array, arg->array, count * sizeof(*array)); + mtx_lock(sema_mtxp); break; case GETZCNT: - mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_R))) @@ -790,11 +809,10 @@ error = EINVAL; goto done2; } - rval = semakptr->u.sem_base[semnum].semzcnt; + *rval = semakptr->u.sem_base[semnum].semzcnt; break; case SETVAL: - mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_W))) @@ -816,18 +834,11 @@ case SETALL: /* - * Given the fact that this copies in variable amounts of - * userland data in a loop, I don't see any feasible way - * of doing this with a kmem copy of the userland buffer. + * See comment on GETALL for why 'count' shouldn't change + * and why we require a userland buffer. */ - if (bufseg == UIO_SYSSPACE) - return (EINVAL); - mtx_lock(sema_mtxp); -raced: - if ((error = semvalid(semid, semakptr)) != 0) - goto done2; count = semakptr->u.sem_nsems; - mtx_unlock(sema_mtxp); + mtx_unlock(sema_mtxp); array = malloc(sizeof(*array) * count, M_TEMP, M_WAITOK); error = copyin(arg->array, array, count * sizeof(*array)); if (error) @@ -835,12 +846,7 @@ mtx_lock(sema_mtxp); if ((error = semvalid(semid, semakptr)) != 0) goto done2; - /* we could have raced? */ - if (count != semakptr->u.sem_nsems) { - free(array, M_TEMP); - array = NULL; - goto raced; - } + KASSERT(count == semakptr->u.sem_nsems, ("nsems changed")); if ((error = ipcperm(td, &semakptr->u.sem_perm, IPC_W))) goto done2; for (i = 0; i < semakptr->u.sem_nsems; i++) { @@ -862,11 +868,8 @@ break; } - if (error == 0) - td->td_retval[0] = rval; done2: - if (mtx_owned(sema_mtxp)) - mtx_unlock(sema_mtxp); + mtx_unlock(sema_mtxp); if (array != NULL) free(array, M_TEMP); return(error); ==== //depot/projects/smpng/sys/sys/syscallsubr.h#39 (text+ko) ==== @@ -125,15 +125,15 @@ char *buf, enum uio_seg bufseg, int count); int kern_readv(struct thread *td, int fd, struct uio *auio); int kern_recvit(struct thread *td, int s, struct msghdr *mp, - void *namelenp, enum uio_seg uioseg, enum uio_seg fromseg, - struct mbuf **controlp); + void *namelenp, enum uio_seg uioseg, enum uio_seg fromseg, + struct mbuf **controlp); int kern_rename(struct thread *td, char *from, char *to, enum uio_seg pathseg); int kern_rmdir(struct thread *td, char *path, enum uio_seg pathseg); int kern_sched_rr_get_interval(struct thread *td, pid_t pid, struct timespec *ts); int kern_semctl(struct thread *td, int semid, int semnum, int cmd, - union semun *arg, enum uio_seg bufseg); + union semun *arg, register_t *rval); int kern_select(struct thread *td, int nd, fd_set *fd_in, fd_set *fd_ou, fd_set *fd_ex, struct timeval *tvp); int kern_sendfile(struct thread *td, struct sendfile_args *uap,
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200607051858.k65IwVC4038486>