Module Name:    src
Committed By:   riastradh
Date:           Tue Sep  7 16:56:25 UTC 2021

Modified Files:
        src/sys/kern: kern_ksyms.c

Log Message:
Revert "ksyms(4): Simply block unload until last /dev/ksyms close."

This appears to break t_execsnoop -- presumably something goes wrong
with how libdtrace uses ksyms.  To investigate.


To generate a diff of this commit:
cvs rdiff -u -r1.101 -r1.102 src/sys/kern/kern_ksyms.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/kern/kern_ksyms.c
diff -u src/sys/kern/kern_ksyms.c:1.101 src/sys/kern/kern_ksyms.c:1.102
--- src/sys/kern/kern_ksyms.c:1.101	Tue Sep  7 16:56:13 2021
+++ src/sys/kern/kern_ksyms.c	Tue Sep  7 16:56:25 2021
@@ -1,4 +1,4 @@
-/*	$NetBSD: kern_ksyms.c,v 1.101 2021/09/07 16:56:13 riastradh Exp $	*/
+/*	$NetBSD: kern_ksyms.c,v 1.102 2021/09/07 16:56:25 riastradh Exp $	*/
 
 /*-
  * Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -73,7 +73,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_ksyms.c,v 1.101 2021/09/07 16:56:13 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_ksyms.c,v 1.102 2021/09/07 16:56:25 riastradh Exp $");
 
 #if defined(_KERNEL) && defined(_KERNEL_OPT)
 #include "opt_copy_symtab.h"
@@ -117,7 +117,6 @@ static struct ksyms_symtab *ksyms_last_s
 static bool ksyms_initted;
 static bool ksyms_loaded;
 static kmutex_t ksyms_lock __cacheline_aligned;
-static kcondvar_t ksyms_cv;
 static struct ksyms_symtab kernel_symtab;
 
 static void ksyms_hdr_init(const void *);
@@ -246,7 +245,6 @@ ksyms_init(void)
 
 	if (!ksyms_initted) {
 		mutex_init(&ksyms_lock, MUTEX_DEFAULT, IPL_NONE);
-		cv_init(&ksyms_cv, "ksyms");
 		ksyms_initted = true;
 	}
 }
@@ -330,6 +328,7 @@ addsymtab(const char *name, void *symsta
 	tab->sd_minsym = UINTPTR_MAX;
 	tab->sd_maxsym = 0;
 	tab->sd_usroffset = 0;
+	tab->sd_gone = false;
 	tab->sd_ctfstart = ctfstart;
 	tab->sd_ctfsize = ctfsize;
 	tab->sd_nmap = nmap;
@@ -447,9 +446,9 @@ addsymtab(const char *name, void *symsta
 	KASSERT(cold || mutex_owned(&ksyms_lock));
 
 	/*
-	 * Publish the symtab.  Do this at splhigh to ensure ddb never
-	 * witnesses an inconsistent state of the queue, unless memory
-	 * is so corrupt that we crash in TAILQ_INSERT_TAIL.
+	 * Ensure ddb never witnesses an inconsistent state of the
+	 * queue, unless memory is so corrupt that we crash in
+	 * TAILQ_INSERT_TAIL.
 	 */
 	s = splhigh();
 	TAILQ_INSERT_TAIL(&ksyms_symtabs, tab, sd_queue);
@@ -602,6 +601,8 @@ ksyms_getval_unlocked(const char *mod, c
 #endif
 
 	TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+		if (__predict_false(st->sd_gone))
+			continue;
 		if (mod != NULL && strcmp(st->sd_name, mod))
 			continue;
 		if ((es = findsym(sym, st, type)) != NULL) {
@@ -635,6 +636,8 @@ ksyms_get_mod(const char *mod)
 
 	mutex_enter(&ksyms_lock);
 	TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+		if (__predict_false(st->sd_gone))
+			continue;
 		if (mod != NULL && strcmp(st->sd_name, mod))
 			continue;
 		break;
@@ -668,6 +671,8 @@ ksyms_mod_foreach(const char *mod, ksyms
 
 	/* find the module */
 	TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+		if (__predict_false(st->sd_gone))
+			continue;
 		if (mod != NULL && strcmp(st->sd_name, mod))
 			continue;
 
@@ -711,6 +716,8 @@ ksyms_getname(const char **mod, const ch
 		return ENOENT;
 
 	TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+		if (st->sd_gone)
+			continue;
 		if (v < st->sd_minsym || v > st->sd_maxsym)
 			continue;
 		sz = st->sd_symsize/sizeof(Elf_Sym);
@@ -773,44 +780,37 @@ void
 ksyms_modunload(const char *name)
 {
 	struct ksyms_symtab *st;
+	bool do_free = false;
 	int s;
 
 	mutex_enter(&ksyms_lock);
 	TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+		if (st->sd_gone)
+			continue;
 		if (strcmp(name, st->sd_name) != 0)
 			continue;
+		st->sd_gone = true;
+		ksyms_sizes_calc();
+		if (ksyms_opencnt == 0) {
+			/*
+			 * Ensure ddb never witnesses an inconsistent
+			 * state of the queue, unless memory is so
+			 * corrupt that we crash in TAILQ_REMOVE.
+			 */
+			s = splhigh();
+			TAILQ_REMOVE(&ksyms_symtabs, st, sd_queue);
+			splx(s);
+			do_free = true;
+		}
 		break;
 	}
-	KASSERT(st != NULL);
-
-	/*
-	 * Wait for last /dev/ksyms close -- readers may be in the
-	 * middle of viewing a snapshot including this module.  (We
-	 * could skip this if it's past ksyms_last_snapshot, but it's
-	 * not clear that's worth the effort.)
-	 */
-	while (ksyms_opencnt)
-		cv_wait(&ksyms_cv, &ksyms_lock);
-
-	/*
-	 * Remove the symtab.  Do this at splhigh to ensure ddb never
-	 * witnesses an inconsistent state of the queue, unless memory
-	 * is so corrupt that we crash in TAILQ_REMOVE.
-	 */
-	s = splhigh();
-	TAILQ_REMOVE(&ksyms_symtabs, st, sd_queue);
-	splx(s);
-
-	/* Recompute the ksyms sizes now that we've removed st.  */
-	ksyms_sizes_calc();
 	mutex_exit(&ksyms_lock);
+	KASSERT(st != NULL);
 
-	/*
-	 * No more references are possible.  Free the name map and the
-	 * symtab itself, which we had allocated in ksyms_modload.
-	 */
-	kmem_free(st->sd_nmap, st->sd_nmapsize * sizeof(uint32_t));
-	kmem_free(st, sizeof(*st));
+	if (do_free) {
+		kmem_free(st->sd_nmap, st->sd_nmapsize * sizeof(uint32_t));
+		kmem_free(st, sizeof(*st));
+	}
 }
 
 #ifdef DDB
@@ -830,6 +830,8 @@ ksyms_sift(char *mod, char *sym, int mod
 		return ENOENT;
 
 	TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+		if (st->sd_gone)
+			continue;
 		if (mod && strcmp(mod, st->sd_name))
 			continue;
 		sb = st->sd_strstart - st->sd_usroffset;
@@ -891,6 +893,8 @@ ksyms_sizes_calc(void)
 
 	ksyms_symsz = ksyms_strsz = 0;
 	TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+		if (__predict_false(st->sd_gone))
+			continue;
 		delta = ksyms_strsz - st->sd_usroffset;
 		if (delta != 0) {
 			for (i = 0; i < st->sd_symsize/sizeof(Elf_Sym); i++)
@@ -1023,14 +1027,37 @@ out:	mutex_exit(&ksyms_lock);
 static int
 ksymsclose(dev_t dev, int oflags, int devtype, struct lwp *l)
 {
+	struct ksyms_symtab *st, *next;
+	TAILQ_HEAD(, ksyms_symtab) to_free = TAILQ_HEAD_INITIALIZER(to_free);
+	int s;
 
+	/* Discard references to symbol tables. */
 	mutex_enter(&ksyms_lock);
 	if (--ksyms_opencnt)
 		goto out;
 	ksyms_last_snapshot = NULL;
-	cv_broadcast(&ksyms_cv);
+	TAILQ_FOREACH_SAFE(st, &ksyms_symtabs, sd_queue, next) {
+		if (st->sd_gone) {
+			/*
+			 * Ensure ddb never witnesses an inconsistent
+			 * state of the queue, unless memory is so
+			 * corrupt that we crash in TAILQ_REMOVE.
+			 */
+			s = splhigh();
+			TAILQ_REMOVE(&ksyms_symtabs, st, sd_queue);
+			splx(s);
+			TAILQ_INSERT_TAIL(&to_free, st, sd_queue);
+		}
+	}
+	if (!TAILQ_EMPTY(&to_free))
+		ksyms_sizes_calc();
 out:	mutex_exit(&ksyms_lock);
 
+	TAILQ_FOREACH_SAFE(st, &to_free, sd_queue, next) {
+		kmem_free(st->sd_nmap, st->sd_nmapsize * sizeof(uint32_t));
+		kmem_free(st, sizeof(*st));
+	}
+
 	return 0;
 }
 
@@ -1042,7 +1069,8 @@ ksymsread(dev_t dev, struct uio *uio, in
 	int error;
 
 	/*
-	 * First: Copy out the ELF header.
+	 * First: Copy out the ELF header.   XXX Lose if ksymsopen()
+	 * occurs during read of the header.
 	 */
 	off = uio->uio_offset;
 	if (off < sizeof(struct ksyms_hdr)) {
@@ -1053,17 +1081,12 @@ ksymsread(dev_t dev, struct uio *uio, in
 	}
 
 	/*
-	 * Copy out the symbol table.  The list of symtabs is
-	 * guaranteed to be nonempty because we always have an entry
-	 * for the main kernel.  We stop at ksyms_last_snapshot, not at
-	 * the end of the tailq or NULL, because entries beyond
-	 * ksyms_last_snapshot are not included in this snapshot (and
-	 * may not be fully initialized memory as we witness it).
+	 * Copy out the symbol table.
 	 */
 	filepos = sizeof(struct ksyms_hdr);
-	for (st = TAILQ_FIRST(&ksyms_symtabs);
-	     ;
-	     st = TAILQ_NEXT(st, sd_queue)) {
+	TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+		if (__predict_false(st->sd_gone))
+			continue;
 		if (uio->uio_resid == 0)
 			return 0;
 		if (uio->uio_offset <= st->sd_symsize + filepos) {
@@ -1102,8 +1125,6 @@ ksymsread(dev_t dev, struct uio *uio, in
 
 	/*
 	 * Copy out the CTF table.
-	 *
-	 * XXX Why does this only copy out the first symtab's CTF?
 	 */
 	st = TAILQ_FIRST(&ksyms_symtabs);
 	if (st->sd_ctfstart != NULL) {
@@ -1175,6 +1196,8 @@ ksymsioctl(dev_t dev, u_long cmd, void *
 		 */
 		mutex_enter(&ksyms_lock);
 		TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+			if (st->sd_gone)
+				continue;
 			if ((sym = findsym(str, st, KSYMS_ANY)) == NULL)
 				continue;
 #ifdef notdef
@@ -1215,6 +1238,8 @@ ksymsioctl(dev_t dev, u_long cmd, void *
 		 */
 		mutex_enter(&ksyms_lock);
 		TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
+			if (st->sd_gone)
+				continue;
 			if ((sym = findsym(str, st, KSYMS_ANY)) == NULL)
 				continue;
 #ifdef notdef

Reply via email to