linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: madvenka@linux.microsoft.com
Cc: broonie@kernel.org, jpoimboe@redhat.com, ardb@kernel.org,
	nobuta.keiya@fujitsu.com, sjitindarsingh@gmail.com,
	catalin.marinas@arm.com, will@kernel.org, jmorris@namei.org,
	pasha.tatashin@soleen.com, jthierry@redhat.com,
	linux-arm-kernel@lists.infradead.org,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v6 1/3] arm64: Improve the unwinder return value
Date: Wed, 28 Jul 2021 17:56:35 +0100	[thread overview]
Message-ID: <20210728165635.GA47345@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20210630223356.58714-2-madvenka@linux.microsoft.com>

On Wed, Jun 30, 2021 at 05:33:54PM -0500, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> Currently, the unwinder returns a tri-state return value:
> 
> 	0		means "continue with the unwind"
> 	-ENOENT		means "successful termination of the stack trace"
> 	-EINVAL		means "fatal error, abort the stack trace"
> 
> This is confusing. To fix this, define an enumeration of different return
> codes to make it clear. Handle the return codes in all of the unwind
> consumers.

I agree the tri-state is confusing, and I also generally agree that
enums are preferabel to a set of error codes. However, I don't think
this is quite the right abstraction; more on that below.

> 
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> ---
>  arch/arm64/include/asm/stacktrace.h | 14 ++++++--
>  arch/arm64/kernel/perf_callchain.c  |  5 ++-
>  arch/arm64/kernel/process.c         |  8 +++--
>  arch/arm64/kernel/return_address.c  | 10 ++++--
>  arch/arm64/kernel/stacktrace.c      | 53 ++++++++++++++++-------------
>  arch/arm64/kernel/time.c            |  9 +++--
>  6 files changed, 64 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index eb29b1fe8255..6fcd58553fb1 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -30,6 +30,12 @@ struct stack_info {
>  	enum stack_type type;
>  };
>  
> +enum unwind_rc {
> +	UNWIND_CONTINUE,		/* No errors encountered */
> +	UNWIND_ABORT,			/* Fatal errors encountered */
> +	UNWIND_FINISH,			/* End of stack reached successfully */
> +};

Generally, there are a bunch of properties we might need to check for an
unwind step relating to reliabiltiy (e.g. as you add
UNWIND_CONTINUE_WITH_RISK in the next patch), and I'd prefer that we
check those properties on the struct stackframe, and simplify
unwind_frame() to return a bool.

Something akin to the x86 unwinders, where the main loop looks like:

for (unwind_start(&state, ...);
     !unwind_done(&state) && !unwind_error(&state);
     unwind_next_frame(&state) {
	...
}

That way we don't have to grow the enum to handle every variation that
we can think of, and it's simple enough for users to check the
properties with the helpers.

> +
>  /*
>   * A snapshot of a frame record or fp/lr register values, along with some
>   * accounting information necessary for robust unwinding.
> @@ -61,7 +67,8 @@ struct stackframe {
>  #endif
>  };
>  
> -extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> +extern enum unwind_rc unwind_frame(struct task_struct *tsk,
> +				   struct stackframe *frame);
>  extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
>  			    bool (*fn)(void *, unsigned long), void *data);
>  extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
> @@ -148,8 +155,8 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
>  	return false;
>  }
>  
> -static inline void start_backtrace(struct stackframe *frame,
> -				   unsigned long fp, unsigned long pc)
> +static inline enum unwind_rc start_backtrace(struct stackframe *frame,
> +					     unsigned long fp, unsigned long pc)
>  {
>  	frame->fp = fp;
>  	frame->pc = pc;
> @@ -169,6 +176,7 @@ static inline void start_backtrace(struct stackframe *frame,
>  	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
>  	frame->prev_fp = 0;
>  	frame->prev_type = STACK_TYPE_UNKNOWN;
> +	return UNWIND_CONTINUE;
>  }
>  
>  #endif	/* __ASM_STACKTRACE_H */
> diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
> index 88ff471b0bce..f459208149ae 100644
> --- a/arch/arm64/kernel/perf_callchain.c
> +++ b/arch/arm64/kernel/perf_callchain.c
> @@ -148,13 +148,16 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
>  			   struct pt_regs *regs)
>  {
>  	struct stackframe frame;
> +	enum unwind_rc rc;
>  
>  	if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
>  		/* We don't support guest os callchain now */
>  		return;
>  	}
>  
> -	start_backtrace(&frame, regs->regs[29], regs->pc);
> +	rc = start_backtrace(&frame, regs->regs[29], regs->pc);
> +	if (rc == UNWIND_FINISH || rc == UNWIND_ABORT)
> +		return;
>  	walk_stackframe(current, &frame, callchain_trace, entry);

As a first step, could we convert this over to arch_stack_walk()?

>  }
>  
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 6e60aa3b5ea9..e9c763b44fd4 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -573,6 +573,7 @@ unsigned long get_wchan(struct task_struct *p)
>  	struct stackframe frame;
>  	unsigned long stack_page, ret = 0;
>  	int count = 0;
> +	enum unwind_rc rc;
>  	if (!p || p == current || p->state == TASK_RUNNING)
>  		return 0;
>  
> @@ -580,10 +581,13 @@ unsigned long get_wchan(struct task_struct *p)
>  	if (!stack_page)
>  		return 0;
>  
> -	start_backtrace(&frame, thread_saved_fp(p), thread_saved_pc(p));
> +	rc = start_backtrace(&frame, thread_saved_fp(p), thread_saved_pc(p));
> +	if (rc == UNWIND_FINISH || rc == UNWIND_ABORT)
> +		return 0;
>  
>  	do {
> -		if (unwind_frame(p, &frame))
> +		rc = unwind_frame(p, &frame);
> +		if (rc == UNWIND_FINISH || rc == UNWIND_ABORT)
>  			goto out;
>  		if (!in_sched_functions(frame.pc)) {
>  			ret = frame.pc;

Likewise, can we convert this to use arch_stack_walk()?

> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
> index a6d18755652f..1224e043e98f 100644
> --- a/arch/arm64/kernel/return_address.c
> +++ b/arch/arm64/kernel/return_address.c
> @@ -36,13 +36,17 @@ void *return_address(unsigned int level)
>  {
>  	struct return_address_data data;
>  	struct stackframe frame;
> +	enum unwind_rc rc;
>  
>  	data.level = level + 2;
>  	data.addr = NULL;
>  
> -	start_backtrace(&frame,
> -			(unsigned long)__builtin_frame_address(0),
> -			(unsigned long)return_address);
> +	rc = start_backtrace(&frame,
> +			     (unsigned long)__builtin_frame_address(0),
> +			     (unsigned long)return_address);
> +	if (rc == UNWIND_FINISH || rc == UNWIND_ABORT)
> +		return NULL;
> +
>  	walk_stackframe(current, &frame, save_return_addr, &data);

Likewise, can we convert this to use arch_stack_walk()?

Thanks,
Mark.

>  
>  	if (!data.level)
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index d55bdfb7789c..e9c2c1fa9dde 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -39,26 +39,27 @@
>   * records (e.g. a cycle), determined based on the location and fp value of A
>   * and the location (but not the fp value) of B.
>   */
> -int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> +enum unwind_rc notrace unwind_frame(struct task_struct *tsk,
> +					struct stackframe *frame)
>  {
>  	unsigned long fp = frame->fp;
>  	struct stack_info info;
>  
>  	/* Terminal record; nothing to unwind */
>  	if (!fp)
> -		return -ENOENT;
> +		return UNWIND_FINISH;
>  
>  	if (fp & 0xf)
> -		return -EINVAL;
> +		return UNWIND_ABORT;
>  
>  	if (!tsk)
>  		tsk = current;
>  
>  	if (!on_accessible_stack(tsk, fp, &info))
> -		return -EINVAL;
> +		return UNWIND_ABORT;
>  
>  	if (test_bit(info.type, frame->stacks_done))
> -		return -EINVAL;
> +		return UNWIND_ABORT;
>  
>  	/*
>  	 * As stacks grow downward, any valid record on the same stack must be
> @@ -75,7 +76,7 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  	 */
>  	if (info.type == frame->prev_type) {
>  		if (fp <= frame->prev_fp)
> -			return -EINVAL;
> +			return UNWIND_ABORT;
>  	} else {
>  		set_bit(frame->prev_type, frame->stacks_done);
>  	}
> @@ -101,14 +102,14 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  		 */
>  		ret_stack = ftrace_graph_get_ret_stack(tsk, frame->graph++);
>  		if (WARN_ON_ONCE(!ret_stack))
> -			return -EINVAL;
> +			return UNWIND_ABORT;
>  		frame->pc = ret_stack->ret;
>  	}
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>  
>  	frame->pc = ptrauth_strip_insn_pac(frame->pc);
>  
> -	return 0;
> +	return UNWIND_CONTINUE;
>  }
>  NOKPROBE_SYMBOL(unwind_frame);
>  
> @@ -116,12 +117,12 @@ void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
>  			     bool (*fn)(void *, unsigned long), void *data)
>  {
>  	while (1) {
> -		int ret;
> +		enum unwind_rc rc;
>  
>  		if (!fn(data, frame->pc))
>  			break;
> -		ret = unwind_frame(tsk, frame);
> -		if (ret < 0)
> +		rc = unwind_frame(tsk, frame);
> +		if (rc == UNWIND_FINISH || rc == UNWIND_ABORT)
>  			break;
>  	}
>  }
> @@ -137,6 +138,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  {
>  	struct stackframe frame;
>  	int skip = 0;
> +	enum unwind_rc rc;
>  
>  	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>  
> @@ -153,17 +155,19 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  		return;
>  
>  	if (tsk == current) {
> -		start_backtrace(&frame,
> -				(unsigned long)__builtin_frame_address(0),
> -				(unsigned long)dump_backtrace);
> +		rc = start_backtrace(&frame,
> +				     (unsigned long)__builtin_frame_address(0),
> +				     (unsigned long)dump_backtrace);
>  	} else {
>  		/*
>  		 * task blocked in __switch_to
>  		 */
> -		start_backtrace(&frame,
> -				thread_saved_fp(tsk),
> -				thread_saved_pc(tsk));
> +		rc = start_backtrace(&frame,
> +				     thread_saved_fp(tsk),
> +				     thread_saved_pc(tsk));
>  	}
> +	if (rc == UNWIND_FINISH || rc == UNWIND_ABORT)
> +		return;
>  
>  	printk("%sCall trace:\n", loglvl);
>  	do {
> @@ -181,7 +185,8 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  			 */
>  			dump_backtrace_entry(regs->pc, loglvl);
>  		}
> -	} while (!unwind_frame(tsk, &frame));
> +		rc = unwind_frame(tsk, &frame);
> +	} while (rc != UNWIND_FINISH && rc != UNWIND_ABORT);
>  
>  	put_task_stack(tsk);
>  }
> @@ -199,17 +204,19 @@ noinline void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  			      struct pt_regs *regs)
>  {
>  	struct stackframe frame;
> +	enum unwind_rc rc;
>  
>  	if (regs)
> -		start_backtrace(&frame, regs->regs[29], regs->pc);
> +		rc = start_backtrace(&frame, regs->regs[29], regs->pc);
>  	else if (task == current)
> -		start_backtrace(&frame,
> +		rc = start_backtrace(&frame,
>  				(unsigned long)__builtin_frame_address(1),
>  				(unsigned long)__builtin_return_address(0));
>  	else
> -		start_backtrace(&frame, thread_saved_fp(task),
> -				thread_saved_pc(task));
> -
> +		rc = start_backtrace(&frame, thread_saved_fp(task),
> +				     thread_saved_pc(task));
> +	if (rc == UNWIND_FINISH || rc == UNWIND_ABORT)
> +		return;
>  	walk_stackframe(task, &frame, consume_entry, cookie);
>  }
>  
> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
> index eebbc8d7123e..eb50218ec9a4 100644
> --- a/arch/arm64/kernel/time.c
> +++ b/arch/arm64/kernel/time.c
> @@ -35,15 +35,18 @@
>  unsigned long profile_pc(struct pt_regs *regs)
>  {
>  	struct stackframe frame;
> +	enum unwind_rc rc;
>  
>  	if (!in_lock_functions(regs->pc))
>  		return regs->pc;
>  
> -	start_backtrace(&frame, regs->regs[29], regs->pc);
> +	rc = start_backtrace(&frame, regs->regs[29], regs->pc);
> +	if (rc == UNWIND_FINISH || rc == UNWIND_ABORT)
> +		return 0;
>  
>  	do {
> -		int ret = unwind_frame(NULL, &frame);
> -		if (ret < 0)
> +		rc = unwind_frame(NULL, &frame);
> +		if (rc == UNWIND_FINISH || rc == UNWIND_ABORT)
>  			return 0;
>  	} while (in_lock_functions(frame.pc));
>  
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-07-28 16:59 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <3f2aab69a35c243c5e97f47c4ad84046355f5b90>
2021-06-30 22:33 ` [RFC PATCH v6 0/3] arm64: Implement stack trace reliability checks madvenka
2021-06-30 22:33   ` [RFC PATCH v6 1/3] arm64: Improve the unwinder return value madvenka
2021-07-28 16:56     ` Mark Rutland [this message]
2021-07-29 13:54       ` Madhavan T. Venkataraman
2021-06-30 22:33   ` [RFC PATCH v6 2/3] arm64: Introduce stack trace reliability checks in the unwinder madvenka
2021-06-30 22:33   ` [RFC PATCH v6 3/3] arm64: Create a list of SYM_CODE functions, check return PC against list madvenka
2021-07-28 17:25     ` Mark Rutland
2021-07-29 14:06       ` Madhavan T. Venkataraman
2021-07-29 14:52         ` Mark Brown
2021-07-29 17:07           ` Madhavan T. Venkataraman
2021-07-29 15:48         ` Mark Rutland
2021-07-29 16:27           ` Mark Brown
2021-07-29 17:09           ` Madhavan T. Venkataraman
2021-07-26 13:49   ` [RFC PATCH v6 0/3] arm64: Implement stack trace reliability checks Madhavan T. Venkataraman
2021-08-12 13:24 ` [RFC PATCH v7 0/4] arm64: Reorganize the unwinder and implement " madvenka
2021-08-12 13:24   ` [RFC PATCH v7 1/4] arm64: Make all stack walking functions use arch_stack_walk() madvenka
2021-08-12 15:23     ` Mark Brown
2021-08-12 16:30       ` Madhavan T. Venkataraman
2021-08-12 13:24   ` [RFC PATCH v7 2/4] arm64: Reorganize the unwinder code for better consistency and maintenance madvenka
2021-08-12 13:24   ` [RFC PATCH v7 3/4] arm64: Introduce stack trace reliability checks in the unwinder madvenka
2021-08-12 13:24   ` [RFC PATCH v7 4/4] arm64: Create a list of SYM_CODE functions, check return PC against list madvenka
2021-08-12 18:31   ` [RFC PATCH v7 0/4] arm64: Reorganize the unwinder and implement stack trace reliability checks Madhavan T. Venkataraman
2021-08-12 18:45     ` Madhavan T. Venkataraman
2021-08-12 18:35 ` madvenka
2021-08-12 18:35   ` [RFC PATCH v7 1/4] arm64: Make all stack walking functions use arch_stack_walk() madvenka
2021-08-12 18:35   ` [RFC PATCH v7 2/4] arm64: Reorganize the unwinder code for better consistency and maintenance madvenka
2021-08-12 18:35   ` [RFC PATCH v7 3/4] arm64: Introduce stack trace reliability checks in the unwinder madvenka
2021-08-12 18:35   ` [RFC PATCH v7 4/4] arm64: Create a list of SYM_CODE functions, check return PC against list madvenka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210728165635.GA47345@C02TD0UTHF1T.local \
    --to=mark.rutland@arm.com \
    --cc=ardb@kernel.org \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=jmorris@namei.org \
    --cc=jpoimboe@redhat.com \
    --cc=jthierry@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=madvenka@linux.microsoft.com \
    --cc=nobuta.keiya@fujitsu.com \
    --cc=pasha.tatashin@soleen.com \
    --cc=sjitindarsingh@gmail.com \
    --cc=will@kernel.org \
    --subject='Re: [RFC PATCH v6 1/3] arm64: Improve the unwinder return value' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).