Date: Fri, 10 Nov 2023 00:57:04 GMT From: Alexander Motin <mav@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: a03c23931eec - main - uma: Improve memory modified after free panic messages Message-ID: <202311100057.3AA0v46k099663@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by mav: URL: https://cgit.FreeBSD.org/src/commit/?id=a03c23931eec567b0957c2a0b1102dba8d538d98 commit a03c23931eec567b0957c2a0b1102dba8d538d98 Author: Alexander Motin <mav@FreeBSD.org> AuthorDate: 2023-11-10 00:46:26 +0000 Commit: Alexander Motin <mav@FreeBSD.org> CommitDate: 2023-11-10 00:46:26 +0000 uma: Improve memory modified after free panic messages - Pass zone pointer to trash_ctor() and report zone name in the panic message. It may be difficult to figyre out zone just by the item size. - Do not pass user arguments to internal trash calls, pass thezone. - Report malloc type name in the same unified panic message. - Report corruption offset from the beginning of the items instead of the full pointer. It makes panic message shorter and more readable. --- sys/kern/kern_malloc.c | 4 +-- sys/kern/kern_mbuf.c | 6 ++--- sys/vm/uma_core.c | 4 +-- sys/vm/uma_dbg.c | 67 ++++++++++++++++++++++++++++++++++---------------- 4 files changed, 53 insertions(+), 28 deletions(-) diff --git a/sys/kern/kern_malloc.c b/sys/kern/kern_malloc.c index 07f59f733e6a..da7408edd186 100644 --- a/sys/kern/kern_malloc.c +++ b/sys/kern/kern_malloc.c @@ -650,7 +650,7 @@ void * size = (size & ~KMEM_ZMASK) + KMEM_ZBASE; indx = kmemsize[size >> KMEM_ZSHIFT]; zone = kmemzones[indx].kz_zone[mtp_get_subzone(mtp)]; - va = uma_zalloc(zone, flags); + va = uma_zalloc_arg(zone, zone, flags); if (va != NULL) { size = zone->uz_size; if ((flags & M_ZERO) == 0) { @@ -690,7 +690,7 @@ malloc_domain(size_t *sizep, int *indxp, struct malloc_type *mtp, int domain, size = (size & ~KMEM_ZMASK) + KMEM_ZBASE; indx = kmemsize[size >> KMEM_ZSHIFT]; zone = kmemzones[indx].kz_zone[mtp_get_subzone(mtp)]; - va = uma_zalloc_domain(zone, NULL, domain, flags); + va = uma_zalloc_domain(zone, zone, domain, flags); if (va != NULL) *sizep = zone->uz_size; *indxp = indx; diff --git a/sys/kern/kern_mbuf.c b/sys/kern/kern_mbuf.c index 0897ac4cc2db..60c638735ec4 100644 --- a/sys/kern/kern_mbuf.c +++ b/sys/kern/kern_mbuf.c @@ -703,7 +703,7 @@ mb_dtor_pack(void *mem, int size, void *arg) KASSERT(m->m_ext.ext_size == MCLBYTES, ("%s: ext_size != MCLBYTES", __func__)); KASSERT(m->m_ext.ext_type == EXT_PACKET, ("%s: ext_type != EXT_PACKET", __func__)); #if defined(INVARIANTS) && !defined(KMSAN) - trash_dtor(m->m_ext.ext_buf, MCLBYTES, arg); + trash_dtor(m->m_ext.ext_buf, MCLBYTES, zone_clust); #endif /* * If there are processes blocked on zone_clust, waiting for pages @@ -782,7 +782,7 @@ mb_zfini_pack(void *mem, int size) #endif uma_zfree_arg(zone_clust, m->m_ext.ext_buf, NULL); #if defined(INVARIANTS) && !defined(KMSAN) - trash_dtor(mem, size, NULL); + trash_dtor(mem, size, zone_clust); #endif } @@ -804,7 +804,7 @@ mb_ctor_pack(void *mem, int size, void *arg, int how) MPASS((flags & M_NOFREE) == 0); #if defined(INVARIANTS) && !defined(KMSAN) - trash_ctor(m->m_ext.ext_buf, MCLBYTES, arg, how); + trash_ctor(m->m_ext.ext_buf, MCLBYTES, zone_clust, how); #endif error = m_init(m, how, type, flags); diff --git a/sys/vm/uma_core.c b/sys/vm/uma_core.c index 661c98b272da..d185f12448ee 100644 --- a/sys/vm/uma_core.c +++ b/sys/vm/uma_core.c @@ -3468,7 +3468,7 @@ item_ctor(uma_zone_t zone, int uz_flags, int size, void *udata, int flags, skipdbg = uma_dbg_zskip(zone, item); if (!skipdbg && (uz_flags & UMA_ZFLAG_TRASH) != 0 && zone->uz_ctor != trash_ctor) - trash_ctor(item, size, udata, flags); + trash_ctor(item, size, zone, flags); #endif /* Check flags before loading ctor pointer. */ @@ -3510,7 +3510,7 @@ item_dtor(uma_zone_t zone, void *item, int size, void *udata, #ifdef INVARIANTS if (!skipdbg && (zone->uz_flags & UMA_ZFLAG_TRASH) != 0 && zone->uz_dtor != trash_dtor) - trash_dtor(item, size, udata); + trash_dtor(item, size, zone); #endif } kasan_mark_item_invalid(zone, item); diff --git a/sys/vm/uma_dbg.c b/sys/vm/uma_dbg.c index 76dd2bfde2fe..c256e62875c0 100644 --- a/sys/vm/uma_dbg.c +++ b/sys/vm/uma_dbg.c @@ -53,18 +53,22 @@ #include <vm/uma_dbg.h> #include <vm/memguard.h> +#include <machine/stack.h> + static const u_long uma_junk = (u_long)0xdeadc0dedeadc0de; /* * Checks an item to make sure it hasn't been overwritten since it was freed, * prior to subsequent reallocation. * - * Complies with standard ctor arg/return + * Complies with standard ctor arg/return. arg should be zone pointer or NULL. */ int trash_ctor(void *mem, int size, void *arg, int flags) { + struct uma_zone *zone = arg; u_long *p = mem, *e; + int off; #ifdef DEBUG_MEMGUARD if (is_memguard_addr(mem)) @@ -73,19 +77,22 @@ trash_ctor(void *mem, int size, void *arg, int flags) e = p + size / sizeof(*p); for (; p < e; p++) { - if (__predict_true(*p == uma_junk)) - continue; - panic("Memory modified after free %p(%d) val=%lx @ %p\n", - mem, size, *p, p); + if (__predict_false(*p != uma_junk)) + goto dopanic; } return (0); + +dopanic: + off = (uintptr_t)p - (uintptr_t)mem; + panic("Memory modified after free %p (%d, %s) + %d = %lx\n", + mem, size, zone ? zone->uz_name : "", off, *p); + return (0); } /* * Fills an item with predictable garbage * * Complies with standard dtor arg/return - * */ void trash_dtor(void *mem, int size, void *arg) @@ -106,7 +113,6 @@ trash_dtor(void *mem, int size, void *arg) * Fills an item with predictable garbage * * Complies with standard init arg/return - * */ int trash_init(void *mem, int size, int flags) @@ -116,10 +122,10 @@ trash_init(void *mem, int size, int flags) } /* - * Checks an item to make sure it hasn't been overwritten since it was freed. + * Checks an item to make sure it hasn't been overwritten since it was freed, + * prior to freeing it back to available memory. * * Complies with standard fini arg/return - * */ void trash_fini(void *mem, int size) @@ -127,11 +133,19 @@ trash_fini(void *mem, int size) (void)trash_ctor(mem, size, NULL, 0); } +/* + * Checks an item to make sure it hasn't been overwritten since it was freed, + * prior to subsequent reallocation. + * + * Complies with standard ctor arg/return. arg should be zone pointer or NULL. + */ int mtrash_ctor(void *mem, int size, void *arg, int flags) { - struct malloc_type **ksp; + struct uma_zone *zone = arg; u_long *p = mem, *e; + struct malloc_type **ksp; + int off, osize = size; #ifdef DEBUG_MEMGUARD if (is_memguard_addr(mem)) @@ -139,17 +153,31 @@ mtrash_ctor(void *mem, int size, void *arg, int flags) #endif size -= sizeof(struct malloc_type *); - ksp = (struct malloc_type **)mem; - ksp += size / sizeof(struct malloc_type *); e = p + size / sizeof(*p); for (; p < e; p++) { - if (__predict_true(*p == uma_junk)) - continue; - printf("Memory modified after free %p(%d) val=%lx @ %p\n", - mem, size, *p, p); - panic("Most recently used by %s\n", (*ksp == NULL)? - "none" : (*ksp)->ks_shortdesc); + if (__predict_false(*p != uma_junk)) + goto dopanic; + } + return (0); + +dopanic: + off = (uintptr_t)p - (uintptr_t)mem; + ksp = (struct malloc_type **)mem; + ksp += size / sizeof(struct malloc_type *); + if (*ksp != NULL && INKERNEL((uintptr_t)*ksp)) { + /* + * If *ksp is corrupted we may be unable to panic clean, + * so print what we have reliably while we still can. + */ + printf("Memory modified after free %p (%d, %s, %p) + %d = %lx\n", + mem, osize, zone ? zone->uz_name : "", *ksp, off, *p); + panic("Memory modified after free %p (%d, %s, %s) + %d = %lx\n", + mem, osize, zone ? zone->uz_name : "", (*ksp)->ks_shortdesc, + off, *p); + } else { + panic("Memory modified after free %p (%d, %s, %p) + %d = %lx\n", + mem, osize, zone ? zone->uz_name : "", *ksp, off, *p); } return (0); } @@ -158,7 +186,6 @@ mtrash_ctor(void *mem, int size, void *arg, int flags) * Fills an item with predictable garbage * * Complies with standard dtor arg/return - * */ void mtrash_dtor(void *mem, int size, void *arg) @@ -181,7 +208,6 @@ mtrash_dtor(void *mem, int size, void *arg) * Fills an item with predictable garbage * * Complies with standard init arg/return - * */ int mtrash_init(void *mem, int size, int flags) @@ -206,7 +232,6 @@ mtrash_init(void *mem, int size, int flags) * prior to freeing it back to available memory. * * Complies with standard fini arg/return - * */ void mtrash_fini(void *mem, int size)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202311100057.3AA0v46k099663>