From: Vitaliy Makkoveev Subject: Re: sysctl(2): unlock KERN_SPLASSERT To: Mark Kettenis Cc: OpenBSD tech Date: Mon, 30 Dec 2024 05:44:14 +0300 > On 30 Dec 2024, at 02:00, Mark Kettenis wrote: > >> Date: Sun, 29 Dec 2024 20:43:40 +0300 >> From: Vitaliy Makkoveev >> >> `splassert_ctl' is atomically accessed integer which accessed lockless >> for a long time. > > Sorry, but no, I don't think this is ok. This solves nothing, but it > does add an extra include to that increases the risk > that folks start failing to explicitly include in > places where it is needed. > sys/atomic.h has #ifndef _SYS_ATOMIC_H_ guard against it. >> Index: sys/arch/alpha/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/alpha/include/intr.h,v >> diff -u -p -r1.50 intr.h >> --- sys/arch/alpha/include/intr.h 17 May 2024 20:07:33 -0000 1.50 >> +++ sys/arch/alpha/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -147,6 +147,9 @@ void intr_barrier(void *); >> >> /* SPL asserts */ >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -156,7 +159,7 @@ extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) \ >> do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/amd64/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/amd64/include/intr.h,v >> diff -u -p -r1.34 intr.h >> --- sys/arch/amd64/include/intr.h 26 May 2024 13:37:31 -0000 1.34 >> +++ sys/arch/amd64/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -152,6 +152,9 @@ void softintr(int); >> >> /* SPL asserts */ >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -160,7 +163,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/arm64/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/arm64/include/intr.h,v >> diff -u -p -r1.23 intr.h >> --- sys/arch/arm64/include/intr.h 14 Oct 2024 10:08:13 -0000 1.23 >> +++ sys/arch/arm64/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -199,6 +199,9 @@ extern void (*intr_send_ipi_func)(struct >> #define ARM_IPI_HALT 2 >> >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -207,7 +210,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void arm_splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> arm_splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/armv7/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/armv7/include/intr.h,v >> diff -u -p -r1.15 intr.h >> --- sys/arch/armv7/include/intr.h 14 Oct 2024 10:08:13 -0000 1.15 >> +++ sys/arch/armv7/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -202,6 +202,9 @@ extern void (*intr_send_ipi_func)(struct >> #define ARM_IPI_DDB 1 >> >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -210,8 +213,8 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void arm_splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> - arm_splassert_check(__wantipl, __func__); \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> + arm_splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> #define splsoftassert(wantipl) splassert(wantipl) >> Index: sys/arch/hppa/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/hppa/include/intr.h,v >> diff -u -p -r1.44 intr.h >> --- sys/arch/hppa/include/intr.h 13 Jan 2018 15:18:11 -0000 1.44 >> +++ sys/arch/hppa/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -71,11 +71,14 @@ >> extern volatile u_long imask[NIPL]; >> >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> void splassert_fail(int, int, const char *); >> extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/i386/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/i386/include/intr.h,v >> diff -u -p -r1.50 intr.h >> --- sys/arch/i386/include/intr.h 26 May 2024 13:37:32 -0000 1.50 >> +++ sys/arch/i386/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -64,6 +64,9 @@ extern void softintr(int); >> >> /* SPL asserts */ >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -72,7 +75,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/landisk/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/landisk/include/intr.h,v >> diff -u -p -r1.14 intr.h >> --- sys/arch/landisk/include/intr.h 20 Aug 2018 15:02:07 -0000 1.14 >> +++ sys/arch/landisk/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -66,6 +66,9 @@ >> #define splx(x) _cpu_intr_resume(x) >> >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -75,7 +78,7 @@ extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) \ >> do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/loongson/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/loongson/include/intr.h,v >> diff -u -p -r1.18 intr.h >> --- sys/arch/loongson/include/intr.h 5 Sep 2019 05:31:38 -0000 1.18 >> +++ sys/arch/loongson/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -129,6 +129,9 @@ void softintr_schedule(void *); >> void splinit(void); >> >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -137,7 +140,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/m88k/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/m88k/include/intr.h,v >> diff -u -p -r1.14 intr.h >> --- sys/arch/m88k/include/intr.h 20 Aug 2018 15:02:07 -0000 1.14 >> +++ sys/arch/m88k/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -72,6 +72,9 @@ int spl0(void); >> >> /* SPL asserts */ >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -80,7 +83,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/octeon/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/octeon/include/intr.h,v >> diff -u -p -r1.22 intr.h >> --- sys/arch/octeon/include/intr.h 5 Sep 2019 05:31:38 -0000 1.22 >> +++ sys/arch/octeon/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -128,6 +128,9 @@ void softintr_schedule(void *); >> void splinit(void); >> >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -136,7 +139,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/powerpc/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/powerpc/include/intr.h,v >> diff -u -p -r1.56 intr.h >> --- sys/arch/powerpc/include/intr.h 20 Aug 2018 15:02:07 -0000 1.56 >> +++ sys/arch/powerpc/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -90,6 +90,9 @@ void do_pending_int(void); >> >> /* SPL asserts */ >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -98,7 +101,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/powerpc64/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/powerpc64/include/intr.h,v >> diff -u -p -r1.14 intr.h >> --- sys/arch/powerpc64/include/intr.h 30 May 2021 15:06:53 -0000 1.14 >> +++ sys/arch/powerpc64/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -65,6 +65,9 @@ void splx(int); >> #define splhigh() splraise(IPL_HIGH) >> >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -73,7 +76,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/riscv64/include/intr.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/riscv64/include/intr.h,v >> diff -u -p -r1.8 intr.h >> --- sys/arch/riscv64/include/intr.h 16 Oct 2024 02:32:27 -0000 1.8 >> +++ sys/arch/riscv64/include/intr.h 29 Dec 2024 17:16:16 -0000 >> @@ -207,6 +207,9 @@ void riscv_intr_cpu_enable(void); >> void intr_send_ipi(struct cpu_info *, int); >> >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -215,7 +218,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void riscv_splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> riscv_splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/arch/sparc64/include/psl.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/sparc64/include/psl.h,v >> diff -u -p -r1.37 psl.h >> --- sys/arch/sparc64/include/psl.h 6 Nov 2024 07:11:14 -0000 1.37 >> +++ sys/arch/sparc64/include/psl.h 29 Dec 2024 17:16:16 -0000 >> @@ -205,6 +205,9 @@ >> #if defined(_KERNEL) && !defined(_LOCORE) >> >> #ifdef DIAGNOSTIC >> + >> +#include >> + >> /* >> * Although this function is implemented in MI code, it must be in this MD >> * header because we don't want this header to include MI includes. >> @@ -213,7 +216,7 @@ void splassert_fail(int, int, const char >> extern int splassert_ctl; >> void splassert_check(int, const char *); >> #define splassert(__wantipl) do { \ >> - if (splassert_ctl > 0) { \ >> + if (atomic_load_int(&splassert_ctl) > 0) { \ >> splassert_check(__wantipl, __func__); \ >> } \ >> } while (0) >> Index: sys/dev/ic/iosf.c >> =================================================================== >> RCS file: /cvs/src/sys/dev/ic/iosf.c,v >> diff -u -p -r1.2 iosf.c >> --- sys/dev/ic/iosf.c 26 Jun 2024 01:40:49 -0000 1.2 >> +++ sys/dev/ic/iosf.c 29 Dec 2024 17:16:16 -0000 >> @@ -258,7 +258,7 @@ iosf_mbi_assert_punit_acquired(void) >> { >> int s; >> >> - if (splassert_ctl == 0) >> + if (atomic_load_int(&splassert_ctl) == 0) >> return; >> >> s = rw_status(&iosf_lock); >> Index: sys/kern/kern_sysctl.c >> =================================================================== >> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v >> diff -u -p -r1.459 kern_sysctl.c >> --- sys/kern/kern_sysctl.c 28 Dec 2024 20:34:05 -0000 1.459 >> +++ sys/kern/kern_sysctl.c 29 Dec 2024 17:16:17 -0000 >> @@ -614,6 +614,7 @@ kern_sysctl(int *name, u_int namelen, vo >> case KERN_FSCALE: >> case KERN_CCPU: >> case KERN_NPROCS: >> + case KERN_SPLASSERT: >> case KERN_WXABORT: >> case KERN_NETLIVELOCKS: >> case KERN_GLOBAL_PTRACE: >> Index: sys/kern/subr_prf.c >> =================================================================== >> RCS file: /cvs/src/sys/kern/subr_prf.c,v >> diff -u -p -r1.106 subr_prf.c >> --- sys/kern/subr_prf.c 14 Aug 2022 01:58:28 -0000 1.106 >> +++ sys/kern/subr_prf.c 29 Dec 2024 17:16:17 -0000 >> @@ -89,6 +89,11 @@ struct mutex kprintf_mutex = >> MUTEX_INITIALIZER_FLAGS(IPL_HIGH, "kprintf", MTX_NOWITNESS); >> >> /* >> + * Locks used to protect data: >> + * a atomic >> + */ >> + >> +/* >> * globals >> */ >> >> @@ -119,9 +124,9 @@ int db_console = 0; >> * panic on spl assertion failure? >> */ >> #ifdef SPLASSERT_WATCH >> -int splassert_ctl = 3; >> +int splassert_ctl = 3; /* [a] */ >> #else >> -int splassert_ctl = 1; >> +int splassert_ctl = 1; /* [a] */ >> #endif >> >> /* >> @@ -199,7 +204,7 @@ panic(const char *fmt, ...) >> bootopt |= RB_NOSYNC; >> >> /* do not trigger assertions, we know that we are inconsistent */ >> - splassert_ctl = 0; >> + atomic_store_int(&splassert_ctl, 0); >> >> #ifdef BOOT_QUIET >> printf_flags |= TOCONS; /* make sure we see kernel printf output */ >> @@ -243,7 +248,7 @@ splassert_fail(int wantipl, int haveipl, >> return; >> >> printf("splassert: %s: want %d have %d\n", func, wantipl, haveipl); >> - switch (splassert_ctl) { >> + switch (atomic_load_int(&splassert_ctl)) { >> case 1: >> break; >> case 2: >> Index: sys/kern/uipc_socket2.c >> =================================================================== >> RCS file: /cvs/src/sys/kern/uipc_socket2.c,v >> diff -u -p -r1.161 uipc_socket2.c >> --- sys/kern/uipc_socket2.c 26 Dec 2024 12:16:17 -0000 1.161 >> +++ sys/kern/uipc_socket2.c 29 Dec 2024 17:16:17 -0000 >> @@ -460,7 +460,7 @@ soassertlocked(struct socket *so) >> if (rw_status(&netlock) == RW_READ) { >> NET_ASSERT_LOCKED(); >> >> - if (splassert_ctl > 0 && >> + if (atomic_load_int(&splassert_ctl) > 0 && >> rw_status(&so->so_lock) != RW_WRITE) >> splassert_fail(0, RW_WRITE, __func__); >> } else >> @@ -503,7 +503,8 @@ void >> sbmtxassertlocked(struct socket *so, struct sockbuf *sb) >> { >> if (sb->sb_flags & SB_MTXLOCK) { >> - if (splassert_ctl > 0 && mtx_owned(&sb->sb_mtx) == 0) >> + if (atomic_load_int(&splassert_ctl) > 0 && >> + mtx_owned(&sb->sb_mtx) == 0) >> splassert_fail(0, RW_WRITE, __func__); >> } else >> soassertlocked(so); >> Index: sys/sys/systm.h >> =================================================================== >> RCS file: /cvs/src/sys/sys/systm.h,v >> diff -u -p -r1.171 systm.h >> --- sys/sys/systm.h 28 May 2024 12:50:23 -0000 1.171 >> +++ sys/sys/systm.h 29 Dec 2024 17:16:17 -0000 >> @@ -347,21 +347,22 @@ extern struct rwlock netlock; >> #define NET_ASSERT_UNLOCKED() \ >> do { \ >> int _s = rw_status(&netlock); \ >> - if ((splassert_ctl > 0) && (_s == RW_WRITE)) \ >> + if ((atomic_load_int(&splassert_ctl) > 0) && (_s == RW_WRITE)) \ >> splassert_fail(0, RW_WRITE, __func__); \ >> } while (0) >> >> #define NET_ASSERT_LOCKED() \ >> do { \ >> int _s = rw_status(&netlock); \ >> - if ((splassert_ctl > 0) && (_s != RW_WRITE && _s != RW_READ)) \ >> + if ((atomic_load_int(&splassert_ctl) > 0) && \ >> + (_s != RW_WRITE && _s != RW_READ)) \ >> splassert_fail(RW_READ, _s, __func__); \ >> } while (0) >> >> #define NET_ASSERT_LOCKED_EXCLUSIVE() \ >> do { \ >> int _s = rw_status(&netlock); \ >> - if ((splassert_ctl > 0) && (_s != RW_WRITE)) \ >> + if ((atomic_load_int(&splassert_ctl) > 0) && (_s != RW_WRITE)) \ >> splassert_fail(RW_WRITE, _s, __func__); \ >> } while (0) >> >> >> >