From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: Re: stacktrace_save_utrace() should be more robust
To: Ted Unangst <tedu@tedunangst.com>
Cc: Mark Kettenis <mark.kettenis@xs4all.nl>, tech@openbsd.org
Date: Thu, 22 May 2025 05:09:17 +0200

Hello,

On Wed, May 21, 2025 at 08:15:48PM -0400, Ted Unangst wrote:
</snip>
> 
> Is it possible to use two loops here? One loop to get out of the kernel,
> and then the second that does the copyin? I think there is not so much
> code in common that duplicating is a problem, and it will be more clear
> what we're trying to do in each loop.
> 

    I like this idea. it makes code lot more clear.
    updated diff follows.

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/arch/amd64/amd64/db_trace.c b/sys/arch/amd64/amd64/db_trace.c
index 1e7fcf2208a..c1292af7d23 100644
--- a/sys/arch/amd64/amd64/db_trace.c
+++ b/sys/arch/amd64/amd64/db_trace.c
@@ -298,30 +298,30 @@ stacktrace_save_utrace(struct stacktrace *st)
 	if (pcb == NULL)
 		return;
 
+	lastframe = NULL;
 	frame = __builtin_frame_address(0);
 	KASSERT(INKERNEL(frame));
-	f = *frame;
 
 	curcpu()->ci_inatomic++;
-	while (st->st_count < STACKTRACE_MAX) {
-		if (f.f_retaddr != 0 && !INKERNEL(f.f_retaddr))
-			st->st_pc[st->st_count++] = f.f_retaddr;
-
+	/*
+	 * skip kernel frames
+	 */
+	while (frame != NULL && lastframe < frame && INKERNEL(frame)) {
 		lastframe = frame;
-		frame = f.f_frame;
+		frame = frame->f_frame;
+	}
 
-		if (frame == NULL)
-			break;
-		if (INKERNEL(f.f_retaddr)) {
-			if (frame <= lastframe)
-				break;
-			f = *frame;
-			continue;
-		}
-		if (!INKERNEL(lastframe) && frame <= lastframe)
-			break;
+	/*
+	 * start saving userland frames
+	 */
+	if (lastframe != NULL)
+		st->st_pc[st->st_count++] = lastframe->f_retaddr;
+
+	while (frame != NULL && st->st_count < STACKTRACE_MAX) {
 		if (copyin(frame, &f, sizeof(f)) != 0)
 			break;
+		st->st_pc[st->st_count++] = f.f_retaddr;
+		frame = f.f_frame;
 	}
 	curcpu()->ci_inatomic--;
 }
diff --git a/sys/arch/i386/i386/db_trace.c b/sys/arch/i386/i386/db_trace.c
index 2e1c9faefca..f30150b88c8 100644
--- a/sys/arch/i386/i386/db_trace.c
+++ b/sys/arch/i386/i386/db_trace.c
@@ -297,30 +297,30 @@ stacktrace_save_utrace(struct stacktrace *st)
 	if (pcb == NULL)
 		return;
 
+	lastframe = NULL;
 	frame = __builtin_frame_address(0);
 	KASSERT(INKERNEL(frame));
-	f = *frame;
 
 	curcpu()->ci_inatomic++;
-	while (st->st_count < STACKTRACE_MAX) {
-		if (f.f_retaddr != 0 && !INKERNEL(f.f_retaddr))
-			st->st_pc[st->st_count++] = f.f_retaddr;
-
+	/*
+	 * skip kernel frames
+	 */
+	while (frame != NULL && lastframe < frame && INKERNEL(frame)) {
 		lastframe = frame;
-		frame = f.f_frame;
+		frame = frame->f_frame;
+	}
 
-		if (frame == NULL)
-			break;
-		if (INKERNEL(f.f_retaddr)) {
-			if (frame <= lastframe)
-				break;
-			f = *frame;
-			continue;
-		}
-		if (!INKERNEL(lastframe) && frame <= lastframe)
-			break;
+	/*
+	 * start saving userland frames
+	 */
+	if (lastframe != NULL)
+		st->st_pc[st->st_count++] = lastframe->f_retaddr;
+
+	while (frame != NULL && st->st_count < STACKTRACE_MAX) {
 		if (copyin(frame, &f, sizeof(f)) != 0)
 			break;
+		st->st_pc[st->st_count++] = f.f_retaddr;
+		frame = f.f_frame;
 	}
 	curcpu()->ci_inatomic--;
 }