From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: sysctl(2): unlock sysctl_malloc()
To: tech@openbsd.org, Martin Pieuchot <mpi@openbsd.org>
Date: Sat, 28 Dec 2024 08:28:36 +0300

On Tue, Dec 17, 2024 at 01:33:26AM +0300, Vitaliy Makkoveev wrote:
> 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. 
> 

The new one. Against previous, statically allocated `buckstring' became
dynamically allocated each time when KERN_MALLOC_BUCKETS case visited.
This string filled with immutable data, no extra locks required. This
diff makes kern.malloc.buckets access a little bit longer, but as I said
before, this path visited very rare.

Martin, malloc(9) is a part of VM area, so I ask you to review.

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	28 Dec 2024 04:47:10 -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,17 +95,15 @@ 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;
 #if defined(KMEMSTATS) || defined(DIAGNOSTIC)
 char *memname[] = INITKMEMNAMES;
-char *memall = NULL;
+char *memall = NULL;					/* [S] */
 struct rwlock sysctl_kmemlock = RWLOCK_INITIALIZER("sysctlklk");
 #endif
 
@@ -601,23 +605,26 @@ sysctl_malloc(int *name, u_int namelen, 
 		return (ENOTDIR);		/* overloaded */
 
 	switch (name[0]) {
-	case KERN_MALLOC_BUCKETS:
-		/* Initialize the first time */
-		if (buckstring_init == 0) {
-			buckstring_init = 1;
-			memset(buckstring, 0, sizeof(buckstring));
-			for (siz = 0, i = MINBUCKET; i < MINBUCKET + 16; i++) {
-				snprintf(buckstring + siz,
-				    sizeof buckstring - siz,
-				    "%d,", (u_int)(1<<i));
-				siz += strlen(buckstring + siz);
-			}
-			/* Remove trailing comma */
-			if (siz)
-				buckstring[siz - 1] = '\0';
+	case KERN_MALLOC_BUCKETS: {
+		static const size_t buckstring_sz=(16 * sizeof("123456,"));
+		char *buckstring;
+
+		buckstring = malloc(buckstring_sz, M_TEMP, M_WAITOK|M_ZERO);
+
+		for (siz = 0, i = MINBUCKET; i < MINBUCKET + 16; i++) {
+			snprintf(buckstring + siz, buckstring_sz - siz,
+			    "%d,", (u_int)(1<<i));
+			siz += strlen(buckstring + siz);
 		}
-		return (sysctl_rdstring(oldp, oldlenp, newp, buckstring));
+		/* Remove trailing comma */
+		if (siz)
+			buckstring[siz - 1] = '\0';
 
+		error = sysctl_rdstring(oldp, oldlenp, newp, buckstring);
+		free(buckstring, M_TEMP, buckstring_sz);
+
+		return (error);
+	}
 	case KERN_MALLOC_BUCKET:
 		mtx_enter(&malloc_mtx);
 		memcpy(&kb, &bucket[BUCKETINDX(name[1])], sizeof(kb));
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	28 Dec 2024 04:47:10 -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));