Date: Fri, 5 Jun 2015 16:21:44 +0000 (UTC) From: Sean Bruno <sbruno@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r284029 - head/sys/kern Message-ID: <201506051621.t55GLiKp095005@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: sbruno Date: Fri Jun 5 16:21:43 2015 New Revision: 284029 URL: https://svnweb.freebsd.org/changeset/base/284029 Log: This change uses a reader count instead of holding the mutex for the interpreter list to avoid the problem of holding a non-sleep lock during a page fault as reported by witness. In addition, it consistently uses memset()/memcpy() instead of bzero()/bcopy() except in the case where bcopy() is required (i.e. overlapping copy). Differential Revision: https://reviews.freebsd.org/D2123 Submitted by: sson MFC after: 2 weeks Relnotes: Yes Modified: head/sys/kern/imgact_binmisc.c Modified: head/sys/kern/imgact_binmisc.c ============================================================================== --- head/sys/kern/imgact_binmisc.c Fri Jun 5 16:02:24 2015 (r284028) +++ head/sys/kern/imgact_binmisc.c Fri Jun 5 16:21:43 2015 (r284029) @@ -1,5 +1,5 @@ /*- - * Copyright (c) 2013, Stacey D. Son + * Copyright (c) 2013-15, Stacey D. Son * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -41,6 +41,9 @@ __FBSDID("$FreeBSD$"); #include <sys/malloc.h> #include <sys/mutex.h> #include <sys/sysctl.h> +#include <sys/sx.h> + +#include <machine/atomic.h> /** * Miscellaneous binary interpreter image activator. @@ -95,11 +98,68 @@ static SLIST_HEAD(, imgact_binmisc_entry 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. @@ -111,7 +171,7 @@ imgact_binmisc_populate_interp(char *str char t[IBE_INTERP_LEN_MAX]; char *sp, *tp; - bzero(t, sizeof(t)); + memset(t, 0, sizeof(t)); /* * Normalize interpreter string. Replace white space between args with @@ -152,8 +212,6 @@ imgact_binmisc_new_entry(ximgact_binmisc 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); @@ -203,7 +261,7 @@ imgact_binmisc_find_entry(char *name) { imgact_binmisc_entry_t *ibe; - mtx_assert(&interp_list_mtx, MA_OWNED); + interp_list_assert_locked(); SLIST_FOREACH(ibe, &interpreter_list, link) { if (strncmp(name, ibe->ibe_name, IBE_NAME_MAX) == 0) @@ -260,21 +318,21 @@ imgact_binmisc_add_entry(ximgact_binmisc } } - 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); - + /* Preallocate a new entry. */ ibe = imgact_binmisc_new_entry(xbe); if (!ibe) return (ENOMEM); - mtx_lock(&interp_list_mtx); + interp_list_wlock(); + if (imgact_binmisc_find_entry(xbe->xbe_name) != NULL) { + interp_list_wunlock(); + imgact_binmisc_destroy_entry(ibe); + return (EEXIST); + } + SLIST_INSERT_HEAD(&interpreter_list, ibe, link); interp_list_entry_count++; - mtx_unlock(&interp_list_mtx); + interp_list_wunlock(); return (0); } @@ -288,14 +346,14 @@ imgact_binmisc_remove_entry(char *name) { imgact_binmisc_entry_t *ibe; - mtx_lock(&interp_list_mtx); + interp_list_wlock(); if ((ibe = imgact_binmisc_find_entry(name)) == NULL) { - mtx_unlock(&interp_list_mtx); + interp_list_wunlock(); return (ENOENT); } SLIST_REMOVE(&interpreter_list, ibe, imgact_binmisc_entry, link); interp_list_entry_count--; - mtx_unlock(&interp_list_mtx); + interp_list_wunlock(); imgact_binmisc_destroy_entry(ibe); @@ -311,14 +369,14 @@ imgact_binmisc_disable_entry(char *name) { imgact_binmisc_entry_t *ibe; - mtx_lock(&interp_list_mtx); + interp_list_rlock(); if ((ibe = imgact_binmisc_find_entry(name)) == NULL) { - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); return (ENOENT); } - ibe->ibe_flags &= ~IBF_ENABLED; - mtx_unlock(&interp_list_mtx); + atomic_clear_32(&ibe->ibe_flags, IBF_ENABLED); + interp_list_runlock(); return (0); } @@ -332,14 +390,14 @@ imgact_binmisc_enable_entry(char *name) { imgact_binmisc_entry_t *ibe; - mtx_lock(&interp_list_mtx); + interp_list_rlock(); if ((ibe = imgact_binmisc_find_entry(name)) == NULL) { - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); return (ENOENT); } - ibe->ibe_flags |= IBF_ENABLED; - mtx_unlock(&interp_list_mtx); + atomic_set_32(&ibe->ibe_flags, IBF_ENABLED); + interp_list_runlock(); return (0); } @@ -350,9 +408,9 @@ imgact_binmisc_populate_xbe(ximgact_binm { uint32_t i; - mtx_assert(&interp_list_mtx, MA_OWNED); + interp_list_assert_rlocked(); - bzero(xbe, sizeof(*xbe)); + memset(xbe, 0, sizeof(*xbe)); strlcpy(xbe->xbe_name, ibe->ibe_name, IBE_NAME_MAX); /* Copy interpreter string. Replace NULL breaks with space. */ @@ -382,14 +440,14 @@ imgact_binmisc_lookup_entry(char *name, imgact_binmisc_entry_t *ibe; int error = 0; - mtx_lock(&interp_list_mtx); + interp_list_rlock(); if ((ibe = imgact_binmisc_find_entry(name)) == NULL) { - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); return (ENOENT); } error = imgact_binmisc_populate_xbe(xbe, ibe); - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); return (error); } @@ -404,12 +462,12 @@ imgact_binmisc_get_all_entries(struct sy imgact_binmisc_entry_t *ibe; int error = 0, count; - mtx_lock(&interp_list_mtx); + interp_list_rlock(); 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) { - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); return (ENOMEM); } @@ -419,7 +477,7 @@ imgact_binmisc_get_all_entries(struct sy if (error) break; } - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); if (!error) error = SYSCTL_OUT(req, xbe, sizeof(*xbe) * count); @@ -556,7 +614,7 @@ imgact_binmisc_find_interpreter(const ch int i; size_t sz; - mtx_assert(&interp_list_mtx, MA_OWNED); + interp_list_assert_rlocked(); SLIST_FOREACH(ibe, &interpreter_list, link) { if (!(IBF_ENABLED & ibe->ibe_flags)) @@ -593,15 +651,15 @@ imgact_binmisc_exec(struct image_params char *s, *d; /* Do we have an interpreter for the given image header? */ - mtx_lock(&interp_list_mtx); + interp_list_rlock(); if ((ibe = imgact_binmisc_find_interpreter(image_header)) == NULL) { - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); return (-1); } /* No interpreter nesting allowed. */ if (imgp->interpreted & IMGACT_BINMISC) { - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); return (ENOEXEC); } @@ -649,7 +707,7 @@ imgact_binmisc_exec(struct image_params default: /* Hmm... This shouldn't happen. */ - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); printf("%s: Unknown macro #%c sequence in " "interpreter string\n", KMOD_NAME, *(s + 1)); error = EINVAL; @@ -660,12 +718,15 @@ imgact_binmisc_exec(struct image_params /* Check to make sure we won't overrun the stringspace. */ if (offset > imgp->args->stringspace) { - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); error = E2BIG; goto done; } - /* Make room for the interpreter */ + /* + * Make room for the interpreter. Doing an overlapping copy so + * bcopy() must be used. + */ bcopy(imgp->args->begin_argv, imgp->args->begin_argv + offset, imgp->args->endp - imgp->args->begin_argv); @@ -720,7 +781,7 @@ imgact_binmisc_exec(struct image_params s++; } *d = '\0'; - mtx_unlock(&interp_list_mtx); + interp_list_runlock(); if (!error) imgp->interpreter_name = imgp->args->begin_argv; @@ -745,13 +806,13 @@ imgact_binmisc_fini(void *arg) imgact_binmisc_entry_t *ibe, *ibe_tmp; /* Free all the interpreters. */ - mtx_lock(&interp_list_mtx); + interp_list_wlock(); SLIST_FOREACH_SAFE(ibe, &interpreter_list, link, ibe_tmp) { SLIST_REMOVE(&interpreter_list, ibe, imgact_binmisc_entry, link); imgact_binmisc_destroy_entry(ibe); } - mtx_unlock(&interp_list_mtx); + interp_list_wunlock(); mtx_destroy(&interp_list_mtx); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201506051621.t55GLiKp095005>