Date: Mon, 29 Nov 2010 18:18:07 +0000 (UTC) From: Matthew D Fleming <mdf@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r216060 - in head/sys: kern sys Message-ID: <201011291818.oATII7df065248@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: mdf Date: Mon Nov 29 18:18:07 2010 New Revision: 216060 URL: http://svn.freebsd.org/changeset/base/216060 Log: 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 . Reviewed by: jhb MFC after: 1 month X-MFC-note: split oid_refcnt field for oid_running to preserve KBI Modified: head/sys/kern/kern_sysctl.c head/sys/sys/sysctl.h Modified: head/sys/kern/kern_sysctl.c ============================================================================== --- head/sys/kern/kern_sysctl.c Mon Nov 29 18:18:00 2010 (r216059) +++ head/sys/kern/kern_sysctl.c Mon Nov 29 18:18:07 2010 (r216060) @@ -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> @@ -86,13 +87,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); @@ -106,7 +106,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); @@ -313,7 +313,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) { @@ -409,6 +409,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, @@ -581,7 +591,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++) @@ -622,7 +632,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); } @@ -640,7 +652,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); @@ -649,7 +661,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; @@ -665,7 +677,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++; @@ -681,7 +693,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, ""); @@ -692,7 +707,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; @@ -756,7 +771,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)); @@ -773,7 +790,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); @@ -831,8 +848,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 */ @@ -848,7 +863,9 @@ sysctl_sysctl_name2oid(SYSCTL_HANDLER_AR p [req->newlen] = '\0'; + SYSCTL_XLOCK(); error = name2oid(p, oid, &len, &op); + SYSCTL_XUNLOCK(); free(p, M_SYSCTL); @@ -868,16 +885,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); } @@ -891,13 +913,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); } @@ -1177,9 +1204,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); @@ -1307,7 +1334,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) { @@ -1326,6 +1353,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); @@ -1333,6 +1362,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); @@ -1352,7 +1383,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); } @@ -1521,9 +1561,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: head/sys/sys/sysctl.h ============================================================================== --- head/sys/sys/sysctl.h Mon Nov 29 18:18:00 2010 (r216059) +++ head/sys/sys/sysctl.h Mon Nov 29 18:18:07 2010 (r216060) @@ -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. @@ -163,6 +164,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; }; @@ -222,7 +224,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?201011291818.oATII7df065248>