Date: Mon, 29 Dec 2008 12:58:45 +0000 (UTC) From: Ed Schouten <ed@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r186564 - in head/sys: compat/freebsd32 compat/linux i386/ibcs2 kern Message-ID: <200812291258.mBTCwjDv057880@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: ed Date: Mon Dec 29 12:58:45 2008 New Revision: 186564 URL: http://svn.freebsd.org/changeset/base/186564 Log: Push down Giant inside sysctl. Also add some more assertions to the code. In the existing code we didn't really enforce that callers hold Giant before calling userland_sysctl(), even though there is no guarantee it is safe. Fix this by just placing Giant locks around the call to the oid handler. This also means we only pick up Giant for a very short period of time. Maybe we should add MPSAFE flags to sysctl or phase it out all together. I've also added SYSCTL_LOCK_ASSERT(). We have to make sure sysctl_root() and name2oid() are called with the sysctl lock held. Reviewed by: Jille Timmermans <jille quis cx> Modified: head/sys/compat/freebsd32/freebsd32_misc.c head/sys/compat/linux/linux_misc.c head/sys/i386/ibcs2/ibcs2_sysi86.c head/sys/kern/kern_sysctl.c head/sys/kern/kern_xxx.c Modified: head/sys/compat/freebsd32/freebsd32_misc.c ============================================================================== --- head/sys/compat/freebsd32/freebsd32_misc.c Mon Dec 29 12:45:11 2008 (r186563) +++ head/sys/compat/freebsd32/freebsd32_misc.c Mon Dec 29 12:58:45 2008 (r186564) @@ -2019,7 +2019,6 @@ freebsd32_sysctl(struct thread *td, stru error = copyin(uap->name, name, uap->namelen * sizeof(int)); if (error) return (error); - mtx_lock(&Giant); if (uap->oldlenp) oldlen = fuword32(uap->oldlenp); else @@ -2028,12 +2027,10 @@ freebsd32_sysctl(struct thread *td, stru uap->old, &oldlen, 1, uap->new, uap->newlen, &j, SCTL_MASK32); if (error && error != ENOMEM) - goto done2; + return (error); if (uap->oldlenp) suword32(uap->oldlenp, j); -done2: - mtx_unlock(&Giant); - return (error); + return (0); } int Modified: head/sys/compat/linux/linux_misc.c ============================================================================== --- head/sys/compat/linux/linux_misc.c Mon Dec 29 12:45:11 2008 (r186563) +++ head/sys/compat/linux/linux_misc.c Mon Dec 29 12:58:45 2008 (r186564) @@ -1682,7 +1682,6 @@ int linux_sethostname(struct thread *td, struct linux_sethostname_args *args) { int name[2]; - int error; #ifdef DEBUG if (ldebug(sethostname)) @@ -1691,18 +1690,14 @@ linux_sethostname(struct thread *td, str name[0] = CTL_KERN; name[1] = KERN_HOSTNAME; - mtx_lock(&Giant); - error = userland_sysctl(td, name, 2, 0, 0, 0, args->hostname, - args->len, 0, 0); - mtx_unlock(&Giant); - return (error); + return (userland_sysctl(td, name, 2, 0, 0, 0, args->hostname, + args->len, 0, 0)); } int linux_setdomainname(struct thread *td, struct linux_setdomainname_args *args) { int name[2]; - int error; #ifdef DEBUG if (ldebug(setdomainname)) @@ -1711,11 +1706,8 @@ linux_setdomainname(struct thread *td, s name[0] = CTL_KERN; name[1] = KERN_NISDOMAINNAME; - mtx_lock(&Giant); - error = userland_sysctl(td, name, 2, 0, 0, 0, args->name, - args->len, 0, 0); - mtx_unlock(&Giant); - return (error); + return (userland_sysctl(td, name, 2, 0, 0, 0, args->name, + args->len, 0, 0)); } int Modified: head/sys/i386/ibcs2/ibcs2_sysi86.c ============================================================================== --- head/sys/i386/ibcs2/ibcs2_sysi86.c Mon Dec 29 12:45:11 2008 (r186563) +++ head/sys/i386/ibcs2/ibcs2_sysi86.c Mon Dec 29 12:58:45 2008 (r186564) @@ -74,15 +74,11 @@ ibcs2_sysi86(struct thread *td, struct i case SETNAME: { /* set hostname given string w/ len <= 7 chars */ int name[2]; - int error; name[0] = CTL_KERN; name[1] = KERN_HOSTNAME; - mtx_lock(&Giant); - error = userland_sysctl(td, name, 2, 0, 0, 0, - args->arg, 7, 0, 0); - mtx_unlock(&Giant); - return (error); + return (userland_sysctl(td, name, 2, 0, 0, 0, + args->arg, 7, 0, 0)); } case SI86_MEM: /* size of physical memory */ Modified: head/sys/kern/kern_sysctl.c ============================================================================== --- head/sys/kern/kern_sysctl.c Mon Dec 29 12:45:11 2008 (r186563) +++ head/sys/kern/kern_sysctl.c Mon Dec 29 12:58:45 2008 (r186564) @@ -71,6 +71,7 @@ static struct sx sysctllock; #define SYSCTL_LOCK() sx_xlock(&sysctllock) #define SYSCTL_UNLOCK() sx_xunlock(&sysctllock) +#define SYSCTL_LOCK_ASSERT() sx_assert(&sysctllock, SX_XLOCKED) #define SYSCTL_INIT() sx_init(&sysctllock, "sysctl lock") static int sysctl_root(SYSCTL_HANDLER_ARGS); @@ -686,6 +687,8 @@ name2oid (char *name, int *oid, int *len struct sysctl_oid_list *lsp = &sysctl__children; char *p; + SYSCTL_LOCK_ASSERT(); + if (!*name) return (ENOENT); @@ -742,6 +745,8 @@ sysctl_sysctl_name2oid(SYSCTL_HANDLER_AR int error, oid[CTL_MAXNAME], len; struct sysctl_oid *op = 0; + SYSCTL_LOCK_ASSERT(); + if (!req->newlen) return (ENOENT); if (req->newlen >= MAXPATHLEN) /* XXX arbitrary, undocumented */ @@ -1086,14 +1091,12 @@ kernel_sysctl(struct thread *td, int *na req.lock = REQ_LOCKED; SYSCTL_LOCK(); - error = sysctl_root(0, name, namelen, &req); + SYSCTL_UNLOCK(); if (req.lock == REQ_WIRED && req.validlen > 0) vsunlock(req.oldptr, req.validlen); - SYSCTL_UNLOCK(); - if (error && error != ENOMEM) return (error); @@ -1118,6 +1121,11 @@ kernel_sysctlbyname(struct thread *td, c oid[1] = 3; /* name2oid */ oidlen = sizeof(oid); + /* + * XXX: Prone to a possible race condition between lookup and + * execution? Maybe put locking around it? + */ + error = kernel_sysctl(td, oid, 2, oid, &oidlen, (void *)name, strlen(name), &plen, flags); if (error) @@ -1270,6 +1278,8 @@ sysctl_root(SYSCTL_HANDLER_ARGS) struct sysctl_oid *oid; int error, indx, lvl; + SYSCTL_LOCK_ASSERT(); + error = sysctl_find_oid(arg1, arg2, &oid, &indx, req); if (error) return (error); @@ -1324,7 +1334,11 @@ sysctl_root(SYSCTL_HANDLER_ARGS) if (error != 0) return (error); #endif + + /* XXX: Handlers are not guaranteed to be Giant safe! */ + mtx_lock(&Giant); error = oid->oid_handler(oid, arg1, arg2, req); + mtx_unlock(&Giant); return (error); } @@ -1352,20 +1366,13 @@ __sysctl(struct thread *td, struct sysct if (error) return (error); - mtx_lock(&Giant); - error = userland_sysctl(td, name, uap->namelen, uap->old, uap->oldlenp, 0, uap->new, uap->newlen, &j, 0); if (error && error != ENOMEM) - goto done2; - if (uap->oldlenp) { - int i = copyout(&j, uap->oldlenp, sizeof(j)); - if (i) - error = i; - } -done2: - mtx_unlock(&Giant); + return (error); + if (uap->oldlenp) + error = copyout(&j, uap->oldlenp, sizeof(j)); return (error); } @@ -1426,12 +1433,12 @@ userland_sysctl(struct thread *td, int * uio_yield(); } - if (req.lock == REQ_WIRED && req.validlen > 0) - vsunlock(req.oldptr, req.validlen); - CURVNET_RESTORE(); SYSCTL_UNLOCK(); + if (req.lock == REQ_WIRED && req.validlen > 0) + vsunlock(req.oldptr, req.validlen); + if (error && error != ENOMEM) return (error); @@ -1519,8 +1526,6 @@ ogetkerninfo(struct thread *td, struct g size_t size; u_int needed = 0; - mtx_lock(&Giant); - switch (uap->op & 0xff00) { case KINFO_RT: @@ -1653,7 +1658,6 @@ ogetkerninfo(struct thread *td, struct g error = copyout(&size, uap->size, sizeof(size)); } } - mtx_unlock(&Giant); return (error); } #endif /* COMPAT_43 */ Modified: head/sys/kern/kern_xxx.c ============================================================================== --- head/sys/kern/kern_xxx.c Mon Dec 29 12:45:11 2008 (r186563) +++ head/sys/kern/kern_xxx.c Mon Dec 29 12:58:45 2008 (r186564) @@ -62,16 +62,12 @@ ogethostname(td, uap) struct gethostname_args *uap; { int name[2]; - int error; size_t len = uap->len; name[0] = CTL_KERN; name[1] = KERN_HOSTNAME; - mtx_lock(&Giant); - error = userland_sysctl(td, name, 2, uap->hostname, &len, - 1, 0, 0, 0, 0); - mtx_unlock(&Giant); - return(error); + return (userland_sysctl(td, name, 2, uap->hostname, &len, + 1, 0, 0, 0, 0)); } #ifndef _SYS_SYSPROTO_H_ @@ -91,11 +87,8 @@ osethostname(td, uap) name[0] = CTL_KERN; name[1] = KERN_HOSTNAME; - mtx_lock(&Giant); - error = userland_sysctl(td, name, 2, 0, 0, 0, uap->hostname, - uap->len, 0, 0); - mtx_unlock(&Giant); - return (error); + return (userland_sysctl(td, name, 2, 0, 0, 0, uap->hostname, + uap->len, 0, 0)); } #ifndef _SYS_SYSPROTO_H_ @@ -173,11 +166,10 @@ freebsd4_uname(struct thread *td, struct name[0] = CTL_KERN; name[1] = KERN_OSTYPE; len = sizeof (uap->name->sysname); - mtx_lock(&Giant); error = userland_sysctl(td, name, 2, uap->name->sysname, &len, 1, 0, 0, 0, 0); if (error) - goto done2; + return (error); subyte( uap->name->sysname + sizeof(uap->name->sysname) - 1, 0); name[1] = KERN_HOSTNAME; @@ -185,7 +177,7 @@ freebsd4_uname(struct thread *td, struct error = userland_sysctl(td, name, 2, uap->name->nodename, &len, 1, 0, 0, 0, 0); if (error) - goto done2; + return (error); subyte( uap->name->nodename + sizeof(uap->name->nodename) - 1, 0); name[1] = KERN_OSRELEASE; @@ -193,7 +185,7 @@ freebsd4_uname(struct thread *td, struct error = userland_sysctl(td, name, 2, uap->name->release, &len, 1, 0, 0, 0, 0); if (error) - goto done2; + return (error); subyte( uap->name->release + sizeof(uap->name->release) - 1, 0); /* @@ -202,7 +194,7 @@ freebsd4_uname(struct thread *td, struct error = userland_sysctl(td, name, 2, uap->name->version, &len, 1, 0, 0, 0, 0); if (error) - goto done2; + return (error); subyte( uap->name->version + sizeof(uap->name->version) - 1, 0); */ @@ -214,11 +206,11 @@ freebsd4_uname(struct thread *td, struct for(us = uap->name->version; *s && *s != ':'; s++) { error = subyte( us++, *s); if (error) - goto done2; + return (error); } error = subyte( us++, 0); if (error) - goto done2; + return (error); name[0] = CTL_HW; name[1] = HW_MACHINE; @@ -226,11 +218,9 @@ freebsd4_uname(struct thread *td, struct error = userland_sysctl(td, name, 2, uap->name->machine, &len, 1, 0, 0, 0, 0); if (error) - goto done2; + return (error); subyte( uap->name->machine + sizeof(uap->name->machine) - 1, 0); -done2: - mtx_unlock(&Giant); - return (error); + return (0); } #ifndef _SYS_SYSPROTO_H_ @@ -245,16 +235,12 @@ freebsd4_getdomainname(struct thread *td struct freebsd4_getdomainname_args *uap) { int name[2]; - int error; size_t len = uap->len; name[0] = CTL_KERN; name[1] = KERN_NISDOMAINNAME; - mtx_lock(&Giant); - error = userland_sysctl(td, name, 2, uap->domainname, &len, - 1, 0, 0, 0, 0); - mtx_unlock(&Giant); - return(error); + return (userland_sysctl(td, name, 2, uap->domainname, &len, + 1, 0, 0, 0, 0)); } #ifndef _SYS_SYSPROTO_H_ @@ -269,14 +255,10 @@ freebsd4_setdomainname(struct thread *td struct freebsd4_setdomainname_args *uap) { int name[2]; - int error; name[0] = CTL_KERN; name[1] = KERN_NISDOMAINNAME; - mtx_lock(&Giant); - error = userland_sysctl(td, name, 2, 0, 0, 0, uap->domainname, - uap->len, 0, 0); - mtx_unlock(&Giant); - return (error); + return (userland_sysctl(td, name, 2, 0, 0, 0, uap->domainname, + uap->len, 0, 0)); } #endif /* COMPAT_FREEBSD4 */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200812291258.mBTCwjDv057880>