Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 25 Mar 2011 22:11:39 +0000 (UTC)
From:      Matthew D Fleming <mdf@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org
Subject:   svn commit: r220011 - in stable/8/sys: kern sys
Message-ID:  <201103252211.p2PMBd0l095046@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: mdf
Date: Fri Mar 25 22:11:39 2011
New Revision: 220011
URL: http://svn.freebsd.org/changeset/base/220011

Log:
  MFC r216060.  This differs from the original commit in that it
  preserves the KBI size of struct sysctl_oid.  Also, on stable/8 the
  compiler thinks that 'len' in sysctl_sysctl_name2oid() is used
  uninitialized.
  
  Do not hold the sysctl lock across a call to the handler.  This fixes a
  general LOR issue where the sysctl lock had no good place in the
  hierarchy.  One specific instance is #284 on
  http://sources.zabbadoz.net/freebsd/lor.html .

Modified:
  stable/8/sys/kern/kern_sysctl.c
  stable/8/sys/sys/sysctl.h
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)

Modified: stable/8/sys/kern/kern_sysctl.c
==============================================================================
--- stable/8/sys/kern/kern_sysctl.c	Fri Mar 25 22:00:43 2011	(r220010)
+++ stable/8/sys/kern/kern_sysctl.c	Fri Mar 25 22:11:39 2011	(r220011)
@@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$");
 #include "opt_ktrace.h"
 
 #include <sys/param.h>
+#include <sys/fail.h>
 #include <sys/systm.h>
 #include <sys/kernel.h>
 #include <sys/sysctl.h>
@@ -85,13 +86,12 @@ static MALLOC_DEFINE(M_SYSCTLTMP, "sysct
 static struct sx sysctllock;
 static struct sx sysctlmemlock;
 
-#define	SYSCTL_SLOCK()		sx_slock(&sysctllock)
-#define	SYSCTL_SUNLOCK()	sx_sunlock(&sysctllock)
 #define	SYSCTL_XLOCK()		sx_xlock(&sysctllock)
 #define	SYSCTL_XUNLOCK()	sx_xunlock(&sysctllock)
 #define	SYSCTL_ASSERT_XLOCKED()	sx_assert(&sysctllock, SA_XLOCKED)
-#define	SYSCTL_ASSERT_LOCKED()	sx_assert(&sysctllock, SA_LOCKED)
 #define	SYSCTL_INIT()		sx_init(&sysctllock, "sysctl lock")
+#define	SYSCTL_SLEEP(ch, wmesg, timo)					\
+				sx_sleep(ch, &sysctllock, 0, wmesg, timo)
 
 static int sysctl_root(SYSCTL_HANDLER_ARGS);
 
@@ -105,7 +105,7 @@ sysctl_find_oidname(const char *name, st
 {
 	struct sysctl_oid *oidp;
 
-	SYSCTL_ASSERT_LOCKED();
+	SYSCTL_ASSERT_XLOCKED();
 	SLIST_FOREACH(oidp, list, oid_link) {
 		if (strcmp(oidp->oid_name, name) == 0) {
 			return (oidp);
@@ -312,7 +312,7 @@ sysctl_ctx_entry_find(struct sysctl_ctx_
 {
 	struct sysctl_ctx_entry *e;
 
-	SYSCTL_ASSERT_LOCKED();
+	SYSCTL_ASSERT_XLOCKED();
 	if (clist == NULL || oidp == NULL)
 		return(NULL);
 	TAILQ_FOREACH(e, clist, link) {
@@ -408,6 +408,16 @@ sysctl_remove_oid_locked(struct sysctl_o
 		}
 		sysctl_unregister_oid(oidp);
 		if (del) {
+			/*
+			 * Wait for all threads running the handler to drain.
+			 * This preserves the previous behavior when the
+			 * sysctl lock was held across a handler invocation,
+			 * and is necessary for module unload correctness.
+			 */
+			while (oidp->oid_running > 0) {
+				oidp->oid_kind |= CTLFLAG_DYING;
+				SYSCTL_SLEEP(&oidp->oid_running, "oidrm", 0);
+			}
 			if (oidp->oid_descr)
 				free((void *)(uintptr_t)(const void *)oidp->oid_descr, M_SYSCTLOID);
 			free((void *)(uintptr_t)(const void *)oidp->oid_name,
@@ -580,7 +590,7 @@ sysctl_sysctl_debug_dump_node(struct sys
 	int k;
 	struct sysctl_oid *oidp;
 
-	SYSCTL_ASSERT_LOCKED();
+	SYSCTL_ASSERT_XLOCKED();
 	SLIST_FOREACH(oidp, l, oid_link) {
 
 		for (k=0; k<i; k++)
@@ -621,7 +631,9 @@ sysctl_sysctl_debug(SYSCTL_HANDLER_ARGS)
 	error = priv_check(req->td, PRIV_SYSCTL_DEBUG);
 	if (error)
 		return (error);
+	SYSCTL_XLOCK();
 	sysctl_sysctl_debug_dump_node(&sysctl__children, 0);
+	SYSCTL_XUNLOCK();
 	return (ENOENT);
 }
 
@@ -639,7 +651,7 @@ sysctl_sysctl_name(SYSCTL_HANDLER_ARGS)
 	struct sysctl_oid_list *lsp = &sysctl__children, *lsp2;
 	char buf[10];
 
-	SYSCTL_ASSERT_LOCKED();
+	SYSCTL_XLOCK();
 	while (namelen) {
 		if (!lsp) {
 			snprintf(buf,sizeof(buf),"%d",*name);
@@ -648,7 +660,7 @@ sysctl_sysctl_name(SYSCTL_HANDLER_ARGS)
 			if (!error)
 				error = SYSCTL_OUT(req, buf, strlen(buf));
 			if (error)
-				return (error);
+				goto out;
 			namelen--;
 			name++;
 			continue;
@@ -664,7 +676,7 @@ sysctl_sysctl_name(SYSCTL_HANDLER_ARGS)
 				error = SYSCTL_OUT(req, oid->oid_name,
 					strlen(oid->oid_name));
 			if (error)
-				return (error);
+				goto out;
 
 			namelen--;
 			name++;
@@ -680,7 +692,10 @@ sysctl_sysctl_name(SYSCTL_HANDLER_ARGS)
 		}
 		lsp = lsp2;
 	}
-	return (SYSCTL_OUT(req, "", 1));
+	error = SYSCTL_OUT(req, "", 1);
+ out:
+	SYSCTL_XUNLOCK();
+	return (error);
 }
 
 static SYSCTL_NODE(_sysctl, 1, name, CTLFLAG_RD, sysctl_sysctl_name, "");
@@ -691,7 +706,7 @@ sysctl_sysctl_next_ls(struct sysctl_oid_
 {
 	struct sysctl_oid *oidp;
 
-	SYSCTL_ASSERT_LOCKED();
+	SYSCTL_ASSERT_XLOCKED();
 	*len = level;
 	SLIST_FOREACH(oidp, lsp, oid_link) {
 		*next = oidp->oid_number;
@@ -755,7 +770,9 @@ sysctl_sysctl_next(SYSCTL_HANDLER_ARGS)
 	struct sysctl_oid_list *lsp = &sysctl__children;
 	int newoid[CTL_MAXNAME];
 
+	SYSCTL_XLOCK();
 	i = sysctl_sysctl_next_ls(lsp, name, namelen, newoid, &j, 1, &oid);
+	SYSCTL_XUNLOCK();
 	if (i)
 		return (ENOENT);
 	error = SYSCTL_OUT(req, newoid, j * sizeof (int));
@@ -772,7 +789,7 @@ name2oid(char *name, int *oid, int *len,
 	struct sysctl_oid_list *lsp = &sysctl__children;
 	char *p;
 
-	SYSCTL_ASSERT_LOCKED();
+	SYSCTL_ASSERT_XLOCKED();
 
 	if (!*name)
 		return (ENOENT);
@@ -830,8 +847,6 @@ sysctl_sysctl_name2oid(SYSCTL_HANDLER_AR
 	int error, oid[CTL_MAXNAME], len;
 	struct sysctl_oid *op = 0;
 
-	SYSCTL_ASSERT_LOCKED();
-
 	if (!req->newlen) 
 		return (ENOENT);
 	if (req->newlen >= MAXPATHLEN)	/* XXX arbitrary, undocumented */
@@ -846,8 +861,10 @@ sysctl_sysctl_name2oid(SYSCTL_HANDLER_AR
 	}
 
 	p [req->newlen] = '\0';
-
+	len = 0;
+	SYSCTL_XLOCK();
 	error = name2oid(p, oid, &len, &op);
+	SYSCTL_XUNLOCK();
 
 	free(p, M_SYSCTL);
 
@@ -867,16 +884,21 @@ sysctl_sysctl_oidfmt(SYSCTL_HANDLER_ARGS
 	struct sysctl_oid *oid;
 	int error;
 
+	SYSCTL_XLOCK();
 	error = sysctl_find_oid(arg1, arg2, &oid, NULL, req);
 	if (error)
-		return (error);
+		goto out;
 
-	if (!oid->oid_fmt)
-		return (ENOENT);
+	if (oid->oid_fmt == NULL) {
+		error = ENOENT;
+		goto out;
+	}
 	error = SYSCTL_OUT(req, &oid->oid_kind, sizeof(oid->oid_kind));
 	if (error)
-		return (error);
+		goto out;
 	error = SYSCTL_OUT(req, oid->oid_fmt, strlen(oid->oid_fmt) + 1);
+ out:
+	SYSCTL_XUNLOCK();
 	return (error);
 }
 
@@ -890,13 +912,18 @@ sysctl_sysctl_oiddescr(SYSCTL_HANDLER_AR
 	struct sysctl_oid *oid;
 	int error;
 
+	SYSCTL_XLOCK();
 	error = sysctl_find_oid(arg1, arg2, &oid, NULL, req);
 	if (error)
-		return (error);
+		goto out;
 
-	if (!oid->oid_descr)
-		return (ENOENT);
+	if (oid->oid_descr == NULL) {
+		error = ENOENT;
+		goto out;
+	}
 	error = SYSCTL_OUT(req, oid->oid_descr, strlen(oid->oid_descr) + 1);
+ out:
+	SYSCTL_XUNLOCK();
 	return (error);
 }
 
@@ -1176,9 +1203,9 @@ kernel_sysctl(struct thread *td, int *na
 	req.newfunc = sysctl_new_kernel;
 	req.lock = REQ_LOCKED;
 
-	SYSCTL_SLOCK();
+	SYSCTL_XLOCK();
 	error = sysctl_root(0, name, namelen, &req);
-	SYSCTL_SUNLOCK();
+	SYSCTL_XUNLOCK();
 
 	if (req.lock == REQ_WIRED && req.validlen > 0)
 		vsunlock(req.oldptr, req.validlen);
@@ -1306,7 +1333,7 @@ sysctl_find_oid(int *name, u_int namelen
 	struct sysctl_oid *oid;
 	int indx;
 
-	SYSCTL_ASSERT_LOCKED();
+	SYSCTL_ASSERT_XLOCKED();
 	lsp = &sysctl__children;
 	indx = 0;
 	while (indx < CTL_MAXNAME) {
@@ -1325,6 +1352,8 @@ sysctl_find_oid(int *name, u_int namelen
 				*noid = oid;
 				if (nindx != NULL)
 					*nindx = indx;
+				KASSERT((oid->oid_kind & CTLFLAG_DYING) == 0,
+				    ("%s found DYING node %p", __func__, oid));
 				return (0);
 			}
 			lsp = SYSCTL_CHILDREN(oid);
@@ -1332,6 +1361,8 @@ sysctl_find_oid(int *name, u_int namelen
 			*noid = oid;
 			if (nindx != NULL)
 				*nindx = indx;
+			KASSERT((oid->oid_kind & CTLFLAG_DYING) == 0,
+			    ("%s found DYING node %p", __func__, oid));
 			return (0);
 		} else {
 			return (ENOTDIR);
@@ -1351,7 +1382,7 @@ sysctl_root(SYSCTL_HANDLER_ARGS)
 	struct sysctl_oid *oid;
 	int error, indx, lvl;
 
-	SYSCTL_ASSERT_LOCKED();
+	SYSCTL_ASSERT_XLOCKED();
 
 	error = sysctl_find_oid(arg1, arg2, &oid, &indx, req);
 	if (error)
@@ -1415,12 +1446,21 @@ sysctl_root(SYSCTL_HANDLER_ARGS)
 	if (error != 0)
 		return (error);
 #endif
+	oid->oid_running++;
+	SYSCTL_XUNLOCK();
+
 	if (!(oid->oid_kind & CTLFLAG_MPSAFE))
 		mtx_lock(&Giant);
 	error = oid->oid_handler(oid, arg1, arg2, req);
 	if (!(oid->oid_kind & CTLFLAG_MPSAFE))
 		mtx_unlock(&Giant);
 
+	KFAIL_POINT_ERROR(_debug_fail_point, sysctl_running, error);
+
+	SYSCTL_XLOCK();
+	oid->oid_running--;
+	if (oid->oid_running == 0 && (oid->oid_kind & CTLFLAG_DYING) != 0)
+		wakeup(&oid->oid_running);
 	return (error);
 }
 
@@ -1520,9 +1560,9 @@ userland_sysctl(struct thread *td, int *
 	for (;;) {
 		req.oldidx = 0;
 		req.newidx = 0;
-		SYSCTL_SLOCK();
+		SYSCTL_XLOCK();
 		error = sysctl_root(0, name, namelen, &req);
-		SYSCTL_SUNLOCK();
+		SYSCTL_XUNLOCK();
 		if (error != EAGAIN)
 			break;
 		uio_yield();

Modified: stable/8/sys/sys/sysctl.h
==============================================================================
--- stable/8/sys/sys/sysctl.h	Fri Mar 25 22:00:43 2011	(r220010)
+++ stable/8/sys/sys/sysctl.h	Fri Mar 25 22:11:39 2011	(r220011)
@@ -87,6 +87,7 @@ struct ctlname {
 #define CTLFLAG_MPSAFE	0x00040000	/* Handler is MP safe */
 #define CTLFLAG_VNET	0x00020000	/* Prisons with vnet can fiddle */
 #define CTLFLAG_RDTUN	(CTLFLAG_RD|CTLFLAG_TUN)
+#define	CTLFLAG_DYING	0x00010000	/* oid is being removed */
 
 /*
  * Secure level.   Note that CTLFLAG_SECURE == CTLFLAG_SECURE1.  
@@ -164,7 +165,8 @@ struct sysctl_oid {
 	const char	*oid_name;
 	int 		(*oid_handler)(SYSCTL_HANDLER_ARGS);
 	const char	*oid_fmt;
-	int		oid_refcnt;
+	int16_t		oid_refcnt;
+	uint16_t	oid_running;
 	const char	*oid_descr;
 };
 
@@ -224,7 +226,7 @@ TAILQ_HEAD(sysctl_ctx_list, sysctl_ctx_e
 #define SYSCTL_OID(parent, nbr, name, kind, a1, a2, handler, fmt, descr) \
 	static struct sysctl_oid sysctl__##parent##_##name = {		 \
 		&sysctl_##parent##_children, { NULL }, nbr, kind,	 \
-		a1, a2, #name, handler, fmt, 0, __DESCR(descr) };     \
+		a1, a2, #name, handler, fmt, 0, 0, __DESCR(descr) };	 \
 	DATA_SET(sysctl_set, sysctl__##parent##_##name)
 
 #define SYSCTL_ADD_OID(ctx, parent, nbr, name, kind, a1, a2, handler, fmt, descr) \



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201103252211.p2PMBd0l095046>