This change replaces the mutex with a sx lock for the interpreter list to

avoid the problem of holding a non-sleep lock during a page fault as
reported by witness. It also uses atomics where possible to avoid having
to acquire the exclusive lock. In addition, it consistently uses
memset()/memcpy() instead of bzero()/bcopy().

Differential Revision:	https://reviews.freebsd.org/D1971
Submitted by:	sson
Reviewed by:	jhb

1 files changed, 40 insertions, 40 deletions

diff --git a/sys/kern/imgact_binmisc.c b/sys/kern/imgact_binmisc.c
index 5f324d3c3e16..4c65c66238bd 100644
--- a/sys/kern/imgact_binmisc.c
+++ b/sys/kern/imgact_binmisc.c
@@ -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.
@@ -96,7 +99,7 @@ static SLIST_HEAD(, imgact_binmisc_entry) interpreter_list =
 
 static int interp_list_entry_count = 0;
 
-static struct mtx interp_list_mtx;
+static struct sx interp_list_sx;
 
 int imgact_binmisc_exec(struct image_params *imgp);
 
@@ -111,7 +114,7 @@ imgact_binmisc_populate_interp(char *str, imgact_binmisc_entry_t *ibe)
 	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 +155,6 @@ 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);
@@ -203,7 +204,7 @@ imgact_binmisc_find_entry(char *name)
 {
 	imgact_binmisc_entry_t *ibe;
 
-	mtx_assert(&interp_list_mtx, MA_OWNED);
+	sx_assert(&interp_list_sx, SA_LOCKED);
 
 	SLIST_FOREACH(ibe, &interpreter_list, link) {
 		if (strncmp(name, ibe->ibe_name, IBE_NAME_MAX) == 0)
@@ -260,21 +261,20 @@ imgact_binmisc_add_entry(ximgact_binmisc_entry_t *xbe)
 		}
 	}
 
-	mtx_lock(&interp_list_mtx);
+	sx_xlock(&interp_list_sx);
 	if (imgact_binmisc_find_entry(xbe->xbe_name) != NULL) {
-		mtx_unlock(&interp_list_mtx);
+		sx_xunlock(&interp_list_sx);
 		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);
 	SLIST_INSERT_HEAD(&interpreter_list, ibe, link);
 	interp_list_entry_count++;
-	mtx_unlock(&interp_list_mtx);
+	sx_xunlock(&interp_list_sx);
 
 	return (0);
 }
@@ -288,14 +288,14 @@ imgact_binmisc_remove_entry(char *name)
 {
 	imgact_binmisc_entry_t *ibe;
 
-	mtx_lock(&interp_list_mtx);
+	sx_xlock(&interp_list_sx);
 	if ((ibe = imgact_binmisc_find_entry(name)) == NULL) {
-		mtx_unlock(&interp_list_mtx);
+		sx_xunlock(&interp_list_sx);
 		return (ENOENT);
 	}
 	SLIST_REMOVE(&interpreter_list, ibe, imgact_binmisc_entry, link);
 	interp_list_entry_count--;
-	mtx_unlock(&interp_list_mtx);
+	sx_xunlock(&interp_list_sx);
 
 	imgact_binmisc_destroy_entry(ibe);
 
@@ -311,14 +311,14 @@ imgact_binmisc_disable_entry(char *name)
 {
 	imgact_binmisc_entry_t *ibe;
 
-	mtx_lock(&interp_list_mtx);
+	sx_slock(&interp_list_sx);
 	if ((ibe = imgact_binmisc_find_entry(name)) == NULL) {
-		mtx_unlock(&interp_list_mtx);
+		sx_sunlock(&interp_list_sx);
 		return (ENOENT);
 	}
 
 	ibe->ibe_flags &= ~IBF_ENABLED;
-	mtx_unlock(&interp_list_mtx);
+	sx_sunlock(&interp_list_sx);
 
 	return (0);
 }
@@ -332,14 +332,14 @@ imgact_binmisc_enable_entry(char *name)
 {
 	imgact_binmisc_entry_t *ibe;
 
-	mtx_lock(&interp_list_mtx);
+	sx_slock(&interp_list_sx);
 	if ((ibe = imgact_binmisc_find_entry(name)) == NULL) {
-		mtx_unlock(&interp_list_mtx);
+		sx_sunlock(&interp_list_sx);
 		return (ENOENT);
 	}
 
-	ibe->ibe_flags |= IBF_ENABLED;
-	mtx_unlock(&interp_list_mtx);
+	atomic_set_32(&ibe->ibe_flags, IBF_ENABLED);
+	sx_sunlock(&interp_list_sx);
 
 	return (0);
 }
@@ -350,9 +350,9 @@ imgact_binmisc_populate_xbe(ximgact_binmisc_entry_t *xbe,
 {
 	uint32_t i;
 
-	mtx_assert(&interp_list_mtx, MA_OWNED);
+	sx_assert(&interp_list_sx, SA_LOCKED);
 
-	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 +382,14 @@ imgact_binmisc_lookup_entry(char *name, ximgact_binmisc_entry_t *xbe)
 	imgact_binmisc_entry_t *ibe;
 	int error = 0;
 
-	mtx_lock(&interp_list_mtx);
+	sx_slock(&interp_list_sx);
 	if ((ibe = imgact_binmisc_find_entry(name)) == NULL) {
-		mtx_unlock(&interp_list_mtx);
+		sx_sunlock(&interp_list_sx);
 		return (ENOENT);
 	}
 
 	error = imgact_binmisc_populate_xbe(xbe, ibe);
-	mtx_unlock(&interp_list_mtx);
+	sx_sunlock(&interp_list_sx);
 
 	return (error);
 }
@@ -404,12 +404,12 @@ imgact_binmisc_get_all_entries(struct sysctl_req *req)
 	imgact_binmisc_entry_t *ibe;
 	int error = 0, count;
 
-	mtx_lock(&interp_list_mtx);
+	sx_slock(&interp_list_sx);
 	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);
+		sx_sunlock(&interp_list_sx);
 		return (ENOMEM);
 	}
 
@@ -419,7 +419,7 @@ imgact_binmisc_get_all_entries(struct sysctl_req *req)
 		if (error)
 			break;
 	}
-	mtx_unlock(&interp_list_mtx);
+	sx_sunlock(&interp_list_sx);
 
 	if (!error)
 		error = SYSCTL_OUT(req, xbe, sizeof(*xbe) * count);
@@ -556,7 +556,7 @@ imgact_binmisc_find_interpreter(const char *image_header)
 	int i;
 	size_t sz;
 
-	mtx_assert(&interp_list_mtx, MA_OWNED);
+	sx_assert(&interp_list_sx, SA_LOCKED);
 
 	SLIST_FOREACH(ibe, &interpreter_list, link) {
 		if (!(IBF_ENABLED & ibe->ibe_flags))
@@ -593,15 +593,15 @@ imgact_binmisc_exec(struct image_params *imgp)
 	char *s, *d;
 
 	/* Do we have an interpreter for the given image header? */
-	mtx_lock(&interp_list_mtx);
+	sx_slock(&interp_list_sx);
 	if ((ibe = imgact_binmisc_find_interpreter(image_header)) == NULL) {
-		mtx_unlock(&interp_list_mtx);
+		sx_sunlock(&interp_list_sx);
 		return (-1);
 	}
 
 	/* No interpreter nesting allowed. */
 	if (imgp->interpreted & IMGACT_BINMISC) {
-		mtx_unlock(&interp_list_mtx);
+		sx_sunlock(&interp_list_sx);
 		return (ENOEXEC);
 	}
 
@@ -649,7 +649,7 @@ imgact_binmisc_exec(struct image_params *imgp)
 
 		default:
 			/* Hmm... This shouldn't happen. */
-			mtx_unlock(&interp_list_mtx);
+			sx_sunlock(&interp_list_sx);
 			printf("%s: Unknown macro #%c sequence in "
 			    "interpreter string\n", KMOD_NAME, *(s + 1));
 			error = EINVAL;
@@ -660,7 +660,7 @@ imgact_binmisc_exec(struct image_params *imgp)
 
 	/* Check to make sure we won't overrun the stringspace. */
 	if (offset > imgp->args->stringspace) {
-		mtx_unlock(&interp_list_mtx);
+		sx_sunlock(&interp_list_sx);
 		error = E2BIG;
 		goto done;
 	}
@@ -720,7 +720,7 @@ imgact_binmisc_exec(struct image_params *imgp)
 		s++;
 	}
 	*d = '\0';
-	mtx_unlock(&interp_list_mtx);
+	sx_sunlock(&interp_list_sx);
 
 	if (!error)
 		imgp->interpreter_name = imgp->args->begin_argv;
@@ -736,7 +736,7 @@ static void
 imgact_binmisc_init(void *arg)
 {
 
-	mtx_init(&interp_list_mtx, KMOD_NAME, NULL, MTX_DEF);
+	sx_init(&interp_list_sx, KMOD_NAME);
 }
 
 static void
@@ -745,15 +745,15 @@ imgact_binmisc_fini(void *arg)
 	imgact_binmisc_entry_t *ibe, *ibe_tmp;
 
 	/* Free all the interpreters. */
-	mtx_lock(&interp_list_mtx);
+	sx_xlock(&interp_list_sx);
 	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);
+	sx_xunlock(&interp_list_sx);
 
-	mtx_destroy(&interp_list_mtx);
+	sx_destroy(&interp_list_sx);
 }
 
 SYSINIT(imgact_binmisc, SI_SUB_EXEC, SI_ORDER_MIDDLE, imgact_binmisc_init, 0);