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

On Thu, May 22, 2025 at 05:09:17AM +0200, Alexandr Nedvedicky wrote:
> 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.

I never looked into this proper but instead of walking the full kernel
stack, can't we start with the information in the trapframe?
Don't remember if there is a pointer to it somewhere in the pcb.
 
> 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--;
>  }
> 

-- 
:wq Claudio