Date: Fri, 25 Mar 2011 22:11:54 +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-7@freebsd.org Subject: svn commit: r220012 - in stable/7/sys: kern sys Message-ID: <201103252211.p2PMBs1V095087@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: mdf Date: Fri Mar 25 22:11:54 2011 New Revision: 220012 URL: http://svn.freebsd.org/changeset/base/220012 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/7/sys/kern/kern_sysctl.c stable/7/sys/sys/sysctl.h Directory Properties: stable/7/sys/ (props changed) stable/7/sys/cddl/contrib/opensolaris/ (props changed) stable/7/sys/contrib/dev/acpica/ (props changed) stable/7/sys/contrib/pf/ (props changed) Modified: stable/7/sys/kern/kern_sysctl.c ============================================================================== --- stable/7/sys/kern/kern_sysctl.c Fri Mar 25 22:11:39 2011 (r220011) +++ stable/7/sys/kern/kern_sysctl.c Fri Mar 25 22:11:54 2011 (r220012) @@ -43,6 +43,7 @@ __FBSDID("$FreeBSD$"); #include "opt_mac.h" #include <sys/param.h> +#include <sys/fail.h> #include <sys/systm.h> #include <sys/kernel.h> #include <sys/sysctl.h> @@ -83,13 +84,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); @@ -103,7 +103,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); @@ -310,7 +310,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) { @@ -406,6 +406,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, @@ -578,7 +588,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++) @@ -619,7 +629,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); } @@ -637,7 +649,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); @@ -646,7 +658,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; @@ -662,7 +674,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++; @@ -678,7 +690,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, ""); @@ -689,7 +704,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; @@ -753,7 +768,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)); @@ -770,7 +787,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); @@ -828,8 +845,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 */ @@ -844,8 +859,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); @@ -865,16 +882,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); } @@ -888,13 +910,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); } @@ -1174,9 +1201,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); @@ -1315,7 +1342,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) { @@ -1334,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); } lsp = SYSCTL_CHILDREN(oid); @@ -1341,6 +1370,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); @@ -1360,7 +1391,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) @@ -1416,12 +1447,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/7/sys/sys/sysctl.h ============================================================================== --- stable/7/sys/sys/sysctl.h Fri Mar 25 22:11:39 2011 (r220011) +++ stable/7/sys/sys/sysctl.h Fri Mar 25 22:11:54 2011 (r220012) @@ -86,6 +86,7 @@ struct ctlname { #define CTLFLAG_TUN 0x00080000 /* Tunable variable */ #define CTLFLAG_MPSAFE 0x00040000 /* Handler is MP safe */ #define CTLFLAG_RDTUN (CTLFLAG_RD|CTLFLAG_TUN) +#define CTLFLAG_DYING 0x00010000 /* oid is being removed */ /* * Secure level. Note that CTLFLAG_SECURE == CTLFLAG_SECURE1. @@ -164,6 +165,7 @@ struct sysctl_oid { int (*oid_handler)(SYSCTL_HANDLER_ARGS); const char *oid_fmt; int oid_refcnt; + u_int oid_running; const char *oid_descr; }; @@ -219,7 +221,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, { 0 }, \ - nbr, kind, a1, a2, #name, handler, fmt, 0, __DESCR(descr) }; \ + nbr, kind, 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.p2PMBs1V095087>