Bug 246114 - net.isr.dispatch can not be set via loader
Summary: net.isr.dispatch can not be set via loader
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Pawel Biernacki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-02 07:25 UTC by Xin LI
Modified: 2020-05-16 17:07 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Xin LI freebsd_committer 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);
Comment 1 Yuri Pankov 2020-05-02 07:27:25 UTC
base r359975 was supposed to fix this, it didn't for you?
Comment 2 Yuri Pankov 2020-05-02 07:31:26 UTC
(In reply to Yuri Pankov from comment #1)
OK, I see it didn't, sorry for the noise.
Comment 3 Xin LI freebsd_committer 2020-05-02 07:46:24 UTC
+hselasky@ -- It looks like base r267961 changed mallocinit from SI_ORDER_FIRST to SI_ORDER_SECOND, but I don't quite follow the motivation, would you please take a look?
Comment 4 Pawel Biernacki freebsd_committer 2020-05-15 21:47:24 UTC
So, as we spoke yesterday, I'd like go wait for hselasky@'s comment's on the SYSINIT change for mallocinit.  If the mallocinit can't be changed back to SI_ORDER_FIRST, given that there isn't (too many) other cases like this, IMO we should consider changing sysctl_netisr_dispatch_policy().
Comment 5 Xin LI freebsd_committer 2020-05-15 22:11:15 UTC
(In reply to Pawel Biernacki from comment #4)
Sorry I missed the discussion on IRC.

The ordering is a separate issue (even if we change it back to SI_ORDER_FIRST it would still panic; I was curious why it was changed to SI_ORDER_SECOND now as there is no obvious reason and obviously having malloc() initialized later would likely to expose more issues).
Comment 6 Pawel Biernacki freebsd_committer 2020-05-16 10:37:24 UTC
https://reviews.freebsd.org/D24858
Comment 7 commit-hook freebsd_committer 2020-05-16 17:06:15 UTC
A commit references this bug:

Author: kaktus
Date: Sat May 16 17:05:44 UTC 2020
New revision: 361113
URL: https://svnweb.freebsd.org/changeset/base/361113

Log:
  sysctl: fix setting net.isr.dispatch during early boot

  Fix another collateral damage of r357614: netisr is initialised way before
  malloc() is available hence it can't use sysctl_handle_string() that
  allocates temporary buffer.  Handle that internally in
  sysctl_netisr_dispatch_policy().

  PR:		246114
  Reported by:	delphij
  Reviewed by:	kib
  Approved by:	kib (mentor)
  Differential Revision:	https://reviews.freebsd.org/D24858

Changes:
  head/sys/net/netisr.c