From: Mark Kettenis Subject: Re: sysctl(2): unlock KERN_SPLASSERT To: Vitaliy Makkoveev Cc: tech@openbsd.org Date: Mon, 30 Dec 2024 00:00:24 +0100 > 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. > 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) > > >