OS-6925 kmem_dump_size is a corrupting influence

Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
Reviewed by: Robert Mustacchi <rm@joyent.com>
Approved by: Robert Mustacchi <rm@joyent.com>

4 files changed, 124 insertions, 143 deletions

diff --git a/usr/src/cmd/mdb/common/modules/genunix/kmem.c b/usr/src/cmd/mdb/common/modules/genunix/kmem.c
index 0df368d70c..6efa51e3b6 100644
--- a/usr/src/cmd/mdb/common/modules/genunix/kmem.c
+++ b/usr/src/cmd/mdb/common/modules/genunix/kmem.c
@@ -24,7 +24,7 @@
  */
 
 /*
- * Copyright (c) 2015, Joyent, Inc. All rights reserved.
+ * Copyright (c) 2018, Joyent, Inc. All rights reserved.
  * Copyright (c) 2012 by Delphix. All rights reserved.
  */
 
@@ -3012,7 +3012,7 @@ typedef struct kmem_verify {
 	uint64_t *kmv_buf;		/* buffer to read cache contents into */
 	size_t kmv_size;		/* number of bytes in kmv_buf */
 	int kmv_corruption;		/* > 0 if corruption found. */
-	int kmv_besilent;		/* report actual corruption sites */
+	uint_t kmv_flags;		/* dcmd flags */
 	struct kmem_cache kmv_cache;	/* the cache we're operating on */
 } kmem_verify_t;
 
@@ -3057,7 +3057,7 @@ verify_free(uintptr_t addr, const void *data, void *private)
 	int64_t corrupt;		/* corruption offset */
 	kmem_buftag_t *buftagp;		/* ptr to buftag */
 	kmem_cache_t *cp = &kmv->kmv_cache;
-	int besilent = kmv->kmv_besilent;
+	boolean_t besilent = !!(kmv->kmv_flags & (DCMD_LOOP | DCMD_PIPE_OUT));
 
 	/*LINTED*/
 	buftagp = KMEM_BUFTAG(cp, buf);
@@ -3103,6 +3103,8 @@ verify_free(uintptr_t addr, const void *data, void *private)
 
 	return (WALK_NEXT);
 corrupt:
+	if (kmv->kmv_flags & DCMD_PIPE_OUT)
+		mdb_printf("%p\n", addr);
 	kmv->kmv_corruption++;
 	return (WALK_NEXT);
 }
@@ -3124,7 +3126,7 @@ verify_alloc(uintptr_t addr, const void *data, void *private)
 	uint32_t *ip = (uint32_t *)buftagp;
 	uint8_t *bp = (uint8_t *)buf;
 	int looks_ok = 0, size_ok = 1;	/* flags for finding corruption */
-	int besilent = kmv->kmv_besilent;
+	boolean_t besilent = !!(kmv->kmv_flags & (DCMD_LOOP | DCMD_PIPE_OUT));
 
 	/*
 	 * Read the buffer to check.
@@ -3182,6 +3184,9 @@ verify_alloc(uintptr_t addr, const void *data, void *private)
 
 	return (WALK_NEXT);
 corrupt:
+	if (kmv->kmv_flags & DCMD_PIPE_OUT)
+		mdb_printf("%p\n", addr);
+
 	kmv->kmv_corruption++;
 	return (WALK_NEXT);
 }
@@ -3200,10 +3205,18 @@ kmem_verify(uintptr_t addr, uint_t flags, int argc, const mdb_arg_t *argv)
 			return (DCMD_ERR);
 		}
 
+		if ((kmv.kmv_cache.cache_dump.kd_unsafe ||
+		    kmv.kmv_cache.cache_dump.kd_alloc_fails) &&
+		    !(flags & (DCMD_LOOP | DCMD_PIPE_OUT))) {
+			mdb_warn("WARNING: cache was used during dump: "
+			    "corruption may be incorrectly reported\n");
+		}
+
 		kmv.kmv_size = kmv.kmv_cache.cache_buftag +
 		    sizeof (kmem_buftag_t);
 		kmv.kmv_buf = mdb_alloc(kmv.kmv_size, UM_SLEEP | UM_GC);
 		kmv.kmv_corruption = 0;
+		kmv.kmv_flags = flags;
 
 		if ((kmv.kmv_cache.cache_flags & KMF_REDZONE)) {
 			check_alloc = 1;
@@ -3218,16 +3231,10 @@ kmem_verify(uintptr_t addr, uint_t flags, int argc, const mdb_arg_t *argv)
 			return (DCMD_ERR);
 		}
 
-		if (flags & DCMD_LOOP) {
-			/*
-			 * table mode, don't print out every corrupt buffer
-			 */
-			kmv.kmv_besilent = 1;
-		} else {
+		if (!(flags & (DCMD_LOOP | DCMD_PIPE_OUT))) {
 			mdb_printf("Summary for cache '%s'\n",
 			    kmv.kmv_cache.cache_name);
 			mdb_inc_indent(2);
-			kmv.kmv_besilent = 0;
 		}
 
 		if (check_alloc)
@@ -3235,31 +3242,31 @@ kmem_verify(uintptr_t addr, uint_t flags, int argc, const mdb_arg_t *argv)
 		if (check_free)
 			(void) mdb_pwalk("freemem", verify_free, &kmv, addr);
 
-		if (flags & DCMD_LOOP) {
-			if (kmv.kmv_corruption == 0) {
-				mdb_printf("%-*s %?p clean\n",
-				    KMEM_CACHE_NAMELEN,
-				    kmv.kmv_cache.cache_name, addr);
+		if (!(flags & DCMD_PIPE_OUT)) {
+			if (flags & DCMD_LOOP) {
+				if (kmv.kmv_corruption == 0) {
+					mdb_printf("%-*s %?p clean\n",
+					    KMEM_CACHE_NAMELEN,
+					    kmv.kmv_cache.cache_name, addr);
+				} else {
+					mdb_printf("%-*s %?p %d corrupt "
+					    "buffer%s\n", KMEM_CACHE_NAMELEN,
+					    kmv.kmv_cache.cache_name, addr,
+					    kmv.kmv_corruption,
+					    kmv.kmv_corruption > 1 ? "s" : "");
+				}
 			} else {
-				char *s = "";	/* optional s in "buffer[s]" */
-				if (kmv.kmv_corruption > 1)
-					s = "s";
-
-				mdb_printf("%-*s %?p %d corrupt buffer%s\n",
-				    KMEM_CACHE_NAMELEN,
-				    kmv.kmv_cache.cache_name, addr,
-				    kmv.kmv_corruption, s);
-			}
-		} else {
-			/*
-			 * This is the more verbose mode, when the user has
-			 * type addr::kmem_verify.  If the cache was clean,
-			 * nothing will have yet been printed. So say something.
-			 */
-			if (kmv.kmv_corruption == 0)
-				mdb_printf("clean\n");
+				/*
+				 * This is the more verbose mode, when the user
+				 * typed addr::kmem_verify.  If the cache was
+				 * clean, nothing will have yet been printed. So
+				 * say something.
+				 */
+				if (kmv.kmv_corruption == 0)
+					mdb_printf("clean\n");
 
-			mdb_dec_indent(2);
+				mdb_dec_indent(2);
+			}
 		}
 	} else {
 		/*
@@ -3267,8 +3274,23 @@ kmem_verify(uintptr_t addr, uint_t flags, int argc, const mdb_arg_t *argv)
 		 * kmem_cache's, specifying ourself as a callback for each...
 		 * this is the equivalent of '::walk kmem_cache .::kmem_verify'
 		 */
-		mdb_printf("%<u>%-*s %-?s %-20s%</b>\n", KMEM_CACHE_NAMELEN,
-		    "Cache Name", "Addr", "Cache Integrity");
+
+		if (!(flags & DCMD_PIPE_OUT)) {
+			uintptr_t dump_curr;
+			uintptr_t dump_end;
+
+			if (mdb_readvar(&dump_curr, "kmem_dump_curr") != -1 &&
+			    mdb_readvar(&dump_end, "kmem_dump_end") != -1 &&
+			    dump_curr == dump_end) {
+				mdb_warn("WARNING: exceeded kmem_dump_size; "
+				    "corruption may be incorrectly reported\n");
+			}
+
+			mdb_printf("%<u>%-*s %-?s %-20s%</b>\n",
+			    KMEM_CACHE_NAMELEN, "Cache Name", "Addr",
+			    "Cache Integrity");
+		}
+
 		(void) (mdb_walk_dcmd("kmem_cache", "kmem_verify", 0, NULL));
 	}
 
diff --git a/usr/src/uts/common/os/dumpsubr.c b/usr/src/uts/common/os/dumpsubr.c
index 38d5f1ab18..ee654074ba 100644
--- a/usr/src/uts/common/os/dumpsubr.c
+++ b/usr/src/uts/common/os/dumpsubr.c
@@ -21,7 +21,7 @@
 
 /*
  * Copyright (c) 1998, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright 2016 Joyent, Inc.
+ * Copyright 2018 Joyent, Inc.
  */
 
 #include <sys/types.h>
@@ -74,6 +74,8 @@
 
 #include <bzip2/bzlib.h>
 
+#define	ONE_GIG	(1024 * 1024 * 1024UL)
+
 /*
  * Crash dump time is dominated by disk write time.  To reduce this,
  * the stronger compression method bzip2 is applied to reduce the dump
@@ -138,7 +140,7 @@ uint_t dump_plat_mincpu = MINCPU_NOT_SET;
 
 /* tunables for pre-reserved heap */
 uint_t dump_kmem_permap = 1024;
-uint_t dump_kmem_pages = 8;
+uint_t dump_kmem_pages = 0;
 
 /* Define multiple buffers per helper to avoid stalling */
 #define	NCBUF_PER_HELPER	2
@@ -679,11 +681,23 @@ dump_update_clevel()
 	}
 
 	/*
-	 * Reserve memory for kmem allocation calls made during crash
-	 * dump.  The hat layer allocates memory for each mapping
-	 * created, and the I/O path allocates buffers and data structs.
-	 * Add a few pages for safety.
+	 * Reserve memory for kmem allocation calls made during crash dump.  The
+	 * hat layer allocates memory for each mapping created, and the I/O path
+	 * allocates buffers and data structs.
+	 *
+	 * On larger systems, we easily exceed the lower amount, so we need some
+	 * more space; the cut-over point is relatively arbitrary.  If we run
+	 * out, the only impact is that kmem state in the dump becomes
+	 * inconsistent.
 	 */
+
+	if (dump_kmem_pages == 0) {
+		if (physmem > (16 * ONE_GIG) / PAGESIZE)
+			dump_kmem_pages = 20;
+		else
+			dump_kmem_pages = 8;
+	}
+
 	kmem_dump_init((new->ncmap * dump_kmem_permap) +
 	    (dump_kmem_pages * PAGESIZE));
 
diff --git a/usr/src/uts/common/os/kmem.c b/usr/src/uts/common/os/kmem.c
index 069e2f2e0a..9a3692053d 100644
--- a/usr/src/uts/common/os/kmem.c
+++ b/usr/src/uts/common/os/kmem.c
@@ -2222,33 +2222,6 @@ typedef struct kmem_dumpctl {
 	((kmem_dumpctl_t *)P2ROUNDUP((uintptr_t)(buf) + (cp)->cache_bufsize, \
 	    sizeof (void *)))
 
-/* Keep some simple stats. */
-#define	KMEM_DUMP_LOGS	(100)
-
-typedef struct kmem_dump_log {
-	kmem_cache_t	*kdl_cache;
-	uint_t		kdl_allocs;		/* # of dump allocations */
-	uint_t		kdl_frees;		/* # of dump frees */
-	uint_t		kdl_alloc_fails;	/* # of allocation failures */
-	uint_t		kdl_free_nondump;	/* # of non-dump frees */
-	uint_t		kdl_unsafe;		/* cache was used, but unsafe */
-} kmem_dump_log_t;
-
-static kmem_dump_log_t *kmem_dump_log;
-static int kmem_dump_log_idx;
-
-#define	KDI_LOG(cp, stat) {						\
-	kmem_dump_log_t *kdl;						\
-	if ((kdl = (kmem_dump_log_t *)((cp)->cache_dumplog)) != NULL) {	\
-		kdl->stat++;						\
-	} else if (kmem_dump_log_idx < KMEM_DUMP_LOGS) {		\
-		kdl = &kmem_dump_log[kmem_dump_log_idx++];		\
-		kdl->stat++;						\
-		kdl->kdl_cache = (cp);					\
-		(cp)->cache_dumplog = kdl;				\
-	}								\
-}
-
 /* set non zero for full report */
 uint_t kmem_dump_verbose = 0;
 
@@ -2278,25 +2251,17 @@ kmem_dumppr(char **pp, char *e, const char *format, ...)
 void
 kmem_dump_init(size_t size)
 {
+	/* Our caller ensures size is always set. */
+	ASSERT3U(size, >, 0);
+
 	if (kmem_dump_start != NULL)
 		kmem_free(kmem_dump_start, kmem_dump_size);
 
-	if (kmem_dump_log == NULL)
-		kmem_dump_log = (kmem_dump_log_t *)kmem_zalloc(KMEM_DUMP_LOGS *
-		    sizeof (kmem_dump_log_t), KM_SLEEP);
-
 	kmem_dump_start = kmem_alloc(size, KM_SLEEP);
-
-	if (kmem_dump_start != NULL) {
-		kmem_dump_size = size;
-		kmem_dump_curr = kmem_dump_start;
-		kmem_dump_end = (void *)((char *)kmem_dump_start + size);
-		copy_pattern(KMEM_UNINITIALIZED_PATTERN, kmem_dump_start, size);
-	} else {
-		kmem_dump_size = 0;
-		kmem_dump_curr = NULL;
-		kmem_dump_end = NULL;
-	}
+	kmem_dump_size = size;
+	kmem_dump_curr = kmem_dump_start;
+	kmem_dump_end = (void *)((char *)kmem_dump_start + size);
+	copy_pattern(KMEM_UNINITIALIZED_PATTERN, kmem_dump_start, size);
 }
 
 /*
@@ -2307,24 +2272,23 @@ kmem_dump_init(size_t size)
 void
 kmem_dump_begin(void)
 {
-	ASSERT(panicstr != NULL);
-	if (kmem_dump_start != NULL) {
-		kmem_cache_t *cp;
+	kmem_cache_t *cp;
 
-		for (cp = list_head(&kmem_caches); cp != NULL;
-		    cp = list_next(&kmem_caches, cp)) {
-			kmem_cpu_cache_t *ccp = KMEM_CPU_CACHE(cp);
+	ASSERT(panicstr != NULL);
 
-			if (cp->cache_arena->vm_cflags & VMC_DUMPSAFE) {
-				cp->cache_flags |= KMF_DUMPDIVERT;
-				ccp->cc_flags |= KMF_DUMPDIVERT;
-				ccp->cc_dump_rounds = ccp->cc_rounds;
-				ccp->cc_dump_prounds = ccp->cc_prounds;
-				ccp->cc_rounds = ccp->cc_prounds = -1;
-			} else {
-				cp->cache_flags |= KMF_DUMPUNSAFE;
-				ccp->cc_flags |= KMF_DUMPUNSAFE;
-			}
+	for (cp = list_head(&kmem_caches); cp != NULL;
+	    cp = list_next(&kmem_caches, cp)) {
+		kmem_cpu_cache_t *ccp = KMEM_CPU_CACHE(cp);
+
+		if (cp->cache_arena->vm_cflags & VMC_DUMPSAFE) {
+			cp->cache_flags |= KMF_DUMPDIVERT;
+			ccp->cc_flags |= KMF_DUMPDIVERT;
+			ccp->cc_dump_rounds = ccp->cc_rounds;
+			ccp->cc_dump_prounds = ccp->cc_prounds;
+			ccp->cc_rounds = ccp->cc_prounds = -1;
+		} else {
+			cp->cache_flags |= KMF_DUMPUNSAFE;
+			ccp->cc_flags |= KMF_DUMPUNSAFE;
 		}
 	}
 }
@@ -2337,18 +2301,18 @@ kmem_dump_begin(void)
 size_t
 kmem_dump_finish(char *buf, size_t size)
 {
-	int kdi_idx;
-	int kdi_end = kmem_dump_log_idx;
 	int percent = 0;
-	int header = 0;
-	int warn = 0;
 	size_t used;
-	kmem_cache_t *cp;
-	kmem_dump_log_t *kdl;
 	char *e = buf + size;
 	char *p = buf;
 
-	if (kmem_dump_size == 0 || kmem_dump_verbose == 0)
+	if (kmem_dump_curr == kmem_dump_end) {
+		cmn_err(CE_WARN, "exceeded kmem_dump space of %lu "
+		    "bytes: kmem state in dump may be inconsistent",
+		    kmem_dump_size);
+	}
+
+	if (kmem_dump_verbose == 0)
 		return (0);
 
 	used = (char *)kmem_dump_curr - (char *)kmem_dump_start;
@@ -2362,25 +2326,6 @@ kmem_dump_finish(char *buf, size_t size)
 	kmem_dumppr(&p, e, "Oversize max size,%ld\n",
 	    kmem_dump_oversize_max);
 
-	for (kdi_idx = 0; kdi_idx < kdi_end; kdi_idx++) {
-		kdl = &kmem_dump_log[kdi_idx];
-		cp = kdl->kdl_cache;
-		if (cp == NULL)
-			break;
-		if (kdl->kdl_alloc_fails)
-			++warn;
-		if (header == 0) {
-			kmem_dumppr(&p, e,
-			    "Cache Name,Allocs,Frees,Alloc Fails,"
-			    "Nondump Frees,Unsafe Allocs/Frees\n");
-			header = 1;
-		}
-		kmem_dumppr(&p, e, "%s,%d,%d,%d,%d,%d\n",
-		    cp->cache_name, kdl->kdl_allocs, kdl->kdl_frees,
-		    kdl->kdl_alloc_fails, kdl->kdl_free_nondump,
-		    kdl->kdl_unsafe);
-	}
-
 	/* return buffer size used */
 	if (p < e)
 		bzero(p, e - p);
@@ -2398,9 +2343,8 @@ kmem_cache_alloc_dump(kmem_cache_t *cp, int kmflag)
 	char *bufend;
 
 	/* return a constructed object */
-	if ((buf = cp->cache_dumpfreelist) != NULL) {
-		cp->cache_dumpfreelist = KMEM_DUMPCTL(cp, buf)->kdc_next;
-		KDI_LOG(cp, kdl_allocs);
+	if ((buf = cp->cache_dump.kd_freelist) != NULL) {
+		cp->cache_dump.kd_freelist = KMEM_DUMPCTL(cp, buf)->kdc_next;
 		return (buf);
 	}
 
@@ -2421,7 +2365,7 @@ kmem_cache_alloc_dump(kmem_cache_t *cp, int kmflag)
 	/* fall back to normal alloc if reserved area is used up */
 	if (bufend > (char *)kmem_dump_end) {
 		kmem_dump_curr = kmem_dump_end;
-		KDI_LOG(cp, kdl_alloc_fails);
+		cp->cache_dump.kd_alloc_fails++;
 		return (NULL);
 	}
 
@@ -2443,12 +2387,11 @@ kmem_cache_alloc_dump(kmem_cache_t *cp, int kmflag)
 		if (kmem_dump_curr == bufend)
 			kmem_dump_curr = curr;
 
+		cp->cache_dump.kd_alloc_fails++;
 		/* fall back to normal alloc if the constructor fails */
-		KDI_LOG(cp, kdl_alloc_fails);
 		return (NULL);
 	}
 
-	KDI_LOG(cp, kdl_allocs);
 	return (buf);
 }
 
@@ -2461,15 +2404,11 @@ kmem_cache_free_dump(kmem_cache_t *cp, void *buf)
 	/* save constructed buffers for next time */
 	if ((char *)buf >= (char *)kmem_dump_start &&
 	    (char *)buf < (char *)kmem_dump_end) {
-		KMEM_DUMPCTL(cp, buf)->kdc_next = cp->cache_dumpfreelist;
-		cp->cache_dumpfreelist = buf;
-		KDI_LOG(cp, kdl_frees);
+		KMEM_DUMPCTL(cp, buf)->kdc_next = cp->cache_dump.kd_freelist;
+		cp->cache_dump.kd_freelist = buf;
 		return (0);
 	}
 
-	/* count all non-dump buf frees */
-	KDI_LOG(cp, kdl_free_nondump);
-
 	/* just drop buffers that were allocated before dump started */
 	if (kmem_dump_curr < kmem_dump_end)
 		return (0);
@@ -2502,7 +2441,7 @@ kmem_cache_alloc(kmem_cache_t *cp, int kmflag)
 				if (ccp->cc_flags & KMF_DUMPUNSAFE) {
 					ASSERT(!(ccp->cc_flags &
 					    KMF_DUMPDIVERT));
-					KDI_LOG(cp, kdl_unsafe);
+					cp->cache_dump.kd_unsafe++;
 				}
 				if ((ccp->cc_flags & KMF_BUFTAG) &&
 				    kmem_cache_alloc_debug(cp, buf, kmflag, 0,
@@ -2533,7 +2472,7 @@ kmem_cache_alloc(kmem_cache_t *cp, int kmflag)
 			if (ccp->cc_flags & KMF_DUMPUNSAFE) {
 				ASSERT(!(ccp->cc_flags & KMF_DUMPDIVERT));
 				/* log it so that we can warn about it */
-				KDI_LOG(cp, kdl_unsafe);
+				cp->cache_dump.kd_unsafe++;
 			} else {
 				if ((buf = kmem_cache_alloc_dump(cp, kmflag)) !=
 				    NULL) {
@@ -2729,7 +2668,7 @@ kmem_cache_free(kmem_cache_t *cp, void *buf)
 		if (ccp->cc_flags & KMF_DUMPUNSAFE) {
 			ASSERT(!(ccp->cc_flags & KMF_DUMPDIVERT));
 			/* log it so that we can warn about it */
-			KDI_LOG(cp, kdl_unsafe);
+			cp->cache_dump.kd_unsafe++;
 		} else if (KMEM_DUMPCC(ccp) && !kmem_cache_free_dump(cp, buf)) {
 			return;
 		}
diff --git a/usr/src/uts/common/sys/kmem_impl.h b/usr/src/uts/common/sys/kmem_impl.h
index 26ab055dbc..6a6dc3d9b5 100644
--- a/usr/src/uts/common/sys/kmem_impl.h
+++ b/usr/src/uts/common/sys/kmem_impl.h
@@ -21,6 +21,7 @@
 
 /*
  * Copyright (c) 1994, 2010, Oracle and/or its affiliates. All rights reserved.
+ * Copyright 2018 Joyent, Inc.
  */
 
 #ifndef _SYS_KMEM_IMPL_H
@@ -328,6 +329,12 @@ typedef struct kmem_defrag {
 	kthread_t	*kmd_thread;		/* thread calling move */
 } kmem_defrag_t;
 
+typedef struct kmem_dump {
+	void		*kd_freelist;		/* heap during crash dump */
+	uint_t		kd_alloc_fails;		/* # of allocation failures */
+	uint_t		kd_unsafe;		/* cache was used, but unsafe */
+} kmem_dump_t;
+
 #define	KMEM_CACHE_NAMELEN	31
 
 struct kmem_cache {
@@ -398,8 +405,7 @@ struct kmem_cache {
 	kmem_magtype_t	*cache_magtype;		/* magazine type */
 	kmem_maglist_t	cache_full;		/* full magazines */
 	kmem_maglist_t	cache_empty;		/* empty magazines */
-	void		*cache_dumpfreelist;	/* heap during crash dump */
-	void		*cache_dumplog;		/* log entry during dump */
+	kmem_dump_t	cache_dump;		/* used during crash dump */
 
 	/*
 	 * Per-CPU layer