> Date: Sat, 2 Aug 2025 23:59:51 +0200
> From: Alexandr Nedvedicky <sashan@fastmail.net>
> 
> Hello,
> 
> this diff resurrects idea from diff I've sent few weeks ago [1].
> kettenis@ was not excited about that change back then.
> 
> I've sent another version earlier [2] today which was still kind
> of disappointing to kettenis@. This time we had a chance to
> discuss those changes at the table.
> 
> The outcome was that the change [3] which avoids the crash 
> went in [4]. The remaining part which I'd like to get it in
> is presented here.
> 
> the change builds an assumption into stacktrace_save_utrace()
> function that if particular stack frame contains an invalid
> address to next frame (,f_frame is bogus address), then
> the return address kept in frame is also invalid.
> 
> stop reading here, if your are not interested in details.
> 
> the reason why I don't want to pass potentially invalid
> instruction address to btrace(8) is the cost the btrace
> process needs to pay to figure out the address does not
> point to instruction in traced process.
> 
> btrace process tries to resolve address to symbol. if
> it finds out there is no matching symbol for presented address,
> it uses ioctl(2) on dt(4) to query kernel which .so object
> the instruction belongs to. If instruction belongs to so .so
> in traced process, the kernel opens a file descriptor in
> btrace file descriptor table and associates a vnode which
> refers to .so with it. It then passes the file descriptor
> number back to btrace process, so btrace can use mmap(2)
> and libelf to load symbol table.
> 
> Works nicely as long as btrace process is presented with
> valid addresses. However if traced process happens not
> to use frame pointers correctly then stacktrace_save_utrace()
> function may misinterpret frame and return invalid address
> to btrace. btrace then does ioctl(2) just to find out
> the address is invalid.
> 
> after a brief discussion kettenis@ agrees current simple
> approach proposed here seems to be good enough for now.
> the alternative option would be to implement a negative
> caching in btrace. the btrace could remember addresses
> (page ranges) the dt(5) failed to resolve to symbol tables.
> This is something that can be done later.
> 
> diff follows.

Thanks Sasha,

I still have my doubts about whether the kernel should enforce this
policy, but I think enforcing the policy consistently and avoiding
taking vmmap lock in this code is better.  So ok kettenis@ for the diff.

I just have a suggestion for changing the text of the comment.

> 
> [1] https://marc.info/?l=openbsd-tech&m=174938917209440&w=2
> 
> [2] https://marc.info/?l=openbsd-tech&m=175414134428800&w=2
> 
> [3] https://marc.info/?l=openbsd-tech&m=175415293201212&w=2
> 
> [4] https://marc.info/?l=openbsd-cvs&m=175416247805522&w=2
> 
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/arch/amd64/amd64/db_trace.c b/sys/arch/amd64/amd64/db_trace.c
> index 5279024e04d..c4499042036 100644
> --- a/sys/arch/amd64/amd64/db_trace.c
> +++ b/sys/arch/amd64/amd64/db_trace.c
> @@ -319,8 +319,19 @@ stacktrace_save_utrace(struct stacktrace *st)
>  		st->st_pc[st->st_count++] = lastframe->f_retaddr;
>  
>  	while (frame != NULL && st->st_count < STACKTRACE_MAX) {
> -		if (copyin(frame, &f, sizeof(f)) != 0)
> +		if (copyin(frame, &f, sizeof(f)) != 0) {
> +			/*
> +			 * We assume the return address from previous frame is
> +			 * invalid, because the frame refers to invalid
> +			 * address. We don't want to provide invalid address
> +			 * to btrace(8) in userland.
> +			 */

			 /*
			  * If the frame pointer read from the previous frame
			  * is invalid, assume the return address we read
			  * from that frame is invalid as well.
			   */

> +			if (st->st_count == 0)
> +				st->st_pc[0] = 0;
> +			else
> +				st->st_count--;
>  			break;
> +		}
>  		st->st_pc[st->st_count++] = f.f_retaddr;
>  		frame = f.f_frame;
>  	}
> diff --git a/sys/arch/i386/i386/db_trace.c b/sys/arch/i386/i386/db_trace.c
> index 140ba9eca33..086afb11a5a 100644
> --- a/sys/arch/i386/i386/db_trace.c
> +++ b/sys/arch/i386/i386/db_trace.c
> @@ -318,8 +318,19 @@ stacktrace_save_utrace(struct stacktrace *st)
>  		st->st_pc[st->st_count++] = lastframe->f_retaddr;
>  
>  	while (frame != NULL && st->st_count < STACKTRACE_MAX) {
> -		if (copyin(frame, &f, sizeof(f)) != 0)
> +		if (copyin(frame, &f, sizeof(f)) != 0) {
> +			/*
> +			 * We assume the return address from previous frame is
> +			 * invalid, because the frame refers to invalid
> +			 * address. We don't want to provide invalid address
> +			 * to btrace(8) in userland.
> +			 */
> +			if (st->st_count == 0)
> +				st->st_pc[0] = 0;
> +			else
> +				st->st_count--;
>  			break;
> +		}
>  		st->st_pc[st->st_count++] = f.f_retaddr;
>  		frame = f.f_frame;
>  	}
> 
>