Date: Fri, 29 Apr 2005 20:25:23 GMT From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 76214 for review Message-ID: <200504292025.j3TKPNcP081382@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=76214 Change 76214 by rwatson@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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200504292025.j3TKPNcP081382>