From 06ac0dc77f336396b1fd8b2a3c2820ca8c44f2e1 Mon Sep 17 00:00:00 2001 From: sbruno Date: Fri, 5 Jun 2015 18:16:10 +0000 Subject: [PATCH] Revert 284029, update imgact_binmisctl.c change mtx to reader count, at the request of the submitter. Will attempt to use an sx_lock for this fix to WITNESS crashes in a later revision. Submitted by: sson --- sys/kern/imgact_binmisc.c | 149 +++++++++++--------------------------- 1 file changed, 44 insertions(+), 105 deletions(-) diff --git a/sys/kern/imgact_binmisc.c b/sys/kern/imgact_binmisc.c index c0ead4b2514b..5f324d3c3e16 100644 --- a/sys/kern/imgact_binmisc.c +++ b/sys/kern/imgact_binmisc.c @@ -1,5 +1,5 @@ /*- - * Copyright (c) 2013-15, Stacey D. Son + * Copyright (c) 2013, Stacey D. Son * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -41,9 +41,6 @@ __FBSDID("$FreeBSD$"); #include #include #include -#include - -#include /** * Miscellaneous binary interpreter image activator. @@ -98,68 +95,11 @@ static SLIST_HEAD(, imgact_binmisc_entry) interpreter_list = SLIST_HEAD_INITIALIZER(interpreter_list); static int interp_list_entry_count = 0; -static int interp_list_rcount = 0; + static struct mtx interp_list_mtx; int imgact_binmisc_exec(struct image_params *imgp); -static inline void -interp_list_rlock() -{ - - /* Don't use atomic_add() here. We must block if the mutex is held. */ - mtx_lock(&interp_list_mtx); - interp_list_rcount++; - mtx_unlock(&interp_list_mtx); -} - -static inline void -interp_list_runlock() -{ - - /* Decrement the reader count and wake up one writer, if any. */ - atomic_subtract_int(&interp_list_rcount, 1); - if (interp_list_rcount == 0) - wakeup_one(&interp_list_rcount); -} - -static inline void -interp_list_wlock() -{ - - /* Wait until there are no readers. */ - mtx_lock(&interp_list_mtx); - while (interp_list_rcount) - mtx_sleep(&interp_list_rcount, &interp_list_mtx, 0, "IABM", 0); -} - -static inline void -interp_list_wunlock() -{ - - mtx_unlock(&interp_list_mtx); -} - -#define interp_list_assert_rlocked() \ - KASSERT(interp_list_rcount > 0, ("%s: interp_list lock not held " \ - "for read", __func__)) - -#define interp_list_assert_wlocked() mtx_assert(&interp_list_mtx, MA_OWNED) - -#ifdef INVARIANTS -#define interp_list_assert_locked() do { \ - if (interp_list_rcount == 0) \ - mtx_assert(&interp_list_mtx, MA_OWNED); \ - else \ - KASSERT(interp_list_rcount > 0, \ - ("%s: interp_list lock not held", __func__)); \ -} while(0) - -#else /* ! INVARIANTS */ - -#define interp_list_assert_locked() do { \ -} while(0) -#endif /* ! INVARIANTS */ /* * Populate the entry with the information about the interpreter. @@ -171,7 +111,7 @@ imgact_binmisc_populate_interp(char *str, imgact_binmisc_entry_t *ibe) char t[IBE_INTERP_LEN_MAX]; char *sp, *tp; - memset(t, 0, sizeof(t)); + bzero(t, sizeof(t)); /* * Normalize interpreter string. Replace white space between args with @@ -212,6 +152,8 @@ imgact_binmisc_new_entry(ximgact_binmisc_entry_t *xbe) imgact_binmisc_entry_t *ibe = NULL; size_t namesz = min(strlen(xbe->xbe_name) + 1, IBE_NAME_MAX); + mtx_assert(&interp_list_mtx, MA_NOTOWNED); + ibe = malloc(sizeof(*ibe), M_BINMISC, M_WAITOK|M_ZERO); ibe->ibe_name = malloc(namesz, M_BINMISC, M_WAITOK|M_ZERO); @@ -261,7 +203,7 @@ imgact_binmisc_find_entry(char *name) { imgact_binmisc_entry_t *ibe; - interp_list_assert_locked(); + mtx_assert(&interp_list_mtx, MA_OWNED); SLIST_FOREACH(ibe, &interpreter_list, link) { if (strncmp(name, ibe->ibe_name, IBE_NAME_MAX) == 0) @@ -318,21 +260,21 @@ imgact_binmisc_add_entry(ximgact_binmisc_entry_t *xbe) } } - /* Preallocate a new entry. */ + mtx_lock(&interp_list_mtx); + if (imgact_binmisc_find_entry(xbe->xbe_name) != NULL) { + mtx_unlock(&interp_list_mtx); + return (EEXIST); + } + mtx_unlock(&interp_list_mtx); + ibe = imgact_binmisc_new_entry(xbe); if (!ibe) return (ENOMEM); - interp_list_wlock(); - if (imgact_binmisc_find_entry(xbe->xbe_name) != NULL) { - interp_list_wunlock(); - imgact_binmisc_destroy_entry(ibe); - return (EEXIST); - } - + mtx_lock(&interp_list_mtx); SLIST_INSERT_HEAD(&interpreter_list, ibe, link); interp_list_entry_count++; - interp_list_wunlock(); + mtx_unlock(&interp_list_mtx); return (0); } @@ -346,14 +288,14 @@ imgact_binmisc_remove_entry(char *name) { imgact_binmisc_entry_t *ibe; - interp_list_wlock(); + mtx_lock(&interp_list_mtx); if ((ibe = imgact_binmisc_find_entry(name)) == NULL) { - interp_list_wunlock(); + mtx_unlock(&interp_list_mtx); return (ENOENT); } SLIST_REMOVE(&interpreter_list, ibe, imgact_binmisc_entry, link); interp_list_entry_count--; - interp_list_wunlock(); + mtx_unlock(&interp_list_mtx); imgact_binmisc_destroy_entry(ibe); @@ -369,14 +311,14 @@ imgact_binmisc_disable_entry(char *name) { imgact_binmisc_entry_t *ibe; - interp_list_rlock(); + mtx_lock(&interp_list_mtx); if ((ibe = imgact_binmisc_find_entry(name)) == NULL) { - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); return (ENOENT); } - atomic_clear_32(&ibe->ibe_flags, IBF_ENABLED); - interp_list_runlock(); + ibe->ibe_flags &= ~IBF_ENABLED; + mtx_unlock(&interp_list_mtx); return (0); } @@ -390,14 +332,14 @@ imgact_binmisc_enable_entry(char *name) { imgact_binmisc_entry_t *ibe; - interp_list_rlock(); + mtx_lock(&interp_list_mtx); if ((ibe = imgact_binmisc_find_entry(name)) == NULL) { - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); return (ENOENT); } - atomic_set_32(&ibe->ibe_flags, IBF_ENABLED); - interp_list_runlock(); + ibe->ibe_flags |= IBF_ENABLED; + mtx_unlock(&interp_list_mtx); return (0); } @@ -408,9 +350,9 @@ imgact_binmisc_populate_xbe(ximgact_binmisc_entry_t *xbe, { uint32_t i; - interp_list_assert_rlocked(); + mtx_assert(&interp_list_mtx, MA_OWNED); - memset(xbe, 0, sizeof(*xbe)); + bzero(xbe, sizeof(*xbe)); strlcpy(xbe->xbe_name, ibe->ibe_name, IBE_NAME_MAX); /* Copy interpreter string. Replace NULL breaks with space. */ @@ -440,14 +382,14 @@ imgact_binmisc_lookup_entry(char *name, ximgact_binmisc_entry_t *xbe) imgact_binmisc_entry_t *ibe; int error = 0; - interp_list_rlock(); + mtx_lock(&interp_list_mtx); if ((ibe = imgact_binmisc_find_entry(name)) == NULL) { - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); return (ENOENT); } error = imgact_binmisc_populate_xbe(xbe, ibe); - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); return (error); } @@ -462,12 +404,12 @@ imgact_binmisc_get_all_entries(struct sysctl_req *req) imgact_binmisc_entry_t *ibe; int error = 0, count; - interp_list_rlock(); + mtx_lock(&interp_list_mtx); count = interp_list_entry_count; /* Don't block in malloc() while holding lock. */ xbe = malloc(sizeof(*xbe) * count, M_BINMISC, M_NOWAIT|M_ZERO); if (!xbe) { - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); return (ENOMEM); } @@ -477,7 +419,7 @@ imgact_binmisc_get_all_entries(struct sysctl_req *req) if (error) break; } - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); if (!error) error = SYSCTL_OUT(req, xbe, sizeof(*xbe) * count); @@ -614,7 +556,7 @@ imgact_binmisc_find_interpreter(const char *image_header) int i; size_t sz; - interp_list_assert_rlocked(); + mtx_assert(&interp_list_mtx, MA_OWNED); SLIST_FOREACH(ibe, &interpreter_list, link) { if (!(IBF_ENABLED & ibe->ibe_flags)) @@ -651,15 +593,15 @@ imgact_binmisc_exec(struct image_params *imgp) char *s, *d; /* Do we have an interpreter for the given image header? */ - interp_list_rlock(); + mtx_lock(&interp_list_mtx); if ((ibe = imgact_binmisc_find_interpreter(image_header)) == NULL) { - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); return (-1); } /* No interpreter nesting allowed. */ if (imgp->interpreted & IMGACT_BINMISC) { - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); return (ENOEXEC); } @@ -707,7 +649,7 @@ imgact_binmisc_exec(struct image_params *imgp) default: /* Hmm... This shouldn't happen. */ - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); printf("%s: Unknown macro #%c sequence in " "interpreter string\n", KMOD_NAME, *(s + 1)); error = EINVAL; @@ -718,15 +660,12 @@ imgact_binmisc_exec(struct image_params *imgp) /* Check to make sure we won't overrun the stringspace. */ if (offset > imgp->args->stringspace) { - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); error = E2BIG; goto done; } - /* - * Make room for the interpreter. Doing an overlapping copy so - * bcopy() must be used. - */ + /* Make room for the interpreter */ bcopy(imgp->args->begin_argv, imgp->args->begin_argv + offset, imgp->args->endp - imgp->args->begin_argv); @@ -781,7 +720,7 @@ imgact_binmisc_exec(struct image_params *imgp) s++; } *d = '\0'; - interp_list_runlock(); + mtx_unlock(&interp_list_mtx); if (!error) imgp->interpreter_name = imgp->args->begin_argv; @@ -806,13 +745,13 @@ imgact_binmisc_fini(void *arg) imgact_binmisc_entry_t *ibe, *ibe_tmp; /* Free all the interpreters. */ - interp_list_wlock(); + mtx_lock(&interp_list_mtx); SLIST_FOREACH_SAFE(ibe, &interpreter_list, link, ibe_tmp) { SLIST_REMOVE(&interpreter_list, ibe, imgact_binmisc_entry, link); imgact_binmisc_destroy_entry(ibe); } - interp_list_wunlock(); + mtx_unlock(&interp_list_mtx); mtx_destroy(&interp_list_mtx); }