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 */
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
