Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/kern ksyms(4): Simply block unload until last /dev/ksyms...



details:   https://anonhg.NetBSD.org/src/rev/bad5373d7a0c
branches:  trunk
changeset: 1023436:bad5373d7a0c
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Tue Sep 07 10:59:46 2021 +0000

description:
ksyms(4): Simply block unload until last /dev/ksyms close.

Otherwise, readers may get a garbled snapshot of ksyms (or a crash on
an assertion failure because of the garbled snapshot) if modules are
unloaded while they read.

https://mail-index.netbsd.org/source-changes-d/2021/08/17/msg013425.html

diffstat:

 sys/kern/kern_ksyms.c |  121 +++++++++++++++++++------------------------------
 1 files changed, 48 insertions(+), 73 deletions(-)

diffs (273 lines):

diff -r 0257be4b6f4b -r bad5373d7a0c sys/kern/kern_ksyms.c
--- a/sys/kern/kern_ksyms.c     Tue Sep 07 10:44:18 2021 +0000
+++ b/sys/kern/kern_ksyms.c     Tue Sep 07 10:59:46 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: kern_ksyms.c,v 1.98 2021/07/18 06:57:28 mlelstv Exp $  */
+/*     $NetBSD: kern_ksyms.c,v 1.99 2021/09/07 10:59:46 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.98 2021/07/18 06:57:28 mlelstv Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_ksyms.c,v 1.99 2021/09/07 10:59:46 riastradh Exp $");
 
 #if defined(_KERNEL) && defined(_KERNEL_OPT)
 #include "opt_copy_symtab.h"
@@ -117,6 +117,7 @@
 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 *);
@@ -245,6 +246,7 @@
 
        if (!ksyms_initted) {
                mutex_init(&ksyms_lock, MUTEX_DEFAULT, IPL_NONE);
+               cv_init(&ksyms_cv, "ksyms");
                ksyms_initted = true;
        }
 }
@@ -328,7 +330,6 @@
        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;
@@ -446,9 +447,9 @@
        KASSERT(cold || mutex_owned(&ksyms_lock));
 
        /*
-        * Ensure ddb never witnesses an inconsistent state of the
-        * queue, unless memory is so corrupt that we crash in
-        * TAILQ_INSERT_TAIL.
+        * 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.
         */
        s = splhigh();
        TAILQ_INSERT_TAIL(&ksyms_symtabs, tab, sd_queue);
@@ -601,8 +602,6 @@
 #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) {
@@ -636,8 +635,6 @@
 
        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;
@@ -671,8 +668,6 @@
 
        /* 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;
 
@@ -716,8 +711,6 @@
                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);
@@ -780,37 +773,44 @@
 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;
        }
-       mutex_exit(&ksyms_lock);
        KASSERT(st != NULL);
 
-       if (do_free) {
-               kmem_free(st->sd_nmap, st->sd_nmapsize * sizeof(uint32_t));
-               kmem_free(st, sizeof(*st));
-       }
+       /*
+        * 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);
+
+       /*
+        * 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));
 }
 
 #ifdef DDB
@@ -830,8 +830,6 @@
                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;
@@ -893,8 +891,6 @@
 
        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++)
@@ -1027,37 +1023,14 @@
 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;
-       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();
+       cv_broadcast(&ksyms_cv);
 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;
 }
 
@@ -1069,8 +1042,7 @@
        int error;
 
        /*
-        * First: Copy out the ELF header.   XXX Lose if ksymsopen()
-        * occurs during read of the header.
+        * First: Copy out the ELF header.
         */
        off = uio->uio_offset;
        if (off < sizeof(struct ksyms_hdr)) {
@@ -1081,12 +1053,17 @@
        }
 
        /*
-        * Copy out the symbol table.
+        * 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).
         */
        filepos = sizeof(struct ksyms_hdr);
-       TAILQ_FOREACH(st, &ksyms_symtabs, sd_queue) {
-               if (__predict_false(st->sd_gone))
-                       continue;
+       for (st = TAILQ_FIRST(&ksyms_symtabs);
+            ;
+            st = TAILQ_NEXT(st, sd_queue)) {
                if (uio->uio_resid == 0)
                        return 0;
                if (uio->uio_offset <= st->sd_symsize + filepos) {
@@ -1125,6 +1102,8 @@
 
        /*
         * 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) {
@@ -1196,8 +1175,6 @@
                 */
                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
@@ -1238,8 +1215,6 @@
                 */
                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



Home | Main Index | Thread Index | Old Index