Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: Avoid panics for user stacktraces in the kernel
To:
Christian Ludwig <cludwig@genua.de>
Cc:
tech@openbsd.org
Date:
Fri, 14 Feb 2025 18:34:21 +0100

Download raw body.

Thread
> Date: Fri, 14 Feb 2025 16:51:10 +0100
> From: Christian Ludwig <cludwig@genua.de>
> 
> Hi,
> 
> When collecting userland stacktraces with dt(4), the probe may run in an
> interrupt routine at IPL_VM or higher. Resolving pagefaults is a very
> bad idea in that case. It might panic or corrupt UVM data structures.
> Userland stack frames cannot be trusted, a bogus frame pointer may force
> us into a page fault.
> 
> The easiest solution is to simply disable pagefault handling when
> walking userland stacks.

So that means the stack trace will terminate early if the stack is
paged out or if somehow we walk from the right path.  Seems reasonable.

The ci_inatomic thing is something I added for inteldrm(4) to avoid
going down the path that resolves page faults in a context where we
can't sleep.  So that's why it doesn't exist on other architectures.
But we can add it to those if/when we need it.

ok kettenis@

> ---
>  sys/arch/amd64/amd64/db_trace.c | 2 ++
>  sys/arch/i386/i386/db_trace.c   | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/sys/arch/amd64/amd64/db_trace.c b/sys/arch/amd64/amd64/db_trace.c
> index 9c2ec0fc7d07..8a22b510976d 100644
> --- a/sys/arch/amd64/amd64/db_trace.c
> +++ b/sys/arch/amd64/amd64/db_trace.c
> @@ -302,6 +302,7 @@ stacktrace_save_utrace(struct stacktrace *st)
>  	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;
> @@ -322,6 +323,7 @@ stacktrace_save_utrace(struct stacktrace *st)
>  		if (copyin(frame, &f, sizeof(f)) != 0)
>  			break;
>  	}
> +	curcpu()->ci_inatomic--;
>  }
>  
>  vaddr_t
> diff --git a/sys/arch/i386/i386/db_trace.c b/sys/arch/i386/i386/db_trace.c
> index 69b55f2f4618..c021d8a7a5fa 100644
> --- a/sys/arch/i386/i386/db_trace.c
> +++ b/sys/arch/i386/i386/db_trace.c
> @@ -301,6 +301,7 @@ stacktrace_save_utrace(struct stacktrace *st)
>  	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;
> @@ -321,6 +322,7 @@ stacktrace_save_utrace(struct stacktrace *st)
>  		if (copyin(frame, &f, sizeof(f)) != 0)
>  			break;
>  	}
> +	curcpu()->ci_inatomic--;
>  }
>  
>  vaddr_t
> -- 
> 2.34.1
> 
> 
> [2:application/pkcs7-signature Show Save:smime.p7s (5kB)]
>