From: Alexander Bluhm <bluhm@openbsd.org>
Subject: syzkaller dt ioctl record race
To: tech@openbsd.org
Date: Thu, 31 Jul 2025 01:12:01 +0200

Hi,

syzkaller finds a crash in dt_ioctl_record_stop().
https://syzkaller.appspot.com/bug?extid=34f860f29dc941cfb548

Note that this also happens on single processor kernel.  The code
is protected by kernel lock.  But that means there should be no
sleeps as they release the kernel lock.  rw_enter_write(&dt_lock)
may sleep which breaks the locking assumptions.

So move the rw_enter_write() at the beginning.  This gives the
benefit that more variables are locked by this.

ok?

bluhm

Index: dev/dt/dt_dev.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/dt/dt_dev.c,v
diff -u -p -r1.44 dt_dev.c
--- dev/dt/dt_dev.c	3 Jun 2025 00:20:31 -0000	1.44
+++ dev/dt/dt_dev.c	30 Jul 2025 12:10:19 -0000
@@ -86,14 +86,20 @@
 #define DPRINTF(x...) /* nothing */
 
 /*
- * Per-CPU Event States
- *
- *  Locks used to protect struct members:
+ *  Locks used to protect struct members and variables in this file:
+ *	a	atomic
+ *	I	invariant after initialization
+ *	K	kernel lock
+ *	D	dtrace rw-lock dt_lock
  *	r	owned by thread doing read(2)
  *	c	owned by CPU
  *	s	sliced ownership, based on read/write indexes
  *	p	written by CPU, read by thread doing read(2)
  */
+
+/*
+ * Per-CPU Event States
+ */
 struct dt_cpubuf {
 	unsigned int		 dc_prod;	/* [r] read index */
 	unsigned int		 dc_cons;	/* [c] write index */
@@ -110,12 +116,6 @@ struct dt_cpubuf {
 /*
  * Descriptor associated with each program opening /dev/dt.  It is used
  * to keep track of enabled PCBs.
- *
- *  Locks used to protect struct members in this file:
- *	a	atomic
- *	K	kernel lock
- *	r	owned by thread doing read(2)
- *	I	invariant after initialization
  */
 struct dt_softc {
 	SLIST_ENTRY(dt_softc)	 ds_next;	/* [K] descriptor list */
@@ -124,7 +124,7 @@ struct dt_softc {
 	void			*ds_si;		/* [I] to defer wakeup(9) */
 
 	struct dt_pcb_list	 ds_pcbs;	/* [K] list of enabled PCBs */
-	int			 ds_recording;	/* [K] currently recording? */
+	int			 ds_recording;	/* [D] currently recording? */
 	unsigned int		 ds_evtcnt;	/* [a] # of readable evts */
 
 	struct dt_cpubuf	 ds_cpu[MAXCPUS]; /* [I] Per-cpu event states */
@@ -141,7 +141,7 @@ unsigned int			dt_nprobes;	/* [I] # of p
 SIMPLEQ_HEAD(, dt_probe)	dt_probe_list;	/* [I] list of probes */
 
 struct rwlock			dt_lock = RWLOCK_INITIALIZER("dtlk");
-volatile uint32_t		dt_tracing = 0;	/* [K] # of processes tracing */
+volatile uint32_t		dt_tracing = 0;	/* [D] # of processes tracing */
 
 int allowdt;					/* [a] */
 
@@ -519,15 +519,20 @@ dt_ioctl_record_start(struct dt_softc *s
 {
 	uint64_t now;
 	struct dt_pcb *dp;
+	int error = 0;
 
-	if (sc->ds_recording)
-		return EBUSY;
+	rw_enter_write(&dt_lock);
+	if (sc->ds_recording) {
+		error = EBUSY;
+		goto out;
+	}
 
 	KERNEL_ASSERT_LOCKED();
-	if (TAILQ_EMPTY(&sc->ds_pcbs))
-		return ENOENT;
+	if (TAILQ_EMPTY(&sc->ds_pcbs)) {
+		error = ENOENT;
+		goto out;
+	}
 
-	rw_enter_write(&dt_lock);
 	now = nsecuptime();
 	TAILQ_FOREACH(dp, &sc->ds_pcbs, dp_snext) {
 		struct dt_probe *dtp = dp->dp_dtp;
@@ -543,12 +548,12 @@ dt_ioctl_record_start(struct dt_softc *s
 			    now + dp->dp_nsecs);
 		}
 	}
-	rw_exit_write(&dt_lock);
-
 	sc->ds_recording = 1;
 	dt_tracing++;
 
-	return 0;
+ out:
+	rw_exit_write(&dt_lock);
+	return error;
 }
 
 void
@@ -556,15 +561,16 @@ dt_ioctl_record_stop(struct dt_softc *sc
 {
 	struct dt_pcb *dp;
 
-	if (!sc->ds_recording)
+	rw_enter_write(&dt_lock);
+	if (!sc->ds_recording) {
+		rw_exit_write(&dt_lock);
 		return;
+	}
 
 	DPRINTF("dt%d: pid %d disable\n", sc->ds_unit, sc->ds_pid);
 
 	dt_tracing--;
 	sc->ds_recording = 0;
-
-	rw_enter_write(&dt_lock);
 	TAILQ_FOREACH(dp, &sc->ds_pcbs, dp_snext) {
 		struct dt_probe *dtp = dp->dp_dtp;