From owner-svn-src-all@freebsd.org Thu Oct 5 12:32:16 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 34FE1E377CC; Thu, 5 Oct 2017 12:32:16 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id EB57B6F6AE; Thu, 5 Oct 2017 12:32:15 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id v95CWFEG043456; Thu, 5 Oct 2017 12:32:15 GMT (envelope-from avg@FreeBSD.org) Received: (from avg@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id v95CWEng043453; Thu, 5 Oct 2017 12:32:14 GMT (envelope-from avg@FreeBSD.org) Message-Id: <201710051232.v95CWEng043453@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: avg set sender to avg@FreeBSD.org using -f From: Andriy Gapon Date: Thu, 5 Oct 2017 12:32:14 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r324311 - in head/sys: kern sys X-SVN-Group: head X-SVN-Commit-Author: avg X-SVN-Commit-Paths: in head/sys: kern sys X-SVN-Commit-Revision: 324311 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Oct 2017 12:32:16 -0000 Author: avg Date: Thu Oct 5 12:32:14 2017 New Revision: 324311 URL: https://svnweb.freebsd.org/changeset/base/324311 Log: sysctl-s in a module should be accessible only when the module is initialized A sysctl can have a custom handler that may access data that is initialized via SYSINIT(9) or via a module event handler (also invoked via SYSINIT). Thus, it is not safe to allow access to the module's sysctl-s until the initialization is performed. Likewise, we should not allow access to teh sysctl-s after the module is uninitialized. The latter is easy to achieve by properly ordering linker_file_unregister_sysctls and linker_file_sysuninit. The former is not as easy for two reasons: - the initialization may depend on tunables which get set when sysctl-s are registered, so we need to set the tunables before running sysinit-s - the initialization may try to dynamically add more sysctl-s under statically defined sysctl nodes So, this change splits the sysctl setup into two phases. In the first phase the sysctl-s are registered as before but they are disabled and hidden from consumers. In the second phase, done after sysinit-s, normal access to the sysctl-s is enabled. The change should affect only dynamic module loading and unloading after the system boot-up. Nothing changes for sysctl-s compiled into the kernel and sysctl-s in preloaded modules. Discussed with: hselasky, ian, jhb Reviewed by: julian, kib MFC after: 2 weeks Sponsored by: Panzura Differential Revision: https://reviews.freebsd.org/D12545 Modified: head/sys/kern/kern_linker.c head/sys/kern/kern_sysctl.c head/sys/sys/sysctl.h Modified: head/sys/kern/kern_linker.c ============================================================================== --- head/sys/kern/kern_linker.c Thu Oct 5 12:29:34 2017 (r324310) +++ head/sys/kern/kern_linker.c Thu Oct 5 12:32:14 2017 (r324311) @@ -288,7 +288,7 @@ linker_file_sysuninit(linker_file_t lf) } static void -linker_file_register_sysctls(linker_file_t lf) +linker_file_register_sysctls(linker_file_t lf, bool enable) { struct sysctl_oid **start, **stop, **oidp; @@ -303,8 +303,34 @@ linker_file_register_sysctls(linker_file_t lf) sx_xunlock(&kld_sx); sysctl_wlock(); + for (oidp = start; oidp < stop; oidp++) { + if (enable) + sysctl_register_oid(*oidp); + else + sysctl_register_disabled_oid(*oidp); + } + sysctl_wunlock(); + sx_xlock(&kld_sx); +} + +static void +linker_file_enable_sysctls(linker_file_t lf) +{ + struct sysctl_oid **start, **stop, **oidp; + + KLD_DPF(FILE, + ("linker_file_enable_sysctls: enable SYSCTLs for %s\n", + lf->filename)); + + sx_assert(&kld_sx, SA_XLOCKED); + + if (linker_file_lookup_set(lf, "sysctl_set", &start, &stop, NULL) != 0) + return; + + sx_xunlock(&kld_sx); + sysctl_wlock(); for (oidp = start; oidp < stop; oidp++) - sysctl_register_oid(*oidp); + sysctl_enable_oid(*oidp); sysctl_wunlock(); sx_xlock(&kld_sx); } @@ -430,7 +456,7 @@ linker_load_file(const char *filename, linker_file_t * return (error); } modules = !TAILQ_EMPTY(&lf->modules); - linker_file_register_sysctls(lf); + linker_file_register_sysctls(lf, false); linker_file_sysinit(lf); lf->flags |= LINKER_FILE_LINKED; @@ -443,6 +469,7 @@ linker_load_file(const char *filename, linker_file_t * linker_file_unload(lf, LINKER_UNLOAD_FORCE); return (ENOEXEC); } + linker_file_enable_sysctls(lf); EVENTHANDLER_INVOKE(kld_load, lf); *result = lf; return (0); @@ -692,8 +719,8 @@ linker_file_unload(linker_file_t file, int flags) */ if (file->flags & LINKER_FILE_LINKED) { file->flags &= ~LINKER_FILE_LINKED; - linker_file_sysuninit(file); linker_file_unregister_sysctls(file); + linker_file_sysuninit(file); } TAILQ_REMOVE(&linker_files, file, link); @@ -1642,7 +1669,7 @@ restart: if (linker_file_lookup_set(lf, "sysinit_set", &si_start, &si_stop, NULL) == 0) sysinit_add(si_start, si_stop); - linker_file_register_sysctls(lf); + linker_file_register_sysctls(lf, true); lf->flags |= LINKER_FILE_LINKED; continue; fail: Modified: head/sys/kern/kern_sysctl.c ============================================================================== --- head/sys/kern/kern_sysctl.c Thu Oct 5 12:29:34 2017 (r324310) +++ head/sys/kern/kern_sysctl.c Thu Oct 5 12:32:14 2017 (r324311) @@ -510,6 +510,37 @@ retry: } void +sysctl_register_disabled_oid(struct sysctl_oid *oidp) +{ + + /* + * Mark the leaf as dormant if it's not to be immediately enabled. + * We do not disable nodes as they can be shared between modules + * and it is always safe to access a node. + */ + KASSERT((oidp->oid_kind & CTLFLAG_DORMANT) == 0, + ("internal flag is set in oid_kind")); + if ((oidp->oid_kind & CTLTYPE) != CTLTYPE_NODE) + oidp->oid_kind |= CTLFLAG_DORMANT; + sysctl_register_oid(oidp); +} + +void +sysctl_enable_oid(struct sysctl_oid *oidp) +{ + + SYSCTL_ASSERT_WLOCKED(); + if ((oidp->oid_kind & CTLTYPE) == CTLTYPE_NODE) { + KASSERT((oidp->oid_kind & CTLFLAG_DORMANT) == 0, + ("sysctl node is marked as dormant")); + return; + } + KASSERT((oidp->oid_kind & CTLFLAG_DORMANT) != 0, + ("enabling already enabled sysctl oid")); + oidp->oid_kind &= ~CTLFLAG_DORMANT; +} + +void sysctl_unregister_oid(struct sysctl_oid *oidp) { struct sysctl_oid *p; @@ -1057,7 +1088,7 @@ sysctl_sysctl_next_ls(struct sysctl_oid_list *lsp, int *next = oidp->oid_number; *oidpp = oidp; - if (oidp->oid_kind & CTLFLAG_SKIP) + if ((oidp->oid_kind & (CTLFLAG_SKIP | CTLFLAG_DORMANT)) != 0) continue; if (!namelen) { @@ -1878,6 +1909,8 @@ sysctl_find_oid(int *name, u_int namelen, struct sysct } lsp = SYSCTL_CHILDREN(oid); } else if (indx == namelen) { + if ((oid->oid_kind & CTLFLAG_DORMANT) != 0) + return (ENOENT); *noid = oid; if (nindx != NULL) *nindx = indx; Modified: head/sys/sys/sysctl.h ============================================================================== --- head/sys/sys/sysctl.h Thu Oct 5 12:29:34 2017 (r324310) +++ head/sys/sys/sysctl.h Thu Oct 5 12:32:14 2017 (r324311) @@ -83,6 +83,7 @@ struct ctlname { #define CTLFLAG_RD 0x80000000 /* Allow reads of variable */ #define CTLFLAG_WR 0x40000000 /* Allow writes to the variable */ #define CTLFLAG_RW (CTLFLAG_RD|CTLFLAG_WR) +#define CTLFLAG_DORMANT 0x20000000 /* This sysctl is not active yet */ #define CTLFLAG_ANYBODY 0x10000000 /* All users can set this var */ #define CTLFLAG_SECURE 0x08000000 /* Permit set only if securelevel<=0 */ #define CTLFLAG_PRISON 0x04000000 /* Prisoned roots can fiddle */ @@ -219,6 +220,8 @@ int sysctl_dpcpu_quad(SYSCTL_HANDLER_ARGS); * These functions are used to add/remove an oid from the mib. */ void sysctl_register_oid(struct sysctl_oid *oidp); +void sysctl_register_disabled_oid(struct sysctl_oid *oidp); +void sysctl_enable_oid(struct sysctl_oid *oidp); void sysctl_unregister_oid(struct sysctl_oid *oidp); /* Declare a static oid to allow child oids to be added to it. */