From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: Maintain preempted copy(9) state in stacktrace_save_utrace()
To: Christian Ludwig <cludwig@genua.de>
Cc: tech@openbsd.org
Date: Sun, 30 Mar 2025 15:42:55 +0200

On 12/03/25(Wed) 21:42, Christian Ludwig wrote:
> Hi,
> 
> A btrace(8) probe can run in interrupt context which preempts copy(9).
> Care must be taken getting userland stacktraces in that situation.
> stacktrace_save_utrace() uses copyin() itself, so we need to save and
> restore the preempted context's copy(9) state.
> 
> This approach is less invasive than the previous one
> (https://marc.info/?l=openbsd-tech&m=174136347113129).

I'm still unhappy with this approach because it still uses the generic
copyin(9) entry point.  What I suggested earlier was to use a different
entry, like db_copyin().  Otherwise we can't inspect copyin(9) with
btrace(8).

> Tested on amd64 and i386.
> 
> 
>  - Christian
> 
> ---
>  sys/arch/amd64/amd64/db_trace.c | 7 +++++++
>  sys/arch/i386/i386/db_trace.c   | 7 +++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/sys/arch/amd64/amd64/db_trace.c b/sys/arch/amd64/amd64/db_trace.c
> index 1e7fcf2208ad..fe5ff40fa50b 100644
> --- a/sys/arch/amd64/amd64/db_trace.c
> +++ b/sys/arch/amd64/amd64/db_trace.c
> @@ -292,6 +292,7 @@ stacktrace_save_utrace(struct stacktrace *st)
>  {
>  	struct callframe f, *frame, *lastframe;
>  	struct pcb *pcb = curpcb;
> +	caddr_t onfault;
>  
>  	st->st_count = 0;
>  
> @@ -302,7 +303,11 @@ stacktrace_save_utrace(struct stacktrace *st)
>  	KASSERT(INKERNEL(frame));
>  	f = *frame;
>  
> +	/* CAREFUL: Might be in interrupt context and/or in copy(9). */
>  	curcpu()->ci_inatomic++;
> +	onfault = pcb->pcb_onfault;
> +	pcb->pcb_onfault = NULL;
> +
>  	while (st->st_count < STACKTRACE_MAX) {
>  		if (f.f_retaddr != 0 && !INKERNEL(f.f_retaddr))
>  			st->st_pc[st->st_count++] = f.f_retaddr;
> @@ -323,6 +328,8 @@ stacktrace_save_utrace(struct stacktrace *st)
>  		if (copyin(frame, &f, sizeof(f)) != 0)
>  			break;
>  	}
> +
> +	pcb->pcb_onfault = onfault;
>  	curcpu()->ci_inatomic--;
>  }
>  
> diff --git a/sys/arch/i386/i386/db_trace.c b/sys/arch/i386/i386/db_trace.c
> index 2e1c9faefca7..4e413dc19245 100644
> --- a/sys/arch/i386/i386/db_trace.c
> +++ b/sys/arch/i386/i386/db_trace.c
> @@ -291,6 +291,7 @@ stacktrace_save_utrace(struct stacktrace *st)
>  {
>  	struct callframe f, *frame, *lastframe;
>  	struct pcb *pcb = curpcb;
> +	caddr_t onfault;
>  
>  	st->st_count = 0;
>  
> @@ -301,7 +302,11 @@ stacktrace_save_utrace(struct stacktrace *st)
>  	KASSERT(INKERNEL(frame));
>  	f = *frame;
>  
> +	/* CAREFUL: Might be in interrupt context and/or in copy(9). */
>  	curcpu()->ci_inatomic++;
> +	onfault = pcb->pcb_onfault;
> +	pcb->pcb_onfault = NULL;
> +
>  	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 +327,8 @@ stacktrace_save_utrace(struct stacktrace *st)
>  		if (copyin(frame, &f, sizeof(f)) != 0)
>  			break;
>  	}
> +
> +	pcb->pcb_onfault = onfault;
>  	curcpu()->ci_inatomic--;
>  }
>  
> -- 
> 2.34.1