git: 04716d51ba5b - stable/14 - ithread: Improve synchronization in ithread_destroy()

The branch stable/14 has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=04716d51ba5bb3b78f244d4acc6b67d933d2a0f1

commit 04716d51ba5bb3b78f244d4acc6b67d933d2a0f1
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2024-07-30 14:36:54 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2024-08-20 13:27:05 +0000

    ithread: Improve synchronization in ithread_destroy()
    
    Previously, to destroy an ithread we would set IT_DEAD in its flags, and
    then wake it up if it wasn't already running.  After doing this,
    intr_event_destroy() would free the intr_event structure.  However, it
    did not wait for the ithread to exit, so it was possible for the ithread
    to access the intr_event after it was freed.
    
    This use-after-free happens readily when running the pf tests in
    parallel, since they frequently create and destroy VNET jails, and pf
    registers several VNET-local swi handlers.
    
    Fix the race by modifying ithread_destroy() to wait until the ithread
    has signaled that it is about to exit by setting ie->ie_thread = NULL.
    Existing callers of intr_event_destroy() are allowed to sleep.
    
    Reported by:    KASAN
    Reviewed by:    kib, jhb
    MFC after:      3 weeks
    Differential Revision:  https://reviews.freebsd.org/D45492
    
    (cherry picked from commit 8381e9f49ec733437754a822ef2e8344115289ac)
---
 sys/kern/kern_intr.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/sys/kern/kern_intr.c b/sys/kern/kern_intr.c
index 2e8ed6c5ece9..739b4ea4b2b5 100644
--- a/sys/kern/kern_intr.c
+++ b/sys/kern/kern_intr.c
@@ -541,14 +541,10 @@ intr_event_destroy(struct intr_event *ie)
 		return (EBUSY);
 	}
 	TAILQ_REMOVE(&event_list, ie, ie_list);
-#ifndef notyet
-	if (ie->ie_thread != NULL) {
+	mtx_unlock(&event_lock);
+	if (ie->ie_thread != NULL)
 		ithread_destroy(ie->ie_thread);
-		ie->ie_thread = NULL;
-	}
-#endif
 	mtx_unlock(&ie->ie_lock);
-	mtx_unlock(&event_lock);
 	mtx_destroy(&ie->ie_lock);
 	free(ie, M_ITHREAD);
 	return (0);
@@ -581,10 +577,16 @@ ithread_create(const char *name)
 static void
 ithread_destroy(struct intr_thread *ithread)
 {
+	struct intr_event *ie;
 	struct thread *td;
 
-	CTR2(KTR_INTR, "%s: killing %s", __func__, ithread->it_event->ie_name);
 	td = ithread->it_thread;
+	ie = ithread->it_event;
+
+	mtx_assert(&ie->ie_lock, MA_OWNED);
+
+	CTR2(KTR_INTR, "%s: killing %s", __func__, ie->ie_name);
+
 	thread_lock(td);
 	ithread->it_flags |= IT_DEAD;
 	if (TD_AWAITING_INTR(td)) {
@@ -592,6 +594,8 @@ ithread_destroy(struct intr_thread *ithread)
 		sched_wakeup(td, SRQ_INTR);
 	} else
 		thread_unlock(td);
+	while (ie->ie_thread != NULL)
+		msleep(ithread, &ie->ie_lock, 0, "ithd_dth", 0);
 }
 
 int
@@ -1235,7 +1239,7 @@ ithread_loop(void *arg)
 	struct intr_event *ie;
 	struct thread *td;
 	struct proc *p;
-	int wake, epoch_count;
+	int epoch_count;
 	bool needs_epoch;
 
 	td = curthread;
@@ -1245,7 +1249,6 @@ ithread_loop(void *arg)
 	    ("%s: ithread and proc linkage out of sync", __func__));
 	ie = ithd->it_event;
 	ie->ie_count = 0;
-	wake = 0;
 
 	/*
 	 * As long as we have interrupts outstanding, go through the
@@ -1255,9 +1258,14 @@ ithread_loop(void *arg)
 		/*
 		 * If we are an orphaned thread, then just die.
 		 */
-		if (ithd->it_flags & IT_DEAD) {
+		if (__predict_false((ithd->it_flags & IT_DEAD) != 0)) {
 			CTR3(KTR_INTR, "%s: pid %d (%s) exiting", __func__,
 			    p->p_pid, td->td_name);
+			mtx_lock(&ie->ie_lock);
+			ie->ie_thread = NULL;
+			wakeup(ithd);
+			mtx_unlock(&ie->ie_lock);
+
 			free(ithd, M_ITHREAD);
 			kthread_exit();
 		}
@@ -1302,17 +1310,12 @@ ithread_loop(void *arg)
 			TD_SET_IWAIT(td);
 			ie->ie_count = 0;
 			mi_switch(SW_VOL | SWT_IWAIT);
-		} else {
-			if (ithd->it_flags & IT_WAIT) {
-				wake = 1;
-				ithd->it_flags &= ~IT_WAIT;
-			}
+		} else if ((ithd->it_flags & IT_WAIT) != 0) {
+			ithd->it_flags &= ~IT_WAIT;
 			thread_unlock(td);
-		}
-		if (wake) {
 			wakeup(ithd);
-			wake = 0;
-		}
+		} else
+			thread_unlock(td);
 	}
 }