Description Xin LI 2020-05-02 07:25:27 UTC

In r357614, sysctl_handle_string was refactored to protect against concurrent writes to sysctl node, and as a result a temporary buffer is allocated via malloc(9).

Unfortunately, netisr is initialized earlier than malloc(9), as the result, setting net.isr.dispatch="deferred" in /boot/loader.conf would cause the kernel to panic immediately:

KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x47/frame 0xffffffff8228d560
vpanic() at vpanic+0x1c7/frame 0xffffffff8228d5c0
panic() at panic+0x43/frame 0xffffffff8228d620
trap_fatal() at trap_fatal+0x4ca/frame 0xffffffff8228d6a0
trap_pfault() at trap_pfault+0xdc/frame 0xffffffff8228d720
trap() at trap+0x3f8/frame 0xffffffff8228d840
calltrap() at calltrap+0x8/frame 0xffffffff8228d840
--- trap 0xc, rip = 0xffffffff8081a985, rsp = 0xffffffff8228d910, rbp = 0xffffffff8228d960 ---
malloc() at malloc+0x95/frame 0xffffffff8228d960
sysctl_handle_string() at sysctl_handle_string+0x215/frame 0xffffffff8228d9a0
sysctl_netisr_dispatch_policy() at sysctl_netisr_dispatch_policy+0x86/frame 0xffffffff8228d9f0
sysctl_root_handler_locked() at sysctl_root_handler_locked+0xf9/frame 0xffffffff8228da40
sysctl_register_oid() at sysctl_register_oid+0x9d1/frame 0xffffffff8228dd60
sysctl_register_all() at sysctl_register_all+0x89/frame 0xffffffff8228dd80
mi_startup() at mi_startup+0x3ac/frame 0xffffffff8228ddf0
btext() at btext+0x2c
KDB: enter: panic
[ thread pid 0 tid 0 ]
Stopped at      kdb_enter+0x67: movq    $0,0xa926e6(%rip)
db>     

A very dirty (and wrong) hack would be to have a short string allocated in stack buffer and fall back to malloc() only when the string is long, but I don't like the approach as it's only papering out the actual issue.

svn diff sys/kern/kern_sysctl.c 
Index: sys/kern/kern_sysctl.c
===================================================================
--- sys/kern/kern_sysctl.c	(revision 360560)
+++ sys/kern/kern_sysctl.c	(working copy)
@@ -1646,6 +1646,7 @@ sysctl_handle_string(SYSCTL_HANDLER_ARGS)
 	char *tmparg;
 	size_t outlen;
 	int error = 0, ro_string = 0;
+	char shortbuf[256];
 
 	/*
 	 * If the sysctl isn't writable and isn't a preallocated tunable that
@@ -1666,7 +1667,11 @@ sysctl_handle_string(SYSCTL_HANDLER_ARGS)
 			tmparg = arg1;
 			outlen = strlen(tmparg) + 1;
 		} else {
-			tmparg = malloc(arg2, M_SYSCTLTMP, M_WAITOK);
+			if (arg2 <= sizeof(shortbuf)) {
+				tmparg = (char *)&shortbuf;
+			} else {
+				tmparg = malloc(arg2, M_SYSCTLTMP, M_WAITOK);
+			}
 			sx_slock(&sysctlstringlock);
 			memcpy(tmparg, arg1, arg2);
 			sx_sunlock(&sysctlstringlock);
@@ -1675,7 +1680,7 @@ sysctl_handle_string(SYSCTL_HANDLER_ARGS)
 
 		error = SYSCTL_OUT(req, tmparg, outlen);
 
-		if (!ro_string)
+		if (!ro_string && arg2 > sizeof(shortbuf))
 			free(tmparg, M_SYSCTLTMP);
 	} else {
 		if (!ro_string)
@@ -1697,11 +1702,16 @@ sysctl_handle_string(SYSCTL_HANDLER_ARGS)
 		sx_xunlock(&sysctlstringlock);
 	} else {
 		arg2 = req->newlen - req->newidx;
-		tmparg = malloc(arg2, M_SYSCTLTMP, M_WAITOK);
+		if (arg2 <= sizeof(shortbuf)) {
+			tmparg = (char *)&shortbuf;
+		} else {
+			tmparg = malloc(arg2, M_SYSCTLTMP, M_WAITOK);
+		}
 
 		error = SYSCTL_IN(req, tmparg, arg2);
 		if (error) {
-			free(tmparg, M_SYSCTLTMP);
+			if (arg2 > sizeof(shortbuf))
+				free(tmparg, M_SYSCTLTMP);
 			return (error);
 		}
 
@@ -1709,7 +1719,8 @@ sysctl_handle_string(SYSCTL_HANDLER_ARGS)
 		memcpy(arg1, tmparg, arg2);
 		((char *)arg1)[arg2] = '\0';
 		sx_xunlock(&sysctlstringlock);
-		free(tmparg, M_SYSCTLTMP);
+		if (arg2 > sizeof(shortbuf))
+			free(tmparg, M_SYSCTLTMP);
 		req->newidx += arg2;
 	}
 	return (error);