PERFORCE change 100646 for review
John Baldwin
jhb at FreeBSD.org
Wed Jul 5 18:58:32 UTC 2006
http://perforce.freebsd.org/chv.cgi?CH=100646
Change 100646 by jhb at 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,
More information about the p4-projects
mailing list