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>