Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: sysctl(2): unlock KERN_SPLASSERT
To:
Vitaliy Makkoveev <mvs@openbsd.org>
Cc:
tech@openbsd.org
Date:
Mon, 30 Dec 2024 00:00:24 +0100

Download raw body.

Thread
> Date: Sun, 29 Dec 2024 20:43:40 +0300
> From: Vitaliy Makkoveev <mvs@openbsd.org>
> 
> `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 <machine/intr.h> that increases the risk
that folks start failing to explicitly include <sys/atomic.h> 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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 <sys/atomic.h>
> +
>  /*
>   * 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)
>  
> 
>