From 391a453e068ebe15410df2c21e48f6fb0ad66d03 Mon Sep 17 00:00:00 2001 From: avg Date: Sun, 1 Oct 2017 16:34:16 +0000 Subject: [PATCH] MFV r323531: 8521 nvlist memory leak in get_clones_stat() and spa_load_best() illumos/illumos-gate@7d3000f774e20097a1ee45cbd06d0e38065ddd5a https://github.com/illumos/illumos-gate/commit/7d3000f774e20097a1ee45cbd06d0e38065ddd5a https://www.illumos.org/issues/8521 Yuri reported this to the mailing list: doing a `reboot -d` on current illumos-gate HEAD gives the following ":: findleaks -dv" output: findleaks: maximum buffers => 301061 findleaks: actual buffers => 297587 findleaks: findleaks: potential pointers => 29289774 findleaks: dismissals => 26242305 (89.5%) findleaks: misses => 331153 ( 1.1%) findleaks: dups => 2419681 ( 8.2%) findleaks: follows => 296635 ( 1.0%) findleaks: findleaks: peak memory usage => 7353 kB findleaks: elapsed CPU time => 1.5 seconds findleaks: elapsed wall time => 2.0 seconds findleaks: CACHE LEAKED BUFCTL CALLER ffffff03d222b008 120 ffffff03ef7ceb78 nv_alloc_sys+0x1f ffffff03d222a448 123 ffffff03f4150cc8 nv_alloc_sys+0x1f ffffff03d222b448 5 ffffff03f28bd598 nv_alloc_sys+0x1f ffffff03d222b888 87 ffffff03f28c10f0 nv_alloc_sys+0x1f ffffff03d222c008 21 ffffff03f4139310 nv_alloc_sys+0x1f ffffff03d222b888 43 ffffff040ef3f3e8 nv_alloc_sys+0x1f ffffff03d222c008 120 ffffff03f4591e58 nv_alloc_sys+0x1f ffffff03d222b008 121 ffffff03f352c068 nv_alloc_sys+0x1f ffffff03d222a448 112 ffffff03f414e5f8 nv_alloc_sys+0x1f ffffff03d222b008 119 ffffff03ee92fdc0 nv_alloc_sys+0x1f ffffff03d222b888 46 ffffff03f28c1378 nv_alloc_sys+0x1f ffffff03d222b448 4 ffffff03f28c7708 nv_alloc_sys+0x1f ffffff03d222c008 20 ffffff03f2a6e7e8 nv_alloc_sys+0x1f Reviewed by: Steve Gonczi Reviewed by: George Wilson Reviewed by: Yuri Pankov Reviewed by: Matt Ahrens Approved by: Dan McDonald Author: Pavel Zakharov MFC after: 5 weeks X-MFC after: r324163 --- .../uts/common/fs/zfs/dsl_dataset.c | 6 +-- .../opensolaris/uts/common/fs/zfs/spa.c | 2 + sys/kern/kern_linker.c | 37 ++++++++++++-- sys/kern/kern_sysctl.c | 48 +++++++++++++++++-- sys/sys/sysctl.h | 3 ++ 5 files changed, 85 insertions(+), 11 deletions(-) diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c index 313ca17a405a..e34dec3be677 100644 --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c @@ -1808,10 +1808,10 @@ get_clones_stat(dsl_dataset_t *ds, nvlist_t *nv) fnvlist_add_nvlist(propval, ZPROP_VALUE, val); fnvlist_add_nvlist(nv, zfs_prop_to_name(ZFS_PROP_CLONES), propval); - } else { - nvlist_free(val); - nvlist_free(propval); } + + nvlist_free(val); + nvlist_free(propval); } /* diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c index f2235cecca51..87649a421ca8 100644 --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c @@ -3096,6 +3096,8 @@ spa_load_best(spa_t *spa, spa_load_state_t state, int mosconfig, if (config && (rewind_error || state != SPA_LOAD_RECOVER)) spa_config_set(spa, config); + else + nvlist_free(config); if (state == SPA_LOAD_RECOVER) { ASSERT3P(loadinfo, ==, NULL); diff --git a/sys/kern/kern_linker.c b/sys/kern/kern_linker.c index 06cd3c0a5750..fc9c8fb33cd7 100644 --- a/sys/kern/kern_linker.c +++ b/sys/kern/kern_linker.c @@ -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; @@ -298,13 +298,39 @@ linker_file_register_sysctls(linker_file_t lf) 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++) { + 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,8 +456,9 @@ linker_load_file(const char *filename, linker_file_t *result) return (error); } modules = !TAILQ_EMPTY(&lf->modules); - linker_file_register_sysctls(lf); + linker_file_register_sysctls(lf, false); linker_file_sysinit(lf); + linker_file_enable_sysctls(lf); lf->flags |= LINKER_FILE_LINKED; /* @@ -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: diff --git a/sys/kern/kern_sysctl.c b/sys/kern/kern_sysctl.c index b70e8fa8a5d6..12b7b6342c31 100644 --- a/sys/kern/kern_sysctl.c +++ b/sys/kern/kern_sysctl.c @@ -408,8 +408,8 @@ SYSCTL_PROC(_sysctl, 0, reuse_test, CTLTYPE_STRING|CTLFLAG_RD|CTLFLAG_MPSAFE, 0, 0, sysctl_reuse_test, "-", ""); #endif -void -sysctl_register_oid(struct sysctl_oid *oidp) +static void +sysctl_register_oid_impl(struct sysctl_oid *oidp, bool enable) { struct sysctl_oid_list *parent = oidp->oid_parent; struct sysctl_oid *p; @@ -491,6 +491,17 @@ retry: } /* update the OID number, if any */ oidp->oid_number = oid_number; + + /* + * 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. + */ + if (!enable && (oidp->oid_kind & CTLTYPE) != CTLTYPE_NODE) { + KASSERT((oidp->oid_kind & CTLFLAG_DORMANT) == 0, + ("internal flag is set in oid_kind")); + oidp->oid_kind |= CTLFLAG_DORMANT; + } if (q != NULL) SLIST_INSERT_AFTER(q, oidp, oid_link); else @@ -509,6 +520,35 @@ retry: } } +void +sysctl_register_oid(struct sysctl_oid *oidp) +{ + + sysctl_register_oid_impl(oidp, true); +} + +void +sysctl_register_disabled_oid(struct sysctl_oid *oidp) +{ + + sysctl_register_oid_impl(oidp, false); +} + +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) { @@ -1057,7 +1097,7 @@ sysctl_sysctl_next_ls(struct sysctl_oid_list *lsp, int *name, u_int namelen, *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 +1918,8 @@ sysctl_find_oid(int *name, u_int namelen, struct sysctl_oid **noid, } lsp = SYSCTL_CHILDREN(oid); } else if (indx == namelen) { + if ((oid->oid_kind & CTLFLAG_DORMANT) != 0) + return (ENOENT); *noid = oid; if (nindx != NULL) *nindx = indx; diff --git a/sys/sys/sysctl.h b/sys/sys/sysctl.h index d7721618cbba..5fcdc11630c1 100644 --- a/sys/sys/sysctl.h +++ b/sys/sys/sysctl.h @@ -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. */