From 4be87b1fcc473738e7c3620d27862f5b3f9d709e Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 30 Aug 2022 21:42:09 +0000 Subject: [PATCH 1/5] ddb(9): New db_num_to_strbuf. Like db_num_to_str, but writes to caller-provided buffer instead of returning pointer to static storage. --- sys/ddb/db_lex.c | 17 ++++++++++++----- sys/ddb/db_lex.h | 1 + 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/sys/ddb/db_lex.c b/sys/ddb/db_lex.c index 6c1bd89f4d32..a2a8f08fa1be 100644 --- a/sys/ddb/db_lex.c +++ b/sys/ddb/db_lex.c @@ -172,14 +172,21 @@ db_num_to_str(db_expr_t val) */ static char buf[25]; + db_num_to_strbuf(val, buf, sizeof(buf)); + + return (buf); +} + +void +db_num_to_strbuf(db_expr_t val, char *buf, size_t len) +{ + if (db_radix == 16) - snprintf(buf, sizeof(buf), "%" DDB_EXPR_FMT "x", val); + snprintf(buf, len, "%" DDB_EXPR_FMT "x", val); else if (db_radix == 8) - snprintf(buf, sizeof(buf), "%" DDB_EXPR_FMT "o", val); + snprintf(buf, len, "%" DDB_EXPR_FMT "o", val); else - snprintf(buf, sizeof(buf), "%" DDB_EXPR_FMT "u", val); - - return (buf); + snprintf(buf, len, "%" DDB_EXPR_FMT "u", val); } void diff --git a/sys/ddb/db_lex.h b/sys/ddb/db_lex.h index e06e3f480bc4..9f7a7a0c997f 100644 --- a/sys/ddb/db_lex.h +++ b/sys/ddb/db_lex.h @@ -34,6 +34,7 @@ */ void db_flush_lex(void); char *db_num_to_str(db_expr_t); +void db_num_to_strbuf(db_expr_t, char *, size_t); int db_read_line(void); void db_set_line(const char *, const char *); void db_get_line(const char **, const char **); From 8a78b073434f5365c0c79f45094b2901b7271efe Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 30 Aug 2022 21:43:52 +0000 Subject: [PATCH 2/5] ddb(4): Use db_num_to_strbuf in db_sym. Simplifies it and will make safer to use. --- sys/ddb/db_sym.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/ddb/db_sym.c b/sys/ddb/db_sym.c index d6abb7455ed7..5d95db31c275 100644 --- a/sys/ddb/db_sym.c +++ b/sys/ddb/db_sym.c @@ -372,7 +372,7 @@ db_symstr(char *buf, size_t buflen, db_expr_t off, db_strategy_t strategy) } } out: - strlcpy(buf, db_num_to_str(off), buflen); + db_num_to_strbuf(off, buf, buflen); #endif } From 0bffd1989ccb17140274e04a34c5fbefe540264c Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 30 Aug 2022 21:48:13 +0000 Subject: [PATCH 3/5] ddb(9): Make db_symstr safe to use concurrently with pserialize(9). --- sys/ddb/db_sym.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sys/ddb/db_sym.c b/sys/ddb/db_sym.c index 5d95db31c275..a15def660622 100644 --- a/sys/ddb/db_sym.c +++ b/sys/ddb/db_sym.c @@ -37,6 +37,7 @@ __KERNEL_RCSID(0, "$NetBSD: db_sym.c,v 1.69 2021/12/13 03:17:50 kre Exp $"); #include #include #include +#include #include @@ -344,13 +345,14 @@ db_symstr(char *buf, size_t buflen, db_expr_t off, db_strategy_t strategy) } #endif #ifdef _KERNEL + const int s = pserialize_read_enter(); if (ksyms_getname(&mod, &name, (vaddr_t)off, strategy|KSYMS_CLOSEST) == 0) { (void)ksyms_getval_unlocked(mod, name, NULL, &val, KSYMS_ANY); if (strategy & KSYMS_PROC && val == off) { if (ksyms_getname(&mod, &name, (vaddr_t)off - 1, strategy|KSYMS_CLOSEST) != 0) - goto out; + goto hex_fallback; (void)ksyms_getval_unlocked(mod, name, NULL, &val, KSYMS_ANY); } if (((off - val) < db_maxoff) && val) { @@ -368,11 +370,12 @@ db_symstr(char *buf, size_t buflen, db_expr_t off, db_strategy_t strategy) " [%s:%d]", filename, linenum); } #endif - return; + goto out; } } -out: +hex_fallback: db_num_to_strbuf(off, buf, buflen); +out: pserialize_read_exit(s); #endif } From 687259809633360f2399d0a4cdfb6a16e2f2159d Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 30 Aug 2022 22:08:09 +0000 Subject: [PATCH 4/5] lockdebug(9): Try to show symbol names if possible. Also print the possible owner in ddb/crash `show lock' even if the kernel is built without LOCKDEBUG. --- sys/kern/subr_lockdebug.c | 68 +++++++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/sys/kern/subr_lockdebug.c b/sys/kern/subr_lockdebug.c index 7f1d0e615b4b..fc6dc4c0a2c8 100644 --- a/sys/kern/subr_lockdebug.c +++ b/sys/kern/subr_lockdebug.c @@ -712,6 +712,7 @@ lockdebug_mem_check(const char *func, size_t line, void *base, size_t sz) #include #include #include +#include #endif /* @@ -725,12 +726,34 @@ lockdebug_dump(lwp_t *l, lockdebug_t *ld, void (*pr)(const char *, ...) { int sleeper = (ld->ld_flags & LD_SLEEPER); lockops_t *lo = ld->ld_lockops; + char locksym[128], initsym[128], lockedsym[128], unlockedsym[128]; + +#ifdef DDB + db_symstr(locksym, sizeof(locksym), (db_expr_t)ld->ld_lock, + DB_STGY_ANY); + db_symstr(initsym, sizeof(initsym), (db_expr_t)ld->ld_initaddr, + DB_STGY_PROC); + db_symstr(lockedsym, sizeof(lockedsym), (db_expr_t)ld->ld_locked, + DB_STGY_PROC); + db_symstr(unlockedsym, sizeof(unlockedsym), (db_expr_t)ld->ld_unlocked, + DB_STGY_PROC); +#else + snprintf(locksym, sizeof(locksym), "%#018lx", + (unsigned long)ld->ld_lock); + snprintf(initsym, sizeof(initsym), "%#018lx", + (unsigned long)ld->ld_initaddr); + snprintf(lockedsym, sizeof(lockedsym), "%#018lx", + (unsigned long)ld->ld_locked); + snprintf(unlockedsym, sizeof(unlockedsym), "%#018lx", + (unsigned long)ld->ld_unlocked); +#endif (*pr)( - "lock address : %#018lx type : %18s\n" - "initialized : %#018lx", - (long)ld->ld_lock, (sleeper ? "sleep/adaptive" : "spin"), - (long)ld->ld_initaddr); + "lock address : %s\n" + "type : %s\n" + "initialized : %s", + locksym, (sleeper ? "sleep/adaptive" : "spin"), + initsym); #ifndef _KERNEL lockops_t los; @@ -742,15 +765,16 @@ lockdebug_dump(lwp_t *l, lockdebug_t *ld, void (*pr)(const char *, ...) "shares wanted: %18u exclusive: %18u\n" "relevant cpu : %18u last held: %18u\n" "relevant lwp : %#018lx last held: %#018lx\n" - "last locked%c : %#018lx unlocked%c: %#018lx\n", + "last locked%c : %s\n" + "unlocked%c : %s\n", (unsigned)ld->ld_shares, ((ld->ld_flags & LD_LOCKED) != 0), (unsigned)ld->ld_shwant, (unsigned)ld->ld_exwant, (unsigned)cpu_index(l->l_cpu), (unsigned)ld->ld_cpu, (long)l, (long)ld->ld_lwp, ((ld->ld_flags & LD_LOCKED) ? '*' : ' '), - (long)ld->ld_locked, + lockedsym, ((ld->ld_flags & LD_LOCKED) ? ' ' : '*'), - (long)ld->ld_unlocked); + unlockedsym); #ifdef _KERNEL if (lo->lo_dump != NULL) @@ -827,7 +851,14 @@ lockdebug_lock_print(void *addr, addr); } #else - (*pr)("Sorry, kernel not built with the LOCKDEBUG option.\n"); + char sym[128]; + uintptr_t word; + + (*pr)("WARNING: lock print is unreliable without LOCKDEBUG\n"); + db_symstr(sym, sizeof(sym), (db_expr_t)addr, DB_STGY_ANY); + db_read_bytes((db_addr_t)addr, sizeof(word), &word); + (*pr)("%s: possible owner: %p, bits: 0x%x\n", sym, + (void *)(word & ~(uintptr_t)ALIGNBYTES), word & ALIGNBYTES); #endif /* LOCKDEBUG */ } @@ -837,11 +868,12 @@ static void lockdebug_show_one(lwp_t *l, lockdebug_t *ld, int i, void (*pr)(const char *, ...) __printflike(1, 2)) { - const char *sym; + char sym[128]; -#ifdef _KERNEL - ksyms_getname(NULL, &sym, (vaddr_t)ld->ld_initaddr, - KSYMS_CLOSEST|KSYMS_PROC|KSYMS_ANY); +#ifdef DDB + db_symstr(sym, sizeof(sym), (db_expr_t)ld->ld_initaddr, DB_STGY_PROC); +#else + snprintf(sym, sizeof(sym), "%p", (void *)ld->ld_initaddr); #endif (*pr)("* Lock %d (initialized at %s)\n", i++, sym); lockdebug_dump(l, ld, pr); @@ -1040,11 +1072,19 @@ lockdebug_abort(const char *func, size_t line, const volatile void *lock, if (atomic_inc_uint_nv(&ld_panic) > 1) return; + char locksym[128]; + +#ifdef DDB + db_symstr(locksym, sizeof(locksym), (db_expr_t)lock, DB_STGY_ANY); +#else + snprintf(locksym, sizeof(locksym), "%#018lx", (unsigned long)lock); +#endif + printf("%s error: %s,%zu: %s\n\n" - "lock address : %#018lx\n" + "lock address : %s\n" "current cpu : %18d\n" "current lwp : %#018lx\n", - ops->lo_name, func, line, msg, (long)lock, + ops->lo_name, func, line, msg, locksym, (int)cpu_index(curcpu()), (long)curlwp); (*ops->lo_dump)(lock, printf); printf("\n"); From 3637ccf7a7e240fd4bc9c055794bb0545342adc8 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 30 Aug 2022 22:11:37 +0000 Subject: [PATCH 5/5] crashme(9): New debug.crashme.mutex_recursion method. Takes a lock twice. Set it to 1 for adaptive lock, 2 for spin lock. --- sys/kern/kern_crashme.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/sys/kern/kern_crashme.c b/sys/kern/kern_crashme.c index 523311d51795..01bfc29b2b0e 100644 --- a/sys/kern/kern_crashme.c +++ b/sys/kern/kern_crashme.c @@ -66,6 +66,7 @@ static int crashme_ddb(int); #ifdef LOCKDEBUG static int crashme_kernel_lock_spinout(int); #endif +static int crashme_mutex_recursion(int); #define CMNODE(name, lname, func) \ { \ @@ -85,6 +86,8 @@ static crashme_node nodes[] = { CMNODE("kernel_lock_spinout", "infinite kernel lock", crashme_kernel_lock_spinout), #endif + CMNODE("mutex_recursion", "enter the same mutex twice", + crashme_mutex_recursion), }; static crashme_node *first_node; static kmutex_t crashme_lock; @@ -290,3 +293,36 @@ crashme_kernel_lock_spinout(int flags) return 0; } #endif + +static int +crashme_mutex_recursion(int flags) +{ + kmutex_t crashme_spinlock; + + switch (flags) { + case 0: + return 0; + case 1: + default: + /* + * printf makes the return address of the first + * mutex_enter call a little more obvious, so the line + * number of the _return address_ for the first + * mutex_enter doesn't confusingly point at the second + * mutex_enter. + */ + mutex_enter(&crashme_lock); + printf("%s: locked once\n", __func__); + mutex_enter(&crashme_lock); + printf("%s: locked twice\n", __func__); + return -1; + case 2: + mutex_init(&crashme_spinlock, MUTEX_DEFAULT, IPL_VM); + printf("%s: initialized\n", __func__); + mutex_enter(&crashme_spinlock); + printf("%s: locked once\n", __func__); + mutex_enter(&crashme_spinlock); + printf("%s: locked twice\n", __func__); + return -1; + } +}