Index: sys/kern/kern_sysctl.c =================================================================== --- sys/kern/kern_sysctl.c +++ sys/kern/kern_sysctl.c @@ -96,9 +96,13 @@ * The sysctlmemlock is used to limit the amount of user memory wired for * sysctl requests. This is implemented by serializing any userland * sysctl requests larger than a single page via an exclusive lock. + * + * The sysctlstringlock is used to protect concurrent access to writable + * string nodes in sysctl_handle_string(). */ static struct rmlock sysctllock; static struct sx __exclusive_cache_line sysctlmemlock; +static struct sx sysctlstringlock; #define SYSCTL_WLOCK() rm_wlock(&sysctllock) #define SYSCTL_WUNLOCK() rm_wunlock(&sysctllock) @@ -170,10 +174,16 @@ else SYSCTL_WUNLOCK(); - if (!(oid->oid_kind & CTLFLAG_MPSAFE)) + /* + * Treat set CTLFLAG_NEEDGIANT and unset CTLFLAG_MPSAFE flags the same, + * untill we're ready to remove all Giant parts in sysctl(9). + */ + if ((oid->oid_kind & CTLFLAG_NEEDGIANT) || + (!(oid->oid_kind & CTLFLAG_MPSAFE))) mtx_lock(&Giant); error = oid->oid_handler(oid, arg1, arg2, req); - if (!(oid->oid_kind & CTLFLAG_MPSAFE)) + if ((oid->oid_kind & CTLFLAG_NEEDGIANT) || + (!(oid->oid_kind & CTLFLAG_MPSAFE))) mtx_unlock(&Giant); KFAIL_POINT_ERROR(_debug_fail_point, sysctl_running, error); @@ -917,6 +927,7 @@ struct sysctl_oid **oidp; sx_init(&sysctlmemlock, "sysctl mem"); + sx_init(&sysctlstringlock, "sysctl string handler"); SYSCTL_INIT(); SYSCTL_WLOCK(); SET_FOREACH(oidp, sysctl_set) @@ -1636,11 +1647,13 @@ int error = 0, ro_string = 0; /* + * If the sysctl isn't writable, microoptimise and treat it as a + * const string. * A zero-length buffer indicates a fixed size read-only * string. In ddb, don't worry about trying to make a malloced * snapshot. */ - if (arg2 == 0 || kdb_active) { + if (!(oidp->oid_kind & CTLFLAG_WR) || arg2 == 0 || kdb_active) { arg2 = strlen((char *)arg1) + 1; ro_string = 1; } @@ -1650,20 +1663,26 @@ if (ro_string) { tmparg = arg1; + outlen = strlen(tmparg) + 1; } else { - /* try to make a coherent snapshot of the string */ tmparg = malloc(arg2, M_SYSCTLTMP, M_WAITOK); + sx_slock(&sysctlstringlock); memcpy(tmparg, arg1, arg2); + outlen = strnlen(tmparg, arg2 - 1) + 1; + sx_sunlock(&sysctlstringlock); } - outlen = strnlen(tmparg, arg2 - 1) + 1; error = SYSCTL_OUT(req, tmparg, outlen); if (!ro_string) free(tmparg, M_SYSCTLTMP); } else { + if (!ro_string) + sx_slock(&sysctlstringlock); outlen = strnlen((char *)arg1, arg2 - 1) + 1; error = SYSCTL_OUT(req, NULL, outlen); + if (!ro_string) + sx_sunlock(&sysctlstringlock); } if (error || !req->newptr) return (error); @@ -1671,9 +1690,11 @@ if ((req->newlen - req->newidx) >= arg2) { error = EINVAL; } else { + sx_xlock(&sysctlstringlock); arg2 = (req->newlen - req->newidx); error = SYSCTL_IN(req, arg1, arg2); ((char *)arg1)[arg2] = '\0'; + sx_xunlock(&sysctlstringlock); } return (error); } Index: sys/sys/sysctl.h =================================================================== --- sys/sys/sysctl.h +++ sys/sys/sysctl.h @@ -42,6 +42,12 @@ #include #endif +#ifdef notyet +#ifndef static_assert +#define static_assert _Static_assert +#endif +#endif + struct thread; /* * Definitions for sysctl call. The sysctl call uses a hierarchical name @@ -105,6 +111,13 @@ #define CTLFLAG_STATS 0x00002000 /* Statistics, not a tuneable */ #define CTLFLAG_NOFETCH 0x00001000 /* Don't fetch tunable from getenv() */ #define CTLFLAG_CAPRW (CTLFLAG_CAPRD|CTLFLAG_CAPWR) +/* + * This is transient flag to be used until all sysctl handlers are converted + * to not lock Giant. + * One, and only one of CTLFLAG_MPSAFE or CTLFLAG_NEEDGIANT is required + * for SYSCTL_PROC and SYSCTL_NODE. + */ +#define CTLFLAG_NEEDGIANT 0x00000800 /* Handler require Giant */ /* * Secure level. Note that CTLFLAG_SECURE == CTLFLAG_SECURE1. @@ -263,6 +276,14 @@ #define __DESCR(d) "" #endif +#ifdef notyet +#define SYSCTL_ENFORCE_FLAGS(x) \ + static_assert(((CTLFLAG_MPSAFE ^ CTLFLAG_NEEDGIANT) & (x)), \ + "Has to be either CTLFLAG_MPSAFE or CTLFLAG_NEEDGIANT") +#else +#define SYSCTL_ENFORCE_FLAGS(x) +#endif + /* This macro is only for internal use */ #define SYSCTL_OID_RAW(id, parent_child_head, nbr, name, kind, a1, a2, handler, fmt, descr, label) \ struct sysctl_oid id = { \ @@ -278,7 +299,8 @@ .oid_descr = __DESCR(descr), \ .oid_label = (label), \ }; \ - DATA_SET(sysctl_set, id) + DATA_SET(sysctl_set, id); \ + SYSCTL_ENFORCE_FLAGS(kind) /* This constructs a static "raw" MIB oid. */ #define SYSCTL_OID(parent, nbr, name, kind, a1, a2, handler, fmt, descr) \ @@ -297,7 +319,11 @@ nbr, #name, kind, a1, a2, handler, fmt, descr, label) #define SYSCTL_ADD_OID(ctx, parent, nbr, name, kind, a1, a2, handler, fmt, descr) \ - sysctl_add_oid(ctx, parent, nbr, name, kind, a1, a2, handler, fmt, __DESCR(descr), NULL) +({ \ + SYSCTL_ENFORCE_FLAGS(kind); \ + sysctl_add_oid(ctx, parent, nbr, name, kind, a1, a2,handler, \ + fmt, __DESCR(descr), NULL); \ +}) /* This constructs a root node from which other nodes can hang. */ #define SYSCTL_ROOT_NODE(nbr, name, access, handler, descr) \ @@ -325,6 +351,7 @@ ({ \ CTASSERT(((access) & CTLTYPE) == 0 || \ ((access) & SYSCTL_CT_ASSERT_MASK) == CTLTYPE_NODE); \ + SYSCTL_ENFORCE_FLAGS(access); \ sysctl_add_oid(ctx, parent, nbr, name, CTLTYPE_NODE|(access), \ NULL, 0, handler, "N", __DESCR(descr), label); \ }) @@ -333,6 +360,7 @@ ({ \ CTASSERT(((access) & CTLTYPE) == 0 || \ ((access) & SYSCTL_CT_ASSERT_MASK) == CTLTYPE_NODE); \ + SYSCTL_ENFORCE_FLAGS(access); \ sysctl_add_oid(ctx, &sysctl__children, nbr, name, \ CTLTYPE_NODE|(access), \ NULL, 0, handler, "N", __DESCR(descr), NULL); \ @@ -340,7 +368,8 @@ /* Oid for a string. len can be 0 to indicate '\0' termination. */ #define SYSCTL_STRING(parent, nbr, name, access, arg, len, descr) \ - SYSCTL_OID(parent, nbr, name, CTLTYPE_STRING|(access), \ + SYSCTL_OID(parent, nbr, name, \ + CTLTYPE_STRING | CTLFLAG_MPSAFE | (access), \ arg, len, sysctl_handle_string, "A", descr); \ CTASSERT(((access) & CTLTYPE) == 0 || \ ((access) & SYSCTL_CT_ASSERT_MASK) == CTLTYPE_STRING) @@ -350,7 +379,8 @@ char *__arg = (arg); \ CTASSERT(((access) & CTLTYPE) == 0 || \ ((access) & SYSCTL_CT_ASSERT_MASK) == CTLTYPE_STRING); \ - sysctl_add_oid(ctx, parent, nbr, name, CTLTYPE_STRING|(access), \ + sysctl_add_oid(ctx, parent, nbr, name, \ + CTLTYPE_STRING | CTLFLAG_MPSAFE | (access), \ __arg, len, sysctl_handle_string, "A", __DESCR(descr), \ NULL); \ }) @@ -741,7 +771,8 @@ /* Oid for an opaque object. Specified by a pointer and a length. */ #define SYSCTL_OPAQUE(parent, nbr, name, access, ptr, len, fmt, descr) \ - SYSCTL_OID(parent, nbr, name, CTLTYPE_OPAQUE|(access), \ + SYSCTL_OID(parent, nbr, name, \ + CTLTYPE_OPAQUE | CTLFLAG_MPSAFE | (access), \ ptr, len, sysctl_handle_opaque, fmt, descr); \ CTASSERT(((access) & CTLTYPE) == 0 || \ ((access) & SYSCTL_CT_ASSERT_MASK) == CTLTYPE_OPAQUE) @@ -750,13 +781,15 @@ ({ \ CTASSERT(((access) & CTLTYPE) == 0 || \ ((access) & SYSCTL_CT_ASSERT_MASK) == CTLTYPE_OPAQUE); \ - sysctl_add_oid(ctx, parent, nbr, name, CTLTYPE_OPAQUE|(access), \ + sysctl_add_oid(ctx, parent, nbr, name, \ + CTLTYPE_OPAQUE | CTLFLAG_MPSAFE | (access), \ ptr, len, sysctl_handle_opaque, fmt, __DESCR(descr), NULL); \ }) /* Oid for a struct. Specified by a pointer and a type. */ #define SYSCTL_STRUCT(parent, nbr, name, access, ptr, type, descr) \ - SYSCTL_OID(parent, nbr, name, CTLTYPE_OPAQUE|(access), \ + SYSCTL_OID(parent, nbr, name, \ + CTLTYPE_OPAQUE | CTLFLAG_MPSAFE | (access), \ ptr, sizeof(struct type), sysctl_handle_opaque, \ "S," #type, descr); \ CTASSERT(((access) & CTLTYPE) == 0 || \ @@ -766,7 +799,8 @@ ({ \ CTASSERT(((access) & CTLTYPE) == 0 || \ ((access) & SYSCTL_CT_ASSERT_MASK) == CTLTYPE_OPAQUE); \ - sysctl_add_oid(ctx, parent, nbr, name, CTLTYPE_OPAQUE|(access), \ + sysctl_add_oid(ctx, parent, nbr, name, \ + CTLTYPE_OPAQUE | CTLFLAG_MPSAFE | (access), \ (ptr), sizeof(struct type), \ sysctl_handle_opaque, "S," #type, __DESCR(descr), NULL); \ }) @@ -780,6 +814,7 @@ #define SYSCTL_ADD_PROC(ctx, parent, nbr, name, access, ptr, arg, handler, fmt, descr) \ ({ \ CTASSERT(((access) & CTLTYPE) != 0); \ + SYSCTL_ENFORCE_FLAGS(access); \ sysctl_add_oid(ctx, parent, nbr, name, (access), \ (ptr), (arg), (handler), (fmt), __DESCR(descr), NULL); \ })