git: ae77041e0714 - main - kthread: Set *newtdp earlier in kthread_add1()

The branch main has been updated by markj:

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

commit ae77041e0714627f9ec8045ca9ee2b6ea563138e
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2023-12-09 15:22:06 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-12-09 19:11:33 +0000

    kthread: Set *newtdp earlier in kthread_add1()
    
    syzbot reported a single boot-time crash in g_event_procbody(), a page
    fault when dereferencing g_event_td.  g_event_td is initialized by the
    kproc_kthread_add() call which creates the GEOM event thread:
    
      kproc_kthread_add(g_event_procbody, NULL, &g_proc, &g_event_td,
          RFHIGHPID, 0, "geom", "g_event");
    
    I believe that the caller of kproc_kthread_add() was preempted after
    adding the new thread to the scheduler, and before setting *newtdp,
    which is equal to g_event_td.  Thus, since the first action of the GEOM
    event thread is to lock itself, it ended up dereferencing a NULL
    pointer.
    
    Fix the problem simply by initializing *newtdp earlier.  I see no harm
    in that, and it matches kproc_create1().  The scheduler provides
    sufficient synchronization to ensure that the store is visible to the
    new thread, wherever it happens to run.
    
    Reported by:    syzbot+5397f4d39219b85a9409@syzkaller.appspotmail.com
    Reviewed by:    kib
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D42986
---
 sys/kern/kern_kthread.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sys/kern/kern_kthread.c b/sys/kern/kern_kthread.c
index 3205ced9b9fd..8a84fd70918d 100644
--- a/sys/kern/kern_kthread.c
+++ b/sys/kern/kern_kthread.c
@@ -286,6 +286,13 @@ kthread_add1(void (*func)(void *), void *arg, struct proc *p,
 	}
 	oldtd = FIRST_THREAD_IN_PROC(p);
 
+	/*
+	 * Set the new thread pointer before the thread starts running: *newtdp
+	 * could be a pointer that is referenced by "func".
+	 */
+	if (newtdp != NULL)
+		*newtdp = newtd;
+
 	bzero(&newtd->td_startzero,
 	    __rangeof(struct thread, td_startzero, td_endzero));
 	bcopy(&oldtd->td_startcopy, &newtd->td_startcopy,
@@ -330,8 +337,6 @@ kthread_add1(void (*func)(void *), void *arg, struct proc *p,
 		thread_lock(newtd);
 		sched_add(newtd, SRQ_BORING); 
 	}
-	if (newtdp)
-		*newtdp = newtd;
 	return (0);
 }