From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: sysctl(2): unlock sysctl_malloc()
To: tech@openbsd.org
Date: Tue, 17 Dec 2024 01:33:26 +0300

Except KERN_MALLOC_BUCKETS case it is already mp-safe. This case does
read-only access to `buckstring' which is immutable since is is
initialized at first usage. This is not the fast path, moreover it never
be visited in the most cases, so use existing `sysctl_kmemlock'
rwlock(9) to protect `buckstring' initialization in the way of
KERN_MALLOC_KMEMNAMES case. 

Index: sys/kern/kern_malloc.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_malloc.c,v
diff -u -p -r1.152 kern_malloc.c
--- sys/kern/kern_malloc.c	26 Jun 2024 01:40:49 -0000	1.152
+++ sys/kern/kern_malloc.c	16 Dec 2024 22:30:39 -0000
@@ -50,6 +50,12 @@
 #include <ddb/db_output.h>
 #endif
 
+/*
+ * Locks used to protect data:
+ *	M	malloc_mtx
+ *	S	sysctl_kmemlock
+ */
+
 static
 #ifndef SMALL_KERNEL
 __inline__
@@ -89,20 +95,21 @@ struct vm_map *kmem_map = NULL;
 u_int	nkmempages = NKMEMPAGES;
 
 struct mutex malloc_mtx = MUTEX_INITIALIZER(IPL_VM);
-struct kmembuckets bucket[MINBUCKET + 16];
+struct kmembuckets bucket[MINBUCKET + 16];		/* [M] */
 #ifdef KMEMSTATS
-struct kmemstats kmemstats[M_LAST];
+struct kmemstats kmemstats[M_LAST];			/* [M] */
 #endif
 struct kmemusage *kmemusage;
 char *kmembase, *kmemlimit;
-char buckstring[16 * sizeof("123456,")];
-int buckstring_init = 0;
+char buckstring[16 * sizeof("123456,")];		/* [S] */
+int buckstring_init = 0;				/* [S] */
 #if defined(KMEMSTATS) || defined(DIAGNOSTIC)
 char *memname[] = INITKMEMNAMES;
-char *memall = NULL;
-struct rwlock sysctl_kmemlock = RWLOCK_INITIALIZER("sysctlklk");
+char *memall = NULL;					/* [S] */
 #endif
 
+struct rwlock sysctl_kmemlock = RWLOCK_INITIALIZER("sysctlklk");
+
 /*
  * Normally the freelist structure is used only to hold the list pointer
  * for free objects.  However, when running with diagnostics, the first
@@ -602,6 +609,9 @@ sysctl_malloc(int *name, u_int namelen, 
 
 	switch (name[0]) {
 	case KERN_MALLOC_BUCKETS:
+		error = rw_enter(&sysctl_kmemlock, RW_WRITE|RW_INTR);
+		if (error)
+			return (error);
 		/* Initialize the first time */
 		if (buckstring_init == 0) {
 			buckstring_init = 1;
@@ -616,6 +626,7 @@ sysctl_malloc(int *name, u_int namelen, 
 			if (siz)
 				buckstring[siz - 1] = '\0';
 		}
+		rw_exit_write(&sysctl_kmemlock);
 		return (sysctl_rdstring(oldp, oldlenp, newp, buckstring));
 
 	case KERN_MALLOC_BUCKET:
Index: sys/kern/kern_sysctl.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
diff -u -p -r1.458 kern_sysctl.c
--- sys/kern/kern_sysctl.c	16 Dec 2024 21:22:51 -0000	1.458
+++ sys/kern/kern_sysctl.c	16 Dec 2024 22:30:39 -0000
@@ -403,6 +403,9 @@ kern_sysctl_dirs(int top_name, int *name
 	int error;
 
 	switch (top_name) {
+	case KERN_MALLOCSTATS:
+		return (sysctl_malloc(name, namelen, oldp, oldlenp,
+		    newp, newlen, p));
 #if NAUDIO > 0
 	case KERN_AUDIO:
 		return (sysctl_audio(name, namelen, oldp, oldlenp,
@@ -452,9 +455,6 @@ kern_sysctl_dirs_locked(int top_name, in
 		return (sysctl_doprof(name, namelen, oldp, oldlenp,
 		    newp, newlen));
 #endif
-	case KERN_MALLOCSTATS:
-		return (sysctl_malloc(name, namelen, oldp, oldlenp,
-		    newp, newlen, p));
 	case KERN_TTY:
 		return (sysctl_tty(name, namelen, oldp, oldlenp,
 		    newp, newlen));