Rework kern_semctl a bit to always assume the UIO_SYSSPACE case. This

mostly consists of pushing a few copyin's and copyout's up into
__semctl() as all the other callers were already doing the UIO_SYSSPACE
case.  This also changes kern_semctl() to set the return value in a passed
in pointer to a register_t rather than td->td_retval[0] directly so that
callers can only set td->td_retval[0] if all the various copyout's succeed.

As a result of these changes, kern_semctl() no longer does copyin/copyout
(except for GETALL/SETALL) so simplify the locking to acquire the semakptr
mutex before the MAC check and hold it all the way until the end of the
big switch statement.  The GETALL/SETALL cases have to temporarily drop it
while they do copyin/malloc and copyout.  Also, simplify the SETALL case to
remove handling for a non-existent race condition.
This commit is contained in:
John Baldwin 2006-07-08 19:51:38 +00:00
parent db2bc1bb82
commit b1ee5b654d
Notes: svn2git 2020-12-20 02:59:44 +00:00
svn path=/head/; revision=160187
4 changed files with 113 additions and 105 deletions

View file

@ -494,6 +494,7 @@ linux_semctl(struct thread *td, struct linux_semctl_args *args)
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 @@ linux_semctl(struct thread *td, struct linux_semctl_args *args)
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 @@ linux_semctl(struct thread *td, struct linux_semctl_args *args)
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

View file

@ -209,6 +209,7 @@ svr4_semctl(td, v)
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 @@ svr4_semctl(td, v)
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 @@ svr4_semctl(td, v)
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 {

View file

@ -556,8 +556,9 @@ __semctl(td, uap)
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 @@ __semctl(td, uap)
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);
arg = &real_arg;
break;
default:
arg = NULL;
break;
}
return (kern_semctl(td, uap->semid, uap->semnum, uap->cmd, arg,
UIO_USERSPACE));
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);
semun.buf = &dsbuf;
break;
case GETALL:
case SETALL:
semun.array = arg.array;
break;
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;
}
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 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
goto done2;
}
#endif
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);
*rval = IXSEQ_TO_IPCID(semid, semakptr->u.sem_perm);
mtx_unlock(sema_mtxp);
return (0);
}
semidx = IPCID_TO_IX(semid);
@ -643,23 +668,20 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
semakptr = &sema[semidx];
sema_mtxp = &sema_mtx[semidx];
#ifdef MAC
mtx_lock(sema_mtxp);
#ifdef MAC
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;
rval = 0;
*rval = 0;
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 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
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));
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 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
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 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
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 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
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 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
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,16 +834,9 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
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);
array = malloc(sizeof(*array) * count, M_TEMP, M_WAITOK);
@ -835,12 +846,7 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
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,10 +868,7 @@ kern_semctl(struct thread *td, int semid, int semnum, int cmd, union semun *arg,
break;
}
if (error == 0)
td->td_retval[0] = rval;
done2:
if (mtx_owned(sema_mtxp))
mtx_unlock(sema_mtxp);
if (array != NULL)
free(array, M_TEMP);

View file

@ -126,7 +126,7 @@ 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,