PERFORCE change 76214 for review
Robert Watson
rwatson at FreeBSD.org
Fri Apr 29 20:25:23 GMT 2005
http://perforce.freebsd.org/chv.cgi?CH=76214
Change 76214 by rwatson at rwatson_paprika on 2005/04/29 20:24:24
Simplify -- re-fine-grain-lock posix_sem until such time as we can
demonstrate that the locking changes are necessary. Requires more
testing.
Affected files ...
.. //depot/projects/trustedbsd/mac/sys/kern/uipc_sem.c#24 edit
Differences ...
==== //depot/projects/trustedbsd/mac/sys/kern/uipc_sem.c#24 (text+ko) ====
@@ -34,8 +34,8 @@
#include <sys/cdefs.h>
__FBSDID("$FreeBSD: src/sys/kern/uipc_sem.c,v 1.17 2005/02/25 21:00:14 rwatson Exp $");
+#include "opt_mac.h"
#include "opt_posix.h"
-#include "opt_mac.h"
#include <sys/param.h>
#include <sys/systm.h>
@@ -67,7 +67,7 @@
static struct ksem *sem_lookup_byname(const char *name);
static int sem_create(struct thread *td, const char *name,
struct ksem **ksret, mode_t mode, unsigned int value);
-static void sem_free(struct ksem *ksnew, int checkrefs);
+static void sem_free(struct ksem *ksnew);
static int sem_perm(struct thread *td, struct ksem *ks);
static void sem_enter(struct proc *p, struct ksem *ks);
static int sem_leave(struct proc *p, struct ksem *ks);
@@ -84,8 +84,6 @@
semid_t *idp);
static int kern_sem_open(struct thread *td, int dir, const char *name,
int oflag, mode_t mode, unsigned int value, semid_t *idp);
-static int ksem_open_existing(struct thread *td, struct ksem *ks, int dir,
- semid_t *idpu, semid_t *idpk);
static int kern_sem_unlink(struct thread *td, const char *name);
#ifndef SEM_MAX
@@ -96,22 +94,16 @@
#define SEM_TO_ID(x) ((intptr_t)(x))
#define ID_TO_SEM(x) id_to_sem(x)
-#define SEM_FREE(ks) sem_free(ks, 1)
-#define SEM_DROP(ks) sem_free(ks, 0)
-#define REF_UP(ks) sem_ref(ks)
-#define REF_DOWN(ksem) \
- do { \
- sem_rel((ksem)); /* Pump down */ \
- if((ksem)->ks_unlinked && \
- LIST_EMPTY(&(ksem)->ks_users)) \
- SEM_FREE((ksem)); \
- } while (0)
/*
* available semaphores go here, this includes sem_init and any semaphores
* created via sem_open that have not yet been unlinked.
*/
LIST_HEAD(, ksem) ksem_head = LIST_HEAD_INITIALIZER(&ksem_head);
+/*
+ * semaphores still in use but have been sem_unlink()'d go here.
+ */
+LIST_HEAD(, ksem) ksem_deadhead = LIST_HEAD_INITIALIZER(&ksem_deadhead);
static struct mtx sem_lock;
static MALLOC_DEFINE(M_SEM, "sems", "semaphore data");
@@ -144,8 +136,9 @@
{
mtx_assert(&sem_lock, MA_OWNED);
- ks->ks_ref--;
DP(("sem_rel: ks = %p, ref = %d\n", ks, ks->ks_ref - 1));
+ if (--ks->ks_ref == 0)
+ sem_free(ks);
}
static __inline struct ksem *id_to_sem(semid_t id);
@@ -180,8 +173,6 @@
return (NULL);
}
-/* Used by both sem_init and sem_open to create a new semaphore. */
-
static int
sem_create(td, name, ksret, mode, value)
struct thread *td;
@@ -194,11 +185,11 @@
struct proc *p;
struct ucred *uc;
size_t len;
+ int error;
DP(("sem_create\n"));
p = td->td_proc;
uc = td->td_ucred;
- /* XXX Use p31b_getcfg(CTL_P1003_1B_SEM_VALUE_MAX) instead? */
if (value > SEM_VALUE_MAX)
return (EINVAL);
ret = malloc(sizeof(*ret), M_SEM, M_WAITOK | M_ZERO);
@@ -220,34 +211,31 @@
}
ret->ks_mode = mode;
ret->ks_value = value;
- ret->ks_ref = 0;
+ ret->ks_ref = 1;
ret->ks_waiters = 0;
ret->ks_uid = uc->cr_uid;
ret->ks_gid = uc->cr_gid;
ret->ks_onlist = 0;
cv_init(&ret->ks_cv, "sem");
LIST_INIT(&ret->ks_users);
- mtx_init(&ret->ks_mtx, "ks_mtx", "ks_mtx", MTX_DEF);
- if (name != NULL)
- sem_enter(td->td_proc, ret); /* This invokes sem_ref */
- else
- ret->ks_ref = 1;
#ifdef MAC
mac_init_posix_sem(ret);
mac_create_posix_sem(uc, ret);
#endif
+ if (name != NULL)
+ sem_enter(td->td_proc, ret);
+ *ksret = ret;
mtx_lock(&sem_lock);
- nsems++;
if (nsems >= p31b_getcfg(CTL_P1003_1B_SEM_NSEMS_MAX)) {
- if (name != NULL)
- sem_leave(td->td_proc, ret); /* This invokes sem_rel */
- SEM_DROP(ret); /* sem_free does a nsem-- */
- mtx_unlock(&sem_lock);
- return (ENFILE);
- }
+ sem_leave(td->td_proc, ret);
+ sem_free(ret);
+ error = ENFILE;
+ } else {
+ nsems++;
+ error = 0;
+ }
mtx_unlock(&sem_lock);
- *ksret = ret;
- return (0);
+ return (error);
}
#ifndef _SYS_SYSPROTO_H_
@@ -279,14 +267,15 @@
semid_t id;
int error;
- if((error = sem_create(td, NULL, &ks, S_IRWXU | S_IRWXG, value)))
+ error = sem_create(td, NULL, &ks, S_IRWXU | S_IRWXG, value);
+ if (error)
return (error);
id = SEM_TO_ID(ks);
if (dir == UIO_USERSPACE) {
error = copyout(&id, idp, sizeof(id));
if (error) {
mtx_lock(&sem_lock);
- SEM_DROP(ks);
+ sem_rel(ks);
mtx_unlock(&sem_lock);
return (error);
}
@@ -317,9 +306,10 @@
{
char name[SEM_MAX_NAMELEN + 1];
size_t done;
- int error = 0;
+ int error;
- if ((error = copyinstr(uap->name, name, SEM_MAX_NAMELEN + 1, &done)))
+ error = copyinstr(uap->name, name, SEM_MAX_NAMELEN + 1, &done);
+ if (error)
return (error);
DP((">>> sem_open start\n"));
error = kern_sem_open(td, UIO_USERSPACE,
@@ -329,52 +319,6 @@
}
static int
-ksem_open_existing(struct thread *td, struct ksem *ks, int dir, semid_t *idpu,
- semid_t *idpk)
-{
- int error = 0;
- mtx_assert(&sem_lock,MA_OWNED);
- mtx_assert(&ks->ks_mtx,MA_NOTOWNED);
- if((error = sem_perm(td, ks))) {
- mtx_unlock(&sem_lock);
- return (error);
- }
- /*
- * If already queued up for unlinking.
- * Then according to spec cant let reconnect to this semaphore.
- */
- if(ks->ks_unlinked) {
- mtx_unlock(&sem_lock);
- return (EPERM);
- }
- *idpk = SEM_TO_ID(ks);
- REF_UP(ks); /* Pump up the refs to avoid the race with SEM_FREE */
- mtx_unlock(&sem_lock);
-#ifdef MAC
- mtx_lock(&ks->ks_mtx);
- if((error = mac_check_posix_sem_open(td->td_ucred, ks))) {
- DP(("MAC Framework: mac_check_posix_sem_open access denied\n"));
- mtx_unlock(&ks->ks_mtx);
- goto err_open_existing;
- }
- mtx_unlock(&ks->ks_mtx);
-#endif
- if (dir == UIO_USERSPACE) {
- if ((error = copyout(idpk, idpu, sizeof(*idpk)))) {
- goto err_open_existing;
- }
- } else {
- *idpu = *idpk;
- }
- sem_enter(td->td_proc, ks);
-err_open_existing:
- mtx_lock(&sem_lock);
- REF_DOWN(ks);
- mtx_unlock(&sem_lock);
- return(error);
-}
-
-static int
kern_sem_open(td, dir, name, oflag, mode, value, idp)
struct thread *td;
int dir;
@@ -385,7 +329,7 @@
semid_t *idp;
{
struct ksem *ksnew, *ks;
- int error = 0;
+ int error;
semid_t id;
ksnew = NULL;
@@ -413,7 +357,8 @@
* We may block during creation, so drop the lock.
*/
mtx_unlock(&sem_lock);
- if((error = sem_create(td, name, &ksnew, mode, value)))
+ error = sem_create(td, name, &ksnew, mode, value);
+ if (error != 0)
return (error);
id = SEM_TO_ID(ksnew);
if (dir == UIO_USERSPACE) {
@@ -422,7 +367,7 @@
if (error) {
mtx_lock(&sem_lock);
sem_leave(td->td_proc, ksnew);
- SEM_DROP(ksnew);
+ sem_rel(ksnew);
mtx_unlock(&sem_lock);
return (error);
}
@@ -439,29 +384,51 @@
if (ks != NULL) {
/* we lost... */
sem_leave(td->td_proc, ksnew);
- SEM_DROP(ksnew);
+ sem_rel(ksnew);
/* we lost and we can't loose... */
if ((oflag & O_EXCL) != 0) {
mtx_unlock(&sem_lock);
return (EEXIST);
}
- /* Use the sem created by the winner */
- else {
- /* ksem_open_existing unlocks sem_lock */
- error = ksem_open_existing(td, ks, dir, idp, &id);
- }
} else {
DP(("sem_create: about to add to list...\n"));
LIST_INSERT_HEAD(&ksem_head, ksnew, ks_entry);
DP(("sem_create: setting list bit...\n"));
ksnew->ks_onlist = 1;
DP(("sem_create: done, about to unlock...\n"));
- mtx_unlock(&sem_lock);
}
} else {
- /* ksem_open_existing unlocks sem_lock */
- error = ksem_open_existing(td, ks, dir, idp, &id);
+#ifdef MAC
+ error = mac_check_posix_sem_open(td->td_ucred, ks);
+ if (error)
+ goto err_open;
+#endif
+ /*
+ * if we aren't the creator, then enforce permissions.
+ */
+ error = sem_perm(td, ks);
+ if (error)
+ goto err_open;
+ sem_ref(ks);
+ mtx_unlock(&sem_lock);
+ id = SEM_TO_ID(ks);
+ if (dir == UIO_USERSPACE) {
+ error = copyout(&id, idp, sizeof(id));
+ if (error) {
+ mtx_lock(&sem_lock);
+ sem_rel(ks);
+ mtx_unlock(&sem_lock);
+ return (error);
+ }
+ } else {
+ *idp = id;
+ }
+ sem_enter(td->td_proc, ks);
+ mtx_lock(&sem_lock);
+ sem_rel(ks);
}
+err_open:
+ mtx_unlock(&sem_lock);
return (error);
}
@@ -484,27 +451,18 @@
}
static void
-sem_free(struct ksem *ks, int checkrefs)
+sem_free(struct ksem *ks)
{
- mtx_assert(&sem_lock, MA_OWNED);
- mtx_assert(&ks->ks_mtx, MA_NOTOWNED);
- if(checkrefs && (ks->ks_ref > 0))
- return;
+
nsems--;
if (ks->ks_onlist)
LIST_REMOVE(ks, ks_entry);
-
if (ks->ks_name != NULL)
free(ks->ks_name, M_SEM);
cv_destroy(&ks->ks_cv);
-#ifdef MAC
- mac_destroy_posix_sem(ks);
-#endif
- mtx_destroy(&ks->ks_mtx);
free(ks, M_SEM);
}
-
static __inline struct kuser *sem_getuser(struct proc *p, struct ksem *ks);
static __inline struct kuser *
@@ -513,8 +471,7 @@
struct ksem *ks;
{
struct kuser *k;
-
- mtx_assert(&sem_lock, MA_OWNED);
+
LIST_FOREACH(k, &ks->ks_users, ku_next)
if (k->ku_pid == p->p_pid)
return (k);
@@ -526,15 +483,9 @@
struct thread *td;
struct ksem *ks;
{
- struct kuser *k;
- int ret = 0;
-
- mtx_assert(&sem_lock, MA_OWNED);
- k = sem_getuser(td->td_proc, ks);
- if ((ks->ks_name == NULL && sem_perm(td, ks) == 0) || k != NULL)
- ret = 1;
- return ret;
+ return ((ks->ks_name == NULL && sem_perm(td, ks) == 0)
+ || sem_getuser(td->td_proc, ks) != NULL);
}
static int
@@ -542,21 +493,20 @@
struct proc *p;
struct ksem *ks;
{
- struct kuser *k=NULL;
+ struct kuser *k;
DP(("sem_leave: ks = %p\n", ks));
- mtx_assert(&sem_lock, MA_OWNED);
+ k = sem_getuser(p, ks);
DP(("sem_leave: ks = %p, k = %p\n", ks, k));
- k = sem_getuser(p, ks);
- if (k == NULL) {
- return (EINVAL);
+ if (k != NULL) {
+ LIST_REMOVE(k, ku_next);
+ sem_rel(ks);
+ DP(("sem_leave: about to free k\n"));
+ free(k, M_SEM);
+ DP(("sem_leave: returning\n"));
+ return (0);
}
- LIST_REMOVE(k, ku_next);
- sem_rel(ks);
- DP(("sem_leave: about to free k\n"));
- free(k, M_SEM);
- DP(("sem_leave: returning\n"));
- return (0);
+ return (EINVAL);
}
static void
@@ -566,9 +516,7 @@
{
struct kuser *ku, *k;
- mtx_assert(&sem_lock, MA_NOTOWNED);
- mtx_assert(&ks->ks_mtx, MA_NOTOWNED);
- ku = malloc(sizeof(*ku), M_SEM, M_WAITOK | M_ZERO);
+ ku = malloc(sizeof(*ku), M_SEM, M_WAITOK);
ku->ku_pid = p->p_pid;
mtx_lock(&sem_lock);
k = sem_getuser(p, ks);
@@ -609,28 +557,27 @@
const char *name;
{
struct ksem *ks;
- int error = 0;
+ int error;
mtx_lock(&sem_lock);
ks = sem_lookup_byname(name);
- if (ks == NULL) {
- error = ENOENT;
- goto err_unlink;
- }
- if ((error = sem_perm(td, ks)))
- goto err_unlink;
+ if (ks != NULL) {
#ifdef MAC
- if((error = mac_check_posix_sem_unlink(td->td_ucred, ks))) {
- DP(("MAC Framework: mac_check_posix_sem_unlink access \
- denied\n"));
- goto err_unlink;
- }
+ error = mac_check_posix_sem_unlink(td->td_ucred, ks);
+ if (error) {
+ mtx_unlock(&sem_lock);
+ return (error);
+ }
#endif
+ error = sem_perm(td, ks);
+ } else
+ error = ENOENT;
DP(("sem_unlink: '%s' ks = %p, error = %d\n", name, ks, error));
- ks->ks_unlinked = 1;
- if(LIST_EMPTY(&ks->ks_users))
- SEM_FREE(ks);
-err_unlink:
+ if (error == 0) {
+ LIST_REMOVE(ks, ks_entry);
+ LIST_INSERT_HEAD(&ksem_deadhead, ks, ks_entry);
+ sem_rel(ks);
+ }
mtx_unlock(&sem_lock);
return (error);
}
@@ -657,17 +604,12 @@
struct ksem *ks;
int error;
+ error = EINVAL;
mtx_lock(&sem_lock);
ks = ID_TO_SEM(id);
/* this is not a valid operation for unnamed sems */
- error = EINVAL;
- if (ks != NULL && ks->ks_name != NULL) {
- if ((error = sem_leave(td->td_proc, ks)))
- goto err_close;
- if (ks->ks_unlinked && LIST_EMPTY(&ks->ks_users))
- SEM_FREE(ks);
- }
-err_close:
+ if (ks != NULL && ks->ks_name != NULL)
+ error = sem_leave(td->td_proc, ks);
mtx_unlock(&sem_lock);
return (error);
}
@@ -693,35 +635,28 @@
semid_t id;
{
struct ksem *ks;
- int error = 0;
+ int error;
mtx_lock(&sem_lock);
ks = ID_TO_SEM(id);
if (ks == NULL || !sem_hasopen(td, ks)) {
- mtx_unlock(&sem_lock);
- return (EINVAL);
+ error = EINVAL;
+ goto err;
}
- REF_UP(ks);/* Pump up the refs to avoid the race with SEM_FREE */
- mtx_unlock(&sem_lock);
-
- mtx_lock(&ks->ks_mtx);
+#ifdef MAC
+ error = mac_check_posix_sem_post(td->td_ucred, ks);
+ if (error)
+ goto err;
+#endif
if (ks->ks_value == SEM_VALUE_MAX) {
error = EOVERFLOW;
- goto err_post;
+ goto err;
}
-#ifdef MAC
- if ((error = mac_check_posix_sem_post(td->td_ucred, ks))) {
- DP(("MAC Framework: mac_check_posix_sem_post access denied\n"));
- goto err_post;
- }
-#endif
++ks->ks_value;
if (ks->ks_waiters > 0)
cv_signal(&ks->ks_cv);
-err_post:
- mtx_unlock(&ks->ks_mtx);
- mtx_lock(&sem_lock);
- REF_DOWN(ks);
+ error = 0;
+err:
mtx_unlock(&sem_lock);
return (error);
}
@@ -797,29 +732,27 @@
struct timespec ts1, ts2;
struct timeval tv;
struct ksem *ks;
- int error = 0;
+ int error;
DP((">>> kern_sem_wait entered!\n"));
mtx_lock(&sem_lock);
ks = ID_TO_SEM(id);
if (ks == NULL) {
DP(("kern_sem_wait ks == NULL\n"));
- mtx_unlock(&sem_lock);
- return (EINVAL);
+ error = EINVAL;
+ goto err;
}
+ sem_ref(ks);
if (!sem_hasopen(td, ks)) {
DP(("kern_sem_wait hasopen failed\n"));
- mtx_unlock(&sem_lock);
- return (EINVAL);
+ error = EINVAL;
+ goto err;
}
- REF_UP(ks);/* Pump up the refs to avoid the race with SEM_FREE */
- mtx_unlock(&sem_lock);
-
- mtx_lock(&ks->ks_mtx);
#ifdef MAC
- if ((error = mac_check_posix_sem_wait(td->td_ucred, ks))) {
- DP(("MAC Framework: mac_check_posix_sem_wait access denied\n"));
- goto err_wait;
+ error = mac_check_posix_sem_wait(td->td_ucred, ks);
+ if (error) {
+ DP(("kern_sem_wait mac failed\n"));
+ goto err;
}
#endif
DP(("kern_sem_wait value = %d, tryflag %d\n", ks->ks_value, tryflag));
@@ -828,7 +761,7 @@
if (tryflag != 0)
error = EAGAIN;
else if (abstime == NULL)
- error = cv_wait_sig(&ks->ks_cv, &ks->ks_mtx);
+ error = cv_wait_sig(&ks->ks_cv, &sem_lock);
else {
for (;;) {
ts1 = *abstime;
@@ -840,22 +773,22 @@
break;
}
error = cv_timedwait_sig(&ks->ks_cv,
- &ks->ks_mtx, tvtohz(&tv));
+ &sem_lock, tvtohz(&tv));
if (error != EWOULDBLOCK)
break;
}
}
ks->ks_waiters--;
if (error)
- goto err_wait;
+ goto err;
}
ks->ks_value--;
-err_wait:
- mtx_unlock(&ks->ks_mtx);
+ error = 0;
+err:
+ if (ks != NULL)
+ sem_rel(ks);
+ mtx_unlock(&sem_lock);
DP(("<<< kern_sem_wait leaving, error = %d\n", error));
- mtx_lock(&sem_lock);
- REF_DOWN(ks);
- mtx_unlock(&sem_lock);
return (error);
}
@@ -880,26 +813,16 @@
mtx_unlock(&sem_lock);
return (EINVAL);
}
- REF_UP(ks);/* Pump up the refs to avoid the race with SEM_FREE */
- mtx_unlock(&sem_lock);
-
- mtx_lock(&ks->ks_mtx);
#ifdef MAC
- if((error = mac_check_posix_sem_getvalue(td->td_ucred, ks))) {
- DP(("MAC Framework: mac_check_posix_sem_getvalue access denied\n"));
- mtx_unlock(&ks->ks_mtx);
- goto err_getvalue;
- }
+ error = mac_check_posix_sem_getvalue(td->td_ucred, ks);
+ if (error) {
+ mtx_unlock(&sem_lock);
+ return (error);
+ }
#endif
val = ks->ks_value;
- mtx_unlock(&ks->ks_mtx);
+ mtx_unlock(&sem_lock);
error = copyout(&val, uap->val, sizeof(val));
-#ifdef MAC
-err_getvalue:
-#endif
- mtx_lock(&sem_lock);
- REF_DOWN(ks);
- mtx_unlock(&sem_lock);
return (error);
}
@@ -915,24 +838,27 @@
struct ksem_destroy_args *uap;
{
struct ksem *ks;
- int error = 0;
+ int error;
mtx_lock(&sem_lock);
ks = ID_TO_SEM(uap->id);
if (ks == NULL || !sem_hasopen(td, ks) ||
ks->ks_name != NULL) {
error = EINVAL;
- goto err_destroy;
+ goto err;
}
#ifdef MAC
- if((error = mac_check_posix_sem_destroy(td->td_ucred, ks))) {
- DP(("MAC Framework: mac_check_posix_sem_destroy access denied\n"));
- goto err_destroy;
+ error = mac_check_posix_sem_destroy(td->td_ucred, ks);
+ if (error)
+ goto err;
+#endif
+ if (ks->ks_waiters != 0) {
+ error = EBUSY;
+ goto err;
}
-#endif
- ks->ks_unlinked = 1; /* Indicate that the sem needs to be destroyed */
- SEM_FREE(ks);
-err_destroy:
+ sem_rel(ks);
+ error = 0;
+err:
mtx_unlock(&sem_lock);
return (error);
}
@@ -1055,9 +981,13 @@
ks = LIST_FIRST(&ksem_head);
while (ks != NULL) {
ksnext = LIST_NEXT(ks, ks_entry);
- if((ks->ks_name != NULL) && (!sem_leave(p, ks)))
- if (ks->ks_unlinked && LIST_EMPTY(&ks->ks_users))
- SEM_FREE(ks);
+ sem_leave(p, ks);
+ ks = ksnext;
+ }
+ ks = LIST_FIRST(&ksem_deadhead);
+ while (ks != NULL) {
+ ksnext = LIST_NEXT(ks, ks_entry);
+ sem_leave(p, ks);
ks = ksnext;
}
mtx_unlock(&sem_lock);
To Unsubscribe: send mail to majordomo at trustedbsd.org
with "unsubscribe trustedbsd-cvs" in the body of the message
More information about the trustedbsd-cvs
mailing list