From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: more parnoia for stacktrace_save_utrace()
To: Alexandr Nedvedicky <sashan@fastmail.net>
Cc: tech@openbsd.org
Date: Sat, 02 Aug 2025 20:11:02 +0200

> Date: Sat, 2 Aug 2025 18:42:06 +0200
> From: Alexandr Nedvedicky <sashan@fastmail.net>
> 
> Hello,
> 
> </snip>
> > > -	while (frame != NULL && lastframe < frame && INKERNEL(frame)) {
> > > +	while (frame != NULL && frame >= topframe &&
> > > +	    frame <= (struct callframe *)pcb->pcb_kstack) {
> > 
> > Why not keep the "lastframe < frame" condition and just replace the
> > INKERNEL(frame) check with the "frame <= (struct callframe *)pcb->pcb_kstack"
> > check?  That is stricter since it enforces the stack order.
> > 
> 
>     yes, this makes sense.
> 
> The diff below comes with fix traversal of kernel stack
> frames. The part which deals with traversal of userland
> frames deserves its own diff.

ok kettenis@

> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/arch/amd64/amd64/db_trace.c b/sys/arch/amd64/amd64/db_trace.c
> index 624d7ed1a73..79b3499dca1 100644
> --- a/sys/arch/amd64/amd64/db_trace.c
> +++ b/sys/arch/amd64/amd64/db_trace.c
> @@ -306,7 +306,8 @@ stacktrace_save_utrace(struct stacktrace *st)
>  	/*
>  	 * skip kernel frames
>  	 */
> -	while (frame != NULL && lastframe < frame && INKERNEL(frame)) {
> +	while (frame != NULL && lastframe < frame &&
> +	    frame <= (struct callframe *)pcb->pcb_kstack) {
>  		lastframe = frame;
>  		frame = frame->f_frame;
>  	}
> diff --git a/sys/arch/i386/i386/db_trace.c b/sys/arch/i386/i386/db_trace.c
> index b7c8b68711a..677b0680846 100644
> --- a/sys/arch/i386/i386/db_trace.c
> +++ b/sys/arch/i386/i386/db_trace.c
> @@ -305,7 +305,8 @@ stacktrace_save_utrace(struct stacktrace *st)
>  	/*
>  	 * skip kernel frames
>  	 */
> -	while (frame != NULL && lastframe < frame && INKERNEL(frame)) {
> +	while (frame != NULL && lastframe < frame &&
> +	    frame <= (struct callframe *)pcb->pcb_kstack) {
>  		lastframe = frame;
>  		frame = frame->f_frame;
>  	}
>