Re: [PATCH] usercopy: delete __noreturn from usercopy_abort

Re: [PATCH] usercopy: delete __noreturn from usercopy_abort

[Josh Poimboeuf]

> >> For the usercopy_abort function, whether '__noreturn' is added
> >> does not affect the internal behavior of the usercopy_abort function.
> >> Therefore, it is recommended that '__noreturn' be deleted
> >> so that backtrace can work properly.
> > 
> > This isn't acceptable. Removing __noreturn this will break
> > objtool's processing of execution flow for livepatching, IBT, and
> > KCFI instrumentation. These all depend on an accurate control flow
> > descriptions, and usercopy_abort is correctly marked __noreturn.

__noreturn also has the benefit of enabling the compiler to produce more
compact code for callees.

> Thank you for providing this information.
> I'll go back to further understand how __noreturn is used
> in features such as KCFI and livepatching.

Adding ARM folks -- see
https://lkml.kernel.org/lkml/1709516385-7778-1-git-send-email-xiaojiangfeng@huawei.com
for the original bug report.

This is an off-by-one bug which is common in unwinders, due to the fact
that the address on the stack points to the return address rather than
the call address.

So, for example, when the last instruction of a function is a function
call (e.g., to a noreturn function), it can cause the unwinder to
incorrectly try to unwind from the function *after* the callee.

For ORC (x86), we fixed this by decrementing the PC for call frames (but
not exception frames).  I've seen user space unwinders do similar, for
non-signal frames.

Something like the following might fix your issue (completely untested):

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 360f0d2406bf..4891e38cdc1f 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -21,9 +21,7 @@ struct stackframe {
 	struct llist_node *kr_cur;
 	struct task_struct *tsk;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
 	bool ex_frame;
-#endif
 };
 
 static __always_inline
@@ -37,9 +35,8 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
 		frame->kr_cur = NULL;
 		frame->tsk = current;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
-		frame->ex_frame = in_entry_text(frame->pc);
-#endif
+		frame->ex_frame = !!regs;
+
 }
 
 extern int unwind_frame(struct stackframe *frame);
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 620aa82e3bdd..caed7436da09 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -154,9 +154,6 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task
 	frame->kr_cur = NULL;
 	frame->tsk = task;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
-	frame->ex_frame = in_entry_text(frame->pc);
-#endif
 }
 
 void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
@@ -167,6 +164,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 	if (regs) {
 		start_stack_trace(&frame, NULL, regs->ARM_fp, regs->ARM_sp,
 				  regs->ARM_lr, regs->ARM_pc);
+		frame.ex_frame = true;
 	} else if (task != current) {
 #ifdef CONFIG_SMP
 		/*
@@ -180,6 +178,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 				  thread_saved_sp(task), 0,
 				  thread_saved_pc(task));
 #endif
+		frame.ex_frame = false;
 	} else {
 here:
 		start_stack_trace(&frame, task,
@@ -187,6 +186,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 				  current_stack_pointer,
 				  (unsigned long)__builtin_return_address(0),
 				  (unsigned long)&&here);
+		frame.ex_frame = false;
 		/* skip this function */
 		if (unwind_frame(&frame))
 			return;
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 3bad79db5d6e..46a5b1eb3f0a 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
 	printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n",
 		loglvl, where, from);
 #elif defined CONFIG_BACKTRACE_VERBOSE
-	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
+	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
 		loglvl, where, (void *)where, from, (void *)from);
 #else
-	printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from);
+	printk("%s %ps from %pB\n", loglvl, (void *)where, (void *)from);
 #endif
 
 	if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 9d2192156087..99ded32196af 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -407,7 +407,7 @@ int unwind_frame(struct stackframe *frame)
 {
 	const struct unwind_idx *idx;
 	struct unwind_ctrl_block ctrl;
-	unsigned long sp_low;
+	unsigned long sp_low, pc;
 
 	/* store the highest address on the stack to avoid crossing it*/
 	sp_low = frame->sp;
@@ -417,19 +417,22 @@ int unwind_frame(struct stackframe *frame)
 	pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__,
 		 frame->pc, frame->lr, frame->sp);
 
-	idx = unwind_find_idx(frame->pc);
+	pc = frame->ex_frame ? frame->pc : frame->pc - 4;
+
+	idx = unwind_find_idx(pc);
 	if (!idx) {
-		if (frame->pc && kernel_text_address(frame->pc)) {
-			if (in_module_plt(frame->pc) && frame->pc != frame->lr) {
+		if (kernel_text_address(pc)) {
+			if (in_module_plt(pc) && frame->pc != frame->lr) {
 				/*
 				 * Quoting Ard: Veneers only set PC using a
 				 * PC+immediate LDR, and so they don't affect
 				 * the state of the stack or the register file
 				 */
 				frame->pc = frame->lr;
+				frame->ex_frame = false;
 				return URC_OK;
 			}
-			pr_warn("unwind: Index not found %08lx\n", frame->pc);
+			pr_warn("unwind: Index not found %08lx\n", pc);
 		}
 		return -URC_FAILURE;
 	}
@@ -442,7 +445,7 @@ int unwind_frame(struct stackframe *frame)
 	if (idx->insn == 1)
 		/* can't unwind */
 		return -URC_FAILURE;
-	else if (frame->pc == prel31_to_addr(&idx->addr_offset)) {
+	else if (frame->ex_frame && pc == prel31_to_addr(&idx->addr_offset)) {
 		/*
 		 * Unwinding is tricky when we're halfway through the prologue,
 		 * since the stack frame that the unwinder expects may not be
@@ -451,9 +454,10 @@ int unwind_frame(struct stackframe *frame)
 		 * a function, we are still effectively in the stack frame of
 		 * the caller, and the unwind info has no relevance yet.
 		 */
-		if (frame->pc == frame->lr)
+		if (pc == frame->lr)
 			return -URC_FAILURE;
 		frame->pc = frame->lr;
+		frame->ex_frame = false;
 		return URC_OK;
 	} else if ((idx->insn & 0x80000000) == 0)
 		/* prel31 to the unwind table */
@@ -515,6 +519,7 @@ int unwind_frame(struct stackframe *frame)
 	frame->lr = ctrl.vrs[LR];
 	frame->pc = ctrl.vrs[PC];
 	frame->lr_addr = ctrl.lr_addr;
+	frame->ex_frame = false;
 
 	return URC_OK;
 }
@@ -544,6 +549,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 		 */
 here:
 		frame.pc = (unsigned long)&&here;
+		frame.ex_frame = false;
 	} else {
 		/* task blocked in __switch_to */
 		frame.fp = thread_saved_fp(tsk);
@@ -554,11 +560,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 		 */
 		frame.lr = 0;
 		frame.pc = thread_saved_pc(tsk);
+		frame.ex_frame = false;
 	}
 
 	while (1) {
 		int urc;
-		unsigned long where = frame.pc;
+		unsigned long where = frame.ex_frame ? frame.pc : frame.pc - 4;
 
 		urc = unwind_frame(&frame);
 		if (urc < 0)

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

Re: [PATCH] usercopy: delete __noreturn from usercopy_abort

[Jiangfeng Xiao]

On 2024/3/6 1:58, Josh Poimboeuf wrote:
>>>> For the usercopy_abort function, whether '__noreturn' is added
>>>> does not affect the internal behavior of the usercopy_abort function.
>>>> Therefore, it is recommended that '__noreturn' be deleted
>>>> so that backtrace can work properly.
>>>
>>> This isn't acceptable. Removing __noreturn this will break
>>> objtool's processing of execution flow for livepatching, IBT, and
>>> KCFI instrumentation. These all depend on an accurate control flow
>>> descriptions, and usercopy_abort is correctly marked __noreturn.
> 
> __noreturn also has the benefit of enabling the compiler to produce more
> compact code for callees.
> 
>> Thank you for providing this information.
>> I'll go back to further understand how __noreturn is used
>> in features such as KCFI and livepatching.
> 
> Adding ARM folks -- see
> https://lkml.kernel.org/lkml/1709516385-7778-1-git-send-email-xiaojiangfeng@huawei.com
> for the original bug report.
> 
> This is an off-by-one bug which is common in unwinders, due to the fact
> that the address on the stack points to the return address rather than
> the call address.
> 

Thanks for your advice. I think I understand. To solve this problem, I need to
fix the off-by-one bug which is common in unwinders. I'll try to fix it later
by referring to your patch.

> So, for example, when the last instruction of a function is a function
> call (e.g., to a noreturn function), it can cause the unwinder to
> incorrectly try to unwind from the function *after* the callee.
> 
> For ORC (x86), we fixed this by decrementing the PC for call frames (but
> not exception frames).  I've seen user space unwinders do similar, for
> non-signal frames.
> 
> Something like the following might fix your issue (completely untested):
> 
> diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
> index 360f0d2406bf..4891e38cdc1f 100644
> --- a/arch/arm/include/asm/stacktrace.h
> +++ b/arch/arm/include/asm/stacktrace.h
> @@ -21,9 +21,7 @@ struct stackframe {
>  	struct llist_node *kr_cur;
>  	struct task_struct *tsk;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>  	bool ex_frame;
> -#endif
>  };
>  
>  static __always_inline
> @@ -37,9 +35,8 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>  		frame->kr_cur = NULL;
>  		frame->tsk = current;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
> -		frame->ex_frame = in_entry_text(frame->pc);
> -#endif
> +		frame->ex_frame = !!regs;
> +
>  }
>  
>  extern int unwind_frame(struct stackframe *frame);
> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> index 620aa82e3bdd..caed7436da09 100644
> --- a/arch/arm/kernel/stacktrace.c
> +++ b/arch/arm/kernel/stacktrace.c
> @@ -154,9 +154,6 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task
>  	frame->kr_cur = NULL;
>  	frame->tsk = task;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
> -	frame->ex_frame = in_entry_text(frame->pc);
> -#endif
>  }
>  
>  void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> @@ -167,6 +164,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
>  	if (regs) {
>  		start_stack_trace(&frame, NULL, regs->ARM_fp, regs->ARM_sp,
>  				  regs->ARM_lr, regs->ARM_pc);
> +		frame.ex_frame = true;
>  	} else if (task != current) {
>  #ifdef CONFIG_SMP
>  		/*
> @@ -180,6 +178,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
>  				  thread_saved_sp(task), 0,
>  				  thread_saved_pc(task));
>  #endif
> +		frame.ex_frame = false;
>  	} else {
>  here:
>  		start_stack_trace(&frame, task,
> @@ -187,6 +186,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
>  				  current_stack_pointer,
>  				  (unsigned long)__builtin_return_address(0),
>  				  (unsigned long)&&here);
> +		frame.ex_frame = false;
>  		/* skip this function */
>  		if (unwind_frame(&frame))
>  			return;
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 3bad79db5d6e..46a5b1eb3f0a 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
>  	printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n",
>  		loglvl, where, from);
>  #elif defined CONFIG_BACKTRACE_VERBOSE
> -	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
> +	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
>  		loglvl, where, (void *)where, from, (void *)from);
>  #else
> -	printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from);
> +	printk("%s %ps from %pB\n", loglvl, (void *)where, (void *)from);
>  #endif
>  
>  	if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index 9d2192156087..99ded32196af 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -407,7 +407,7 @@ int unwind_frame(struct stackframe *frame)
>  {
>  	const struct unwind_idx *idx;
>  	struct unwind_ctrl_block ctrl;
> -	unsigned long sp_low;
> +	unsigned long sp_low, pc;
>  
>  	/* store the highest address on the stack to avoid crossing it*/
>  	sp_low = frame->sp;
> @@ -417,19 +417,22 @@ int unwind_frame(struct stackframe *frame)
>  	pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__,
>  		 frame->pc, frame->lr, frame->sp);
>  
> -	idx = unwind_find_idx(frame->pc);
> +	pc = frame->ex_frame ? frame->pc : frame->pc - 4;
> +
> +	idx = unwind_find_idx(pc);
>  	if (!idx) {
> -		if (frame->pc && kernel_text_address(frame->pc)) {
> -			if (in_module_plt(frame->pc) && frame->pc != frame->lr) {
> +		if (kernel_text_address(pc)) {
> +			if (in_module_plt(pc) && frame->pc != frame->lr) {
>  				/*
>  				 * Quoting Ard: Veneers only set PC using a
>  				 * PC+immediate LDR, and so they don't affect
>  				 * the state of the stack or the register file
>  				 */
>  				frame->pc = frame->lr;
> +				frame->ex_frame = false;
>  				return URC_OK;
>  			}
> -			pr_warn("unwind: Index not found %08lx\n", frame->pc);
> +			pr_warn("unwind: Index not found %08lx\n", pc);
>  		}
>  		return -URC_FAILURE;
>  	}
> @@ -442,7 +445,7 @@ int unwind_frame(struct stackframe *frame)
>  	if (idx->insn == 1)
>  		/* can't unwind */
>  		return -URC_FAILURE;
> -	else if (frame->pc == prel31_to_addr(&idx->addr_offset)) {
> +	else if (frame->ex_frame && pc == prel31_to_addr(&idx->addr_offset)) {
>  		/*
>  		 * Unwinding is tricky when we're halfway through the prologue,
>  		 * since the stack frame that the unwinder expects may not be
> @@ -451,9 +454,10 @@ int unwind_frame(struct stackframe *frame)
>  		 * a function, we are still effectively in the stack frame of
>  		 * the caller, and the unwind info has no relevance yet.
>  		 */
> -		if (frame->pc == frame->lr)
> +		if (pc == frame->lr)
>  			return -URC_FAILURE;
>  		frame->pc = frame->lr;
> +		frame->ex_frame = false;
>  		return URC_OK;
>  	} else if ((idx->insn & 0x80000000) == 0)
>  		/* prel31 to the unwind table */
> @@ -515,6 +519,7 @@ int unwind_frame(struct stackframe *frame)
>  	frame->lr = ctrl.vrs[LR];
>  	frame->pc = ctrl.vrs[PC];
>  	frame->lr_addr = ctrl.lr_addr;
> +	frame->ex_frame = false;
>  
>  	return URC_OK;
>  }
> @@ -544,6 +549,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  		 */
>  here:
>  		frame.pc = (unsigned long)&&here;
> +		frame.ex_frame = false;
>  	} else {
>  		/* task blocked in __switch_to */
>  		frame.fp = thread_saved_fp(tsk);
> @@ -554,11 +560,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  		 */
>  		frame.lr = 0;
>  		frame.pc = thread_saved_pc(tsk);
> +		frame.ex_frame = false;
>  	}
>  
>  	while (1) {
>  		int urc;
> -		unsigned long where = frame.pc;
> +		unsigned long where = frame.ex_frame ? frame.pc : frame.pc - 4;
>  
>  		urc = unwind_frame(&frame);
>  		if (urc < 0)
> .
> 

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

Re: [PATCH] usercopy: delete __noreturn from usercopy_abort

[Russell King (Oracle)]

On Tue, Mar 05, 2024 at 09:58:46AM -0800, Josh Poimboeuf wrote:
> This is an off-by-one bug which is common in unwinders, due to the fact
> that the address on the stack points to the return address rather than
> the call address.
> 
> So, for example, when the last instruction of a function is a function
> call (e.g., to a noreturn function), it can cause the unwinder to
> incorrectly try to unwind from the function *after* the callee.

I suppose this can only happen in __noreturn functions because that
can be:

foo:
...
	bl	bar
... end of function and thus next function ...

which results in LR pointing into the next function.

Would it make better sense to lookup the LR value winding it back by
one instruction like ORC on x86 does (as you mention) rather than
the patch you proposed which looks rather large and complicated?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

Re: [PATCH] usercopy: delete __noreturn from usercopy_abort

[Josh Poimboeuf]

On Wed, Mar 06, 2024 at 09:52:01AM +0000, Russell King (Oracle) wrote:
> On Tue, Mar 05, 2024 at 09:58:46AM -0800, Josh Poimboeuf wrote:
> > This is an off-by-one bug which is common in unwinders, due to the fact
> > that the address on the stack points to the return address rather than
> > the call address.
> > 
> > So, for example, when the last instruction of a function is a function
> > call (e.g., to a noreturn function), it can cause the unwinder to
> > incorrectly try to unwind from the function *after* the callee.
> 
> I suppose this can only happen in __noreturn functions because that
> can be:
> 
> foo:
> ...
> 	bl	bar
> ... end of function and thus next function ...
> 
> which results in LR pointing into the next function.
> 
> Would it make better sense to lookup the LR value winding it back by
> one instruction like ORC on x86 does (as you mention) rather than
> the patch you proposed which looks rather large and complicated?

That patch *is* an attempt to make it match ORC's behavior.  What
specifically looks complicated about it?

-- 
Josh

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

RE: [PATCH] usercopy: delete __noreturn from usercopy_abort

[David Laight]

From: Russell King
> Sent: 06 March 2024 09:52
> 
> On Tue, Mar 05, 2024 at 09:58:46AM -0800, Josh Poimboeuf wrote:
> > This is an off-by-one bug which is common in unwinders, due to the fact
> > that the address on the stack points to the return address rather than
> > the call address.
> >
> > So, for example, when the last instruction of a function is a function
> > call (e.g., to a noreturn function), it can cause the unwinder to
> > incorrectly try to unwind from the function *after* the callee.
> 
> I suppose this can only happen in __noreturn functions because that
> can be:
> 
> foo:
> ..
> 	bl	bar
> .. end of function and thus next function ...
> 
> which results in LR pointing into the next function.
> 
> Would it make better sense to lookup the LR value winding it back by
> one instruction like ORC on x86 does (as you mention) rather than
> the patch you proposed which looks rather large and complicated?

Is it even possible to always reliably get a stack trace from
a no-return function on a cpu that uses a 'lr'?

If the function doesn't return then the compiler need not save
'lr' on stack and can still use it as a temporary register.
Without a valid 'lr' I think all you can do is search the stack
for a likely code address?

Am I missing something?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


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

Re: [PATCH] usercopy: delete __noreturn from usercopy_abort

[Jiangfeng Xiao]

On 2024/3/6 1:58, Josh Poimboeuf wrote:

> Adding ARM folks -- see
> https://lkml.kernel.org/lkml/1709516385-7778-1-git-send-email-xiaojiangfeng@huawei.com
> for the original bug report.
> 
> This is an off-by-one bug which is common in unwinders, due to the fact
> that the address on the stack points to the return address rather than
> the call address.
> 
> So, for example, when the last instruction of a function is a function
> call (e.g., to a noreturn function), it can cause the unwinder to
> incorrectly try to unwind from the function *after* the callee.
> 
> For ORC (x86), we fixed this by decrementing the PC for call frames (but
> not exception frames).  I've seen user space unwinders do similar, for
> non-signal frames.
> 
> Something like the following might fix your issue (completely untested):
> 

Thank you very much. I have verified that your patch can fix my issue.
But I have some little questions.

> diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
> index 360f0d2406bf..4891e38cdc1f 100644
> --- a/arch/arm/include/asm/stacktrace.h
> +++ b/arch/arm/include/asm/stacktrace.h
> @@ -21,9 +21,7 @@ struct stackframe {
>  	struct llist_node *kr_cur;
>  	struct task_struct *tsk;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>  	bool ex_frame;
> -#endif
>  };
>  
>  static __always_inline
> @@ -37,9 +35,8 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>  		frame->kr_cur = NULL;
>  		frame->tsk = current;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
> -		frame->ex_frame = in_entry_text(frame->pc);
> -#endif
> +		frame->ex_frame = !!regs;
> +

'regs' must not be NULL, frame->ex_frame will always be TRUE.
So I think we just need to remove CONFIG_UNWINDER_FRAME_POINTER here.
We don't need to change the frame->ex_frame assignment statement.


> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index 9d2192156087..99ded32196af 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -407,7 +407,7 @@ int unwind_frame(struct stackframe *frame)
>  {
>  	const struct unwind_idx *idx;
>  	struct unwind_ctrl_block ctrl;
> -	unsigned long sp_low;
> +	unsigned long sp_low, pc;
>  
>  	/* store the highest address on the stack to avoid crossing it*/
>  	sp_low = frame->sp;
> @@ -417,19 +417,22 @@ int unwind_frame(struct stackframe *frame)
>  	pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__,
>  		 frame->pc, frame->lr, frame->sp);
>  
> -	idx = unwind_find_idx(frame->pc);
> +	pc = frame->ex_frame ? frame->pc : frame->pc - 4;

For details, see the unwind_next_frame function in the unwind_orc.c.
Why subtract 4 here instead of 1?
`pc = frame->ex_frame ? frame->pc : frame->pc - 1`
Is it more appropriate?

> +
> +	idx = unwind_find_idx(pc);
>  	if (!idx) {
> -		if (frame->pc && kernel_text_address(frame->pc)) {
> -			if (in_module_plt(frame->pc) && frame->pc != frame->lr) {
> +		if (kernel_text_address(pc)) {
> +			if (in_module_plt(pc) && frame->pc != frame->lr) {
>  				/*
>  				 * Quoting Ard: Veneers only set PC using a
>  				 * PC+immediate LDR, and so they don't affect
>  				 * the state of the stack or the register file
>  				 */
>  				frame->pc = frame->lr;
> +				frame->ex_frame = false;
>  				return URC_OK;
>  			}
> -			pr_warn("unwind: Index not found %08lx\n", frame->pc);
> +			pr_warn("unwind: Index not found %08lx\n", pc);
>  		}
>  		return -URC_FAILURE;
>  	}
> @@ -442,7 +445,7 @@ int unwind_frame(struct stackframe *frame)
>  	if (idx->insn == 1)
>  		/* can't unwind */
>  		return -URC_FAILURE;
> -	else if (frame->pc == prel31_to_addr(&idx->addr_offset)) {
> +	else if (frame->ex_frame && pc == prel31_to_addr(&idx->addr_offset)) {
>  		/*
>  		 * Unwinding is tricky when we're halfway through the prologue,
>  		 * since the stack frame that the unwinder expects may not be
> @@ -451,9 +454,10 @@ int unwind_frame(struct stackframe *frame)
>  		 * a function, we are still effectively in the stack frame of
>  		 * the caller, and the unwind info has no relevance yet.
>  		 */
> -		if (frame->pc == frame->lr)
> +		if (pc == frame->lr)
>  			return -URC_FAILURE;
>  		frame->pc = frame->lr;
> +		frame->ex_frame = false;
>  		return URC_OK;
>  	} else if ((idx->insn & 0x80000000) == 0)
>  		/* prel31 to the unwind table */
> @@ -515,6 +519,7 @@ int unwind_frame(struct stackframe *frame)
>  	frame->lr = ctrl.vrs[LR];
>  	frame->pc = ctrl.vrs[PC];
>  	frame->lr_addr = ctrl.lr_addr;
> +	frame->ex_frame = false;

Why is the value of `frame->ex_frame` directly set to false?
Why is the value not determined based on `frame->pc`?
That is, `frame->ex_frame = in_entry_text(frame->pc)`

>  
>  	return URC_OK;
>  }
> @@ -544,6 +549,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  		 */
>  here:
>  		frame.pc = (unsigned long)&&here;
> +		frame.ex_frame = false;
>  	} else {
>  		/* task blocked in __switch_to */
>  		frame.fp = thread_saved_fp(tsk);
> @@ -554,11 +560,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  		 */
>  		frame.lr = 0;
>  		frame.pc = thread_saved_pc(tsk);
> +		frame.ex_frame = false;
>  	}
>  
>  	while (1) {
>  		int urc;
> -		unsigned long where = frame.pc;
> +		unsigned long where = frame.ex_frame ? frame.pc : frame.pc - 4;
>  
>  		urc = unwind_frame(&frame);
>  		if (urc < 0)
> .
> 

If I refer to your demo patch and submit a new bugfix patch,
can I mark you as "Co-developed-by" in this new bugfix patch?

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

[PATCH] ARM: unwind: improve unwinders for noreturn case

[Jiangfeng Xiao]

This is an off-by-one bug which is common in unwinders,
due to the fact that the address on the stack points
to the return address rather than the call address.

So, for example, when the last instruction of a function
is a function call (e.g., to a noreturn function), it can
cause the unwinder to incorrectly try to unwind from
the function after the callee.

foo:
...
	bl	bar
... end of function and thus next function ...

which results in LR pointing into the next function.

Fixed this by subtracting 1 from frmae->pc in the call frame
(but not exception frames) like ORC on x86 does.

Refer to the unwind_next_frame function in the unwind_orc.c

Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com>
---
 arch/arm/include/asm/stacktrace.h |  4 ----
 arch/arm/kernel/stacktrace.c      |  2 --
 arch/arm/kernel/traps.c           |  4 ++--
 arch/arm/kernel/unwind.c          | 18 +++++++++++++++---
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 360f0d2..07e4c16 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -21,9 +21,7 @@ struct stackframe {
 	struct llist_node *kr_cur;
 	struct task_struct *tsk;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
 	bool ex_frame;
-#endif
 };
 
 static __always_inline
@@ -37,9 +35,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
 		frame->kr_cur = NULL;
 		frame->tsk = current;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
 		frame->ex_frame = in_entry_text(frame->pc);
-#endif
 }
 
 extern int unwind_frame(struct stackframe *frame);
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 620aa82..1abd4f9 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -154,9 +154,7 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task
 	frame->kr_cur = NULL;
 	frame->tsk = task;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
 	frame->ex_frame = in_entry_text(frame->pc);
-#endif
 }
 
 void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 3bad79d..b64e442 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
 	printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n",
 		loglvl, where, from);
 #elif defined CONFIG_BACKTRACE_VERBOSE
-	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
+	pr_warn("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
 		loglvl, where, (void *)where, from, (void *)from);
 #else
-	printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from);
+	pr_warn("%s %ps from %pB\n", loglvl, (void *)where, (void *)from);
 #endif
 
 	if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 9d21921..f2baf92 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 
+#include <asm/sections.h>
 #include <asm/stacktrace.h>
 #include <asm/traps.h>
 #include <asm/unwind.h>
@@ -416,8 +417,14 @@ int unwind_frame(struct stackframe *frame)
 
 	pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__,
 		 frame->pc, frame->lr, frame->sp);
-
-	idx = unwind_find_idx(frame->pc);
+	/*
+	 * For a call frame (as opposed to a exception frame), when the last
+	 * instruction of a function is a function call (e.g., to a noreturn
+	 * function), it can cause the unwinder incorrectly try to unwind
+	 * from the function after the callee, fixed this by subtracting 1
+	 * from frame->pc in the call frame like ORC on x86 does.
+	 */
+	idx = unwind_find_idx(frame->ex_frame ? frame->pc : frame->pc - 1);
 	if (!idx) {
 		if (frame->pc && kernel_text_address(frame->pc)) {
 			if (in_module_plt(frame->pc) && frame->pc != frame->lr) {
@@ -427,6 +434,7 @@ int unwind_frame(struct stackframe *frame)
 				 * the state of the stack or the register file
 				 */
 				frame->pc = frame->lr;
+				frame->ex_frame = in_entry_text(frame->pc);
 				return URC_OK;
 			}
 			pr_warn("unwind: Index not found %08lx\n", frame->pc);
@@ -454,6 +462,7 @@ int unwind_frame(struct stackframe *frame)
 		if (frame->pc == frame->lr)
 			return -URC_FAILURE;
 		frame->pc = frame->lr;
+		frame->ex_frame = in_entry_text(frame->pc);
 		return URC_OK;
 	} else if ((idx->insn & 0x80000000) == 0)
 		/* prel31 to the unwind table */
@@ -515,6 +524,7 @@ int unwind_frame(struct stackframe *frame)
 	frame->lr = ctrl.vrs[LR];
 	frame->pc = ctrl.vrs[PC];
 	frame->lr_addr = ctrl.lr_addr;
+	frame->ex_frame = in_entry_text(frame->pc);
 
 	return URC_OK;
 }
@@ -544,6 +554,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 		 */
 here:
 		frame.pc = (unsigned long)&&here;
+		frame.ex_frame = false;
 	} else {
 		/* task blocked in __switch_to */
 		frame.fp = thread_saved_fp(tsk);
@@ -554,11 +565,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 		 */
 		frame.lr = 0;
 		frame.pc = thread_saved_pc(tsk);
+		frame.ex_frame = false;
 	}
 
 	while (1) {
 		int urc;
-		unsigned long where = frame.pc;
+		unsigned long where = frame.ex_frame ? frame.pc : frame.pc - 1;
 
 		urc = unwind_frame(&frame);
 		if (urc < 0)
-- 
1.8.5.6


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

Re: [PATCH] ARM: unwind: improve unwinders for noreturn case

[Kees Cook]

On Wed, Mar 20, 2024 at 10:19:29AM +0800, Jiangfeng Xiao wrote:
> This is an off-by-one bug which is common in unwinders,
> due to the fact that the address on the stack points
> to the return address rather than the call address.
> 
> So, for example, when the last instruction of a function
> is a function call (e.g., to a noreturn function), it can
> cause the unwinder to incorrectly try to unwind from
> the function after the callee.
> 
> foo:
> ...
> 	bl	bar
> ... end of function and thus next function ...
> 
> which results in LR pointing into the next function.
> 
> Fixed this by subtracting 1 from frmae->pc in the call frame
> (but not exception frames) like ORC on x86 does.
> 
> Refer to the unwind_next_frame function in the unwind_orc.c
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
> Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
> Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com>
> ---
>  arch/arm/include/asm/stacktrace.h |  4 ----
>  arch/arm/kernel/stacktrace.c      |  2 --
>  arch/arm/kernel/traps.c           |  4 ++--
>  arch/arm/kernel/unwind.c          | 18 +++++++++++++++---
>  4 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
> index 360f0d2..07e4c16 100644
> --- a/arch/arm/include/asm/stacktrace.h
> +++ b/arch/arm/include/asm/stacktrace.h
> @@ -21,9 +21,7 @@ struct stackframe {
>  	struct llist_node *kr_cur;
>  	struct task_struct *tsk;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>  	bool ex_frame;
> -#endif
>  };
>  
>  static __always_inline
> @@ -37,9 +35,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>  		frame->kr_cur = NULL;
>  		frame->tsk = current;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>  		frame->ex_frame = in_entry_text(frame->pc);
> -#endif
>  }
>  
>  extern int unwind_frame(struct stackframe *frame);
> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> index 620aa82..1abd4f9 100644
> --- a/arch/arm/kernel/stacktrace.c
> +++ b/arch/arm/kernel/stacktrace.c
> @@ -154,9 +154,7 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task
>  	frame->kr_cur = NULL;
>  	frame->tsk = task;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>  	frame->ex_frame = in_entry_text(frame->pc);
> -#endif
>  }
>  
>  void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 3bad79d..b64e442 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
>  	printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n",
>  		loglvl, where, from);
>  #elif defined CONFIG_BACKTRACE_VERBOSE
> -	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
> +	pr_warn("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
>  		loglvl, where, (void *)where, from, (void *)from);

This should stay printk("%s...", loglvl, ...) or loglvl should be
dropped when converting to pr_warn():

	pr_warn([<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
		where, (void *)where, from, (void *)from);

Why did you want to force the "warn" log level?

>  #else
> -	printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from);
> +	pr_warn("%s %ps from %pB\n", loglvl, (void *)where, (void *)from);

Ditto.

-Kees

-- 
Kees Cook

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

Re: [PATCH] ARM: unwind: improve unwinders for noreturn case

[Jiangfeng Xiao]

On 2024/3/20 10:46, Kees Cook wrote:
> On Wed, Mar 20, 2024 at 10:19:29AM +0800, Jiangfeng Xiao wrote:
>> This is an off-by-one bug which is common in unwinders,
>> due to the fact that the address on the stack points
>> to the return address rather than the call address.
>>
>> So, for example, when the last instruction of a function
>> is a function call (e.g., to a noreturn function), it can
>> cause the unwinder to incorrectly try to unwind from
>> the function after the callee.
>>
>> foo:
>> ...
>> 	bl	bar
>> ... end of function and thus next function ...
>>
>> which results in LR pointing into the next function.
>>
>> Fixed this by subtracting 1 from frmae->pc in the call frame
>> (but not exception frames) like ORC on x86 does.
>>
>> Refer to the unwind_next_frame function in the unwind_orc.c
>>
>> Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
>> Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
>> Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com>
>> ---
>>  arch/arm/include/asm/stacktrace.h |  4 ----
>>  arch/arm/kernel/stacktrace.c      |  2 --
>>  arch/arm/kernel/traps.c           |  4 ++--
>>  arch/arm/kernel/unwind.c          | 18 +++++++++++++++---
>>  4 files changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
>> index 360f0d2..07e4c16 100644
>> --- a/arch/arm/include/asm/stacktrace.h
>> +++ b/arch/arm/include/asm/stacktrace.h
>> @@ -21,9 +21,7 @@ struct stackframe {
>>  	struct llist_node *kr_cur;
>>  	struct task_struct *tsk;
>>  #endif
>> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>>  	bool ex_frame;
>> -#endif
>>  };
>>  
>>  static __always_inline
>> @@ -37,9 +35,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>>  		frame->kr_cur = NULL;
>>  		frame->tsk = current;
>>  #endif
>> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>>  		frame->ex_frame = in_entry_text(frame->pc);
>> -#endif
>>  }
>>  
>>  extern int unwind_frame(struct stackframe *frame);
>> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
>> index 620aa82..1abd4f9 100644
>> --- a/arch/arm/kernel/stacktrace.c
>> +++ b/arch/arm/kernel/stacktrace.c
>> @@ -154,9 +154,7 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task
>>  	frame->kr_cur = NULL;
>>  	frame->tsk = task;
>>  #endif
>> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>>  	frame->ex_frame = in_entry_text(frame->pc);
>> -#endif
>>  }
>>  
>>  void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>> index 3bad79d..b64e442 100644
>> --- a/arch/arm/kernel/traps.c
>> +++ b/arch/arm/kernel/traps.c
>> @@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
>>  	printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n",
>>  		loglvl, where, from);
>>  #elif defined CONFIG_BACKTRACE_VERBOSE
>> -	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
>> +	pr_warn("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
>>  		loglvl, where, (void *)where, from, (void *)from);
> 
> This should stay printk("%s...", loglvl, ...) or loglvl should be
> dropped when converting to pr_warn():
> 
> 	pr_warn([<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
> 		where, (void *)where, from, (void *)from);
> 
> Why did you want to force the "warn" log level?
> 

Thank you for your review. I think I'm wrong.

The checkpatch.pl script reports the "WARNING: printk() should
include KERN_<LEVEL> facility level" warning.

That's why I changed printk to pr_warn.
I should change printk to printk(KERN_DEFAULT).

>>  #else
>> -	printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from);
>> +	pr_warn("%s %ps from %pB\n", loglvl, (void *)where, (void *)from);
> 
> Ditto.
> 
> -Kees
> 

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

Re: [PATCH] ARM: unwind: improve unwinders for noreturn case

[Matthew Wilcox]

On Wed, Mar 20, 2024 at 11:30:13AM +0800, Jiangfeng Xiao wrote:
> The checkpatch.pl script reports the "WARNING: printk() should
> include KERN_<LEVEL> facility level" warning.
> 
> That's why I changed printk to pr_warn.
> I should change printk to printk(KERN_DEFAULT).

No, you should ignore checkpatch.  For bonus points, figure out why you
should ignore it specifically in this case.

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

Re: [PATCH] ARM: unwind: improve unwinders for noreturn case

[Jiangfeng Xiao]

On 2024/3/20 11:34, Matthew Wilcox wrote:
> On Wed, Mar 20, 2024 at 11:30:13AM +0800, Jiangfeng Xiao wrote:
>> The checkpatch.pl script reports the "WARNING: printk() should
>> include KERN_<LEVEL> facility level" warning.
>>
>> That's why I changed printk to pr_warn.
>> I should change printk to printk(KERN_DEFAULT).
> 
> No, you should ignore checkpatch.  For bonus points, figure out why you
> should ignore it specifically in this case.
> .
> 

Thank you. I think I understand.

The checkpatch.pl file is a false report
because the 'loglvl' already exists.

I'd better keep printk(" %s...", loglvl, ...) still.

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

[PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Jiangfeng Xiao]

This is an off-by-one bug which is common in unwinders,
due to the fact that the address on the stack points
to the return address rather than the call address.

So, for example, when the last instruction of a function
is a function call (e.g., to a noreturn function), it can
cause the unwinder to incorrectly try to unwind from
the function after the callee.

foo:
...
	bl	bar
... end of function and thus next function ...

which results in LR pointing into the next function.

Fixed this by subtracting 1 from frmae->pc in the call frame
(but not exception frames) like ORC on x86 does.

Refer to the unwind_next_frame function in the unwind_orc.c

Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com>
---
ChangeLog v1->v2
- stay printk("%s...", loglvl, ...)
---
 arch/arm/include/asm/stacktrace.h |  4 ----
 arch/arm/kernel/stacktrace.c      |  2 --
 arch/arm/kernel/traps.c           |  4 ++--
 arch/arm/kernel/unwind.c          | 18 +++++++++++++++---
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 360f0d2..07e4c16 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -21,9 +21,7 @@ struct stackframe {
 	struct llist_node *kr_cur;
 	struct task_struct *tsk;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
 	bool ex_frame;
-#endif
 };
 
 static __always_inline
@@ -37,9 +35,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
 		frame->kr_cur = NULL;
 		frame->tsk = current;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
 		frame->ex_frame = in_entry_text(frame->pc);
-#endif
 }
 
 extern int unwind_frame(struct stackframe *frame);
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 620aa82..1abd4f9 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -154,9 +154,7 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task
 	frame->kr_cur = NULL;
 	frame->tsk = task;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
 	frame->ex_frame = in_entry_text(frame->pc);
-#endif
 }
 
 void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 3bad79d..46a5b1e 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
 	printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n",
 		loglvl, where, from);
 #elif defined CONFIG_BACKTRACE_VERBOSE
-	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
+	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
 		loglvl, where, (void *)where, from, (void *)from);
 #else
-	printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from);
+	printk("%s %ps from %pB\n", loglvl, (void *)where, (void *)from);
 #endif
 
 	if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 9d21921..f2baf92 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 
+#include <asm/sections.h>
 #include <asm/stacktrace.h>
 #include <asm/traps.h>
 #include <asm/unwind.h>
@@ -416,8 +417,14 @@ int unwind_frame(struct stackframe *frame)
 
 	pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__,
 		 frame->pc, frame->lr, frame->sp);
-
-	idx = unwind_find_idx(frame->pc);
+	/*
+	 * For a call frame (as opposed to a exception frame), when the last
+	 * instruction of a function is a function call (e.g., to a noreturn
+	 * function), it can cause the unwinder incorrectly try to unwind
+	 * from the function after the callee, fixed this by subtracting 1
+	 * from frame->pc in the call frame like ORC on x86 does.
+	 */
+	idx = unwind_find_idx(frame->ex_frame ? frame->pc : frame->pc - 1);
 	if (!idx) {
 		if (frame->pc && kernel_text_address(frame->pc)) {
 			if (in_module_plt(frame->pc) && frame->pc != frame->lr) {
@@ -427,6 +434,7 @@ int unwind_frame(struct stackframe *frame)
 				 * the state of the stack or the register file
 				 */
 				frame->pc = frame->lr;
+				frame->ex_frame = in_entry_text(frame->pc);
 				return URC_OK;
 			}
 			pr_warn("unwind: Index not found %08lx\n", frame->pc);
@@ -454,6 +462,7 @@ int unwind_frame(struct stackframe *frame)
 		if (frame->pc == frame->lr)
 			return -URC_FAILURE;
 		frame->pc = frame->lr;
+		frame->ex_frame = in_entry_text(frame->pc);
 		return URC_OK;
 	} else if ((idx->insn & 0x80000000) == 0)
 		/* prel31 to the unwind table */
@@ -515,6 +524,7 @@ int unwind_frame(struct stackframe *frame)
 	frame->lr = ctrl.vrs[LR];
 	frame->pc = ctrl.vrs[PC];
 	frame->lr_addr = ctrl.lr_addr;
+	frame->ex_frame = in_entry_text(frame->pc);
 
 	return URC_OK;
 }
@@ -544,6 +554,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 		 */
 here:
 		frame.pc = (unsigned long)&&here;
+		frame.ex_frame = false;
 	} else {
 		/* task blocked in __switch_to */
 		frame.fp = thread_saved_fp(tsk);
@@ -554,11 +565,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 		 */
 		frame.lr = 0;
 		frame.pc = thread_saved_pc(tsk);
+		frame.ex_frame = false;
 	}
 
 	while (1) {
 		int urc;
-		unsigned long where = frame.pc;
+		unsigned long where = frame.ex_frame ? frame.pc : frame.pc - 1;
 
 		urc = unwind_frame(&frame);
 		if (urc < 0)
-- 
1.8.5.6


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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Wed, Mar 20, 2024 at 11:44:38AM +0800, Jiangfeng Xiao wrote:
> This is an off-by-one bug which is common in unwinders,
> due to the fact that the address on the stack points
> to the return address rather than the call address.
> 
> So, for example, when the last instruction of a function
> is a function call (e.g., to a noreturn function), it can
> cause the unwinder to incorrectly try to unwind from
> the function after the callee.
> 
> foo:
> ...
> 	bl	bar
> ... end of function and thus next function ...
> 
> which results in LR pointing into the next function.
> 
> Fixed this by subtracting 1 from frmae->pc in the call frame
> (but not exception frames) like ORC on x86 does.

The reason that I'm not accepting this patch is because the above says
that it fixes it by subtracting 1 from the PC value, but the patch is
*way* more complicated than that and there's no explanation why.

For example, the following are unexplained:

- Why do we always need ex_frame
- What is the purpose of the change in format string for the display of
  backtraces

> 
> Refer to the unwind_next_frame function in the unwind_orc.c
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
> Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
> Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com>
> ---
> ChangeLog v1->v2
> - stay printk("%s...", loglvl, ...)
> ---
>  arch/arm/include/asm/stacktrace.h |  4 ----
>  arch/arm/kernel/stacktrace.c      |  2 --
>  arch/arm/kernel/traps.c           |  4 ++--
>  arch/arm/kernel/unwind.c          | 18 +++++++++++++++---
>  4 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
> index 360f0d2..07e4c16 100644
> --- a/arch/arm/include/asm/stacktrace.h
> +++ b/arch/arm/include/asm/stacktrace.h
> @@ -21,9 +21,7 @@ struct stackframe {
>  	struct llist_node *kr_cur;
>  	struct task_struct *tsk;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>  	bool ex_frame;
> -#endif
>  };
>  
>  static __always_inline
> @@ -37,9 +35,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>  		frame->kr_cur = NULL;
>  		frame->tsk = current;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>  		frame->ex_frame = in_entry_text(frame->pc);
> -#endif
>  }
>  
>  extern int unwind_frame(struct stackframe *frame);
> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> index 620aa82..1abd4f9 100644
> --- a/arch/arm/kernel/stacktrace.c
> +++ b/arch/arm/kernel/stacktrace.c
> @@ -154,9 +154,7 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task
>  	frame->kr_cur = NULL;
>  	frame->tsk = task;
>  #endif
> -#ifdef CONFIG_UNWINDER_FRAME_POINTER
>  	frame->ex_frame = in_entry_text(frame->pc);
> -#endif
>  }
>  
>  void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 3bad79d..46a5b1e 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
>  	printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n",
>  		loglvl, where, from);
>  #elif defined CONFIG_BACKTRACE_VERBOSE
> -	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
> +	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
>  		loglvl, where, (void *)where, from, (void *)from);
>  #else
> -	printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from);
> +	printk("%s %ps from %pB\n", loglvl, (void *)where, (void *)from);
>  #endif
>  
>  	if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index 9d21921..f2baf92 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -30,6 +30,7 @@
>  #include <linux/list.h>
>  #include <linux/module.h>
>  
> +#include <asm/sections.h>
>  #include <asm/stacktrace.h>
>  #include <asm/traps.h>
>  #include <asm/unwind.h>
> @@ -416,8 +417,14 @@ int unwind_frame(struct stackframe *frame)
>  
>  	pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__,
>  		 frame->pc, frame->lr, frame->sp);
> -
> -	idx = unwind_find_idx(frame->pc);
> +	/*
> +	 * For a call frame (as opposed to a exception frame), when the last
> +	 * instruction of a function is a function call (e.g., to a noreturn
> +	 * function), it can cause the unwinder incorrectly try to unwind
> +	 * from the function after the callee, fixed this by subtracting 1
> +	 * from frame->pc in the call frame like ORC on x86 does.
> +	 */
> +	idx = unwind_find_idx(frame->ex_frame ? frame->pc : frame->pc - 1);
>  	if (!idx) {
>  		if (frame->pc && kernel_text_address(frame->pc)) {
>  			if (in_module_plt(frame->pc) && frame->pc != frame->lr) {
> @@ -427,6 +434,7 @@ int unwind_frame(struct stackframe *frame)
>  				 * the state of the stack or the register file
>  				 */
>  				frame->pc = frame->lr;
> +				frame->ex_frame = in_entry_text(frame->pc);
>  				return URC_OK;
>  			}
>  			pr_warn("unwind: Index not found %08lx\n", frame->pc);
> @@ -454,6 +462,7 @@ int unwind_frame(struct stackframe *frame)
>  		if (frame->pc == frame->lr)
>  			return -URC_FAILURE;
>  		frame->pc = frame->lr;
> +		frame->ex_frame = in_entry_text(frame->pc);
>  		return URC_OK;
>  	} else if ((idx->insn & 0x80000000) == 0)
>  		/* prel31 to the unwind table */
> @@ -515,6 +524,7 @@ int unwind_frame(struct stackframe *frame)
>  	frame->lr = ctrl.vrs[LR];
>  	frame->pc = ctrl.vrs[PC];
>  	frame->lr_addr = ctrl.lr_addr;
> +	frame->ex_frame = in_entry_text(frame->pc);
>  
>  	return URC_OK;
>  }
> @@ -544,6 +554,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  		 */
>  here:
>  		frame.pc = (unsigned long)&&here;
> +		frame.ex_frame = false;
>  	} else {
>  		/* task blocked in __switch_to */
>  		frame.fp = thread_saved_fp(tsk);
> @@ -554,11 +565,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  		 */
>  		frame.lr = 0;
>  		frame.pc = thread_saved_pc(tsk);
> +		frame.ex_frame = false;
>  	}
>  
>  	while (1) {
>  		int urc;
> -		unsigned long where = frame.pc;
> +		unsigned long where = frame.ex_frame ? frame.pc : frame.pc - 1;
>  
>  		urc = unwind_frame(&frame);
>  		if (urc < 0)
> -- 
> 1.8.5.6
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Jiangfeng Xiao]

On 2024/3/20 16:45, Russell King (Oracle) wrote:
> On Wed, Mar 20, 2024 at 11:44:38AM +0800, Jiangfeng Xiao wrote:
>> This is an off-by-one bug which is common in unwinders,
>> due to the fact that the address on the stack points
>> to the return address rather than the call address.
>>
>> So, for example, when the last instruction of a function
>> is a function call (e.g., to a noreturn function), it can
>> cause the unwinder to incorrectly try to unwind from
>> the function after the callee.
>>
>> foo:
>> ...
>> 	bl	bar
>> ... end of function and thus next function ...
>>
>> which results in LR pointing into the next function.
>>
>> Fixed this by subtracting 1 from frmae->pc in the call frame
>> (but not exception frames) like ORC on x86 does.
> 
> The reason that I'm not accepting this patch is because the above says
> that it fixes it by subtracting 1 from the PC value, but the patch is
> *way* more complicated than that and there's no explanation why.
> 
> For example, the following are unexplained:
> 
> - Why do we always need ex_frame

```
bar:
...
... end of function bar ...

foo:
    BUG();
... end of function foo ...
```

For example, when the first instruction of function 'foo'
is a undefined instruction, after function 'foo' is executed
to trigger an exception, 'arm_get_current_stackframe' assigns
'regs->ARM_pc' to 'frame.pc'.

If we always decrement frame.pc by 1, unwinder will incorrectly
try to unwind from the function 'bar' before the function 'foo'.

So we need to 'ext_frame' to distinguish this case
where we don't need to subtract 1.


> - What is the purpose of the change in format string for the display of
>   backtraces
```
unwind_frame(&frame);
dump_backtrace_entry(...from) //from = frame.pc
	printk("...%pS\n", ...(void *)from);
```
%pB will do sprint_backtrace and print the symbol at (from - 1) address
%pS will do sprint_symbol_build_id and print the symbol at (from) address

In unwind_frame, although we use 'frame->pc - 1' to execute unwind_find_idx,
but frame->pc itself does not change, in the noreturn case, frame->pc still
pointing into the next function. So this is going to be replaced by %pB.

> 
>>
>> Refer to the unwind_next_frame function in the unwind_orc.c
>>
>> Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
>> Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
>> Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com>
>> ---
>> ChangeLog v1->v2
>> - stay printk("%s...", loglvl, ...)


Thank you for your suggestion.
I'll change the code to be more concise in my [patch v3].


>> -- 
>> 1.8.5.6
>>
>>
> 

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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Wed, Mar 20, 2024 at 11:30:05PM +0800, Jiangfeng Xiao wrote:
> 
> 
> On 2024/3/20 16:45, Russell King (Oracle) wrote:
> > On Wed, Mar 20, 2024 at 11:44:38AM +0800, Jiangfeng Xiao wrote:
> >> This is an off-by-one bug which is common in unwinders,
> >> due to the fact that the address on the stack points
> >> to the return address rather than the call address.
> >>
> >> So, for example, when the last instruction of a function
> >> is a function call (e.g., to a noreturn function), it can
> >> cause the unwinder to incorrectly try to unwind from
> >> the function after the callee.
> >>
> >> foo:
> >> ...
> >> 	bl	bar
> >> ... end of function and thus next function ...
> >>
> >> which results in LR pointing into the next function.
> >>
> >> Fixed this by subtracting 1 from frmae->pc in the call frame
> >> (but not exception frames) like ORC on x86 does.
> > 
> > The reason that I'm not accepting this patch is because the above says
> > that it fixes it by subtracting 1 from the PC value, but the patch is
> > *way* more complicated than that and there's no explanation why.
> > 
> > For example, the following are unexplained:
> > 
> > - Why do we always need ex_frame
> 
> ```
> bar:
> ...
> ... end of function bar ...
> 
> foo:
>     BUG();
> ... end of function foo ...
> ```
> 
> For example, when the first instruction of function 'foo'
> is a undefined instruction, after function 'foo' is executed
> to trigger an exception, 'arm_get_current_stackframe' assigns
> 'regs->ARM_pc' to 'frame.pc'.
> 
> If we always decrement frame.pc by 1, unwinder will incorrectly
> try to unwind from the function 'bar' before the function 'foo'.
> 
> So we need to 'ext_frame' to distinguish this case
> where we don't need to subtract 1.

This just sounds wrong to me. We have two different cases:

1) we're unwinding a frame where PC points at the offending instruction.
   This may or may not be in the exception text.
2) we're unwinding a frame that has been created because of a branch,
   where the PC points at the next instruction _after_ that callsite.

While we're unwinding, we will mostly hit the second type of frames, but
we'll only hit the first type on the initial frame. Some exception
entries will have the PC pointing at the next instruction to be
executed, others will have it pointing at the offending instruction
(e.g. if it needs to be retried.)

So, I don't see what being in the exception/entry text really has much
to do with any decision making here. I think you've found that it works
for your case, but it won't _always_ work and you're just shifting the
"bug" with how these traces work from one issue to a different but
similar issue.

> > - What is the purpose of the change in format string for the display of
> >   backtraces
> ```
> unwind_frame(&frame);
> dump_backtrace_entry(...from) //from = frame.pc
> 	printk("...%pS\n", ...(void *)from);
> ```
> %pB will do sprint_backtrace and print the symbol at (from - 1) address
> %pS will do sprint_symbol_build_id and print the symbol at (from) address

The quote in printk-formats.rst states:

        %pS     versatile_init+0x0/0x110
	%pB     prev_fn_of_versatile_init+0x88/0x88

This is rather ambiguous on its own, since the definition of "previous
function" is ambiguous. Given the offset and size stated there, it's
also not obvious what pointer was passed. It would be nice if these
examples actually said what the pointer passed in actually was.

I had been interpreting "prev_fn_of_" to mean the caller of
versatile_init() but it could also be the preceeding function in the
kernel text to versatile_init() - that is where the ambiguity comes
from.

So, the question I now have is... if %pB prints the symbol corresponding
with "from - 1", then

- with the frame pointer walker, from will always be the return address
  found on the stack for the function we are currently in.
- with the unwinder it will be whatever the unwinder computes as the LR
  register unless the unwind instructions place a non-zero value in PC.

Is there a case where the unwinder gets this wrong?

I think what would help is if you split this patch up, and addressed
each part separately, describing the issue that each part is addressing
giving an example that clearly explains what the patch is doing.
However, please note my comments above that using the fact that we're
in an exception frame doesn't actually tell you anything about whether
you need to correct the PC value or not.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Jiangfeng Xiao]

On 2024/3/21 3:40, Russell King (Oracle) wrote:
> On Wed, Mar 20, 2024 at 11:30:05PM +0800, Jiangfeng Xiao wrote:
>>
>>
>> On 2024/3/20 16:45, Russell King (Oracle) wrote:
>>> On Wed, Mar 20, 2024 at 11:44:38AM +0800, Jiangfeng Xiao wrote:
>>>> This is an off-by-one bug which is common in unwinders,
>>>> due to the fact that the address on the stack points
>>>> to the return address rather than the call address.
>>>>
>>>> So, for example, when the last instruction of a function
>>>> is a function call (e.g., to a noreturn function), it can
>>>> cause the unwinder to incorrectly try to unwind from
>>>> the function after the callee.
>>>>
>>>> foo:
>>>> ...
>>>> 	bl	bar
>>>> ... end of function and thus next function ...
>>>>
>>>> which results in LR pointing into the next function.
>>>>
>>>> Fixed this by subtracting 1 from frmae->pc in the call frame
>>>> (but not exception frames) like ORC on x86 does.
>>>
>>> The reason that I'm not accepting this patch is because the above says
>>> that it fixes it by subtracting 1 from the PC value, but the patch is
>>> *way* more complicated than that and there's no explanation why.
>>>
>>> For example, the following are unexplained:
>>>
>>> - Why do we always need ex_frame
>>
>> ```
>> bar:
>> ...
>> ... end of function bar ...
>>
>> foo:
>>     BUG();
>> ... end of function foo ...
>> ```
>>
>> For example, when the first instruction of function 'foo'
>> is a undefined instruction, after function 'foo' is executed
>> to trigger an exception, 'arm_get_current_stackframe' assigns
>> 'regs->ARM_pc' to 'frame.pc'.
>>
>> If we always decrement frame.pc by 1, unwinder will incorrectly
>> try to unwind from the function 'bar' before the function 'foo'.
>>
>> So we need to 'ext_frame' to distinguish this case
>> where we don't need to subtract 1.
> 
> This just sounds wrong to me. We have two different cases:
> 
> 1) we're unwinding a frame where PC points at the offending instruction.
>    This may or may not be in the exception text.
> 2) we're unwinding a frame that has been created because of a branch,
>    where the PC points at the next instruction _after_ that callsite.
> 
> While we're unwinding, we will mostly hit the second type of frames, but
> we'll only hit the first type on the initial frame. Some exception
> entries will have the PC pointing at the next instruction to be
> executed, others will have it pointing at the offending instruction
> (e.g. if it needs to be retried.)


Thank you for your summary.

Now we try to enumerate all cases:

1) When we hit the first type of frames

1.1) If the pc pointing at the next instruction
     of the offending instruction

1.1.1) If the offending instruction is the first instruction
       of the function
       pc:   cause to unwind from current function
       pc-1: casue to unwind from current function

1.1.2) If the offending instruction is neither the first
       instruction nor the last instruction of the function
       pc:   cause to unwind from current function
       pc-1: casue to unwind from current function

1.1.3) If the offending instruction is the last instruction
       of the function
       pc:   cause to unwind from next    function
       pc-1: casue to unwind from current function

1.2) If the pc pointing at the offending instruction

1.2.1) If the offending instruction is the first instruction
       of the function
       pc:   cause to unwind from current  function
       pc-1: casue to unwind from previous function

1.2.2) If the offending instruction is neither the first
       instruction nor the last instruction of the function
       pc:   cause to unwind from current function
       pc-1: casue to unwind from current function

1.2.3) If the offending instruction is the last instruction
       of the function
       pc:   cause to unwind from current function
       pc-1: casue to unwind from current function

2) When we hit the second type of frames
2.1) pc always pointing at the next instruction after that callsite
2.1.1) If the callsite is the first instruction
       pc:   cause to unwind from current function
       pc-1: casue to unwind from current function

2.1.2) If the callsite is neither the first nor last instruction
       pc:   cause to unwind from current function
       pc-1: casue to unwind from current function

2.1.3) If the callsite is the last instruction
       pc:   cause to unwind from next    function
       pc-1: casue to unwind from current function


All in all, I think you are right.

In case 2), We can always unwind by 'pc-1'.

In case 1), If we unwind by 'pc', case 1.1.3) is problematic.
If we unwind by 'pc-1', 1.2.1) is problematic.


> 
> So, I don't see what being in the exception/entry text really has much
> to do with any decision making here. I think you've found that it works
> for your case, but it won't _always_ work and you're just shifting the
> "bug" with how these traces work from one issue to a different but
> similar issue.


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

RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[David Laight]

How aggressively does the compiler optimise 'noreturn' functions?
Consider:
void f(...)
{
	...
	if () {
		...
		noreturn(...);
	}
}

Without the noreturn() call it is a leaf function.
So the compiler doesn't need to save 'lr' on stack
(or the save could be deferred to inside the conditional).
Since noreturn() doesn't return it can be jumped to.
Additionally 'lr' can be used as a scratch register prior to
the noreturn() call.

So it is likely that inside noreturn() (and anything it
might call) you don't have a valid 'lr' chain at all.
No amount of picking between 'pc' and 'pc-1' is going to fix that.
The only way you can find a return address is by searching the
stack and hoping to find something that works.

So you need the compiler to 'not believe' the 'noreturn' attribute.
Setup a normal call frame and put a faulting instruction after the
call in case it returns.
That would give you half a chance of generating a backtrace.

Without that I suspect you are playing whack-a-mole.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Thu, Mar 21, 2024 at 10:22:30AM +0000, David Laight wrote:
> How aggressively does the compiler optimise 'noreturn' functions?

I've seen cases where the compiler emits a BL instruction as the very
last thing in the function, and nothing after it.

This is where the problem lies - because the link register value
created by the BL instruction will point to the instruction after the
BL which will _not_ part of the function that invoked the BL. That
will probably cause issues for the ELF unwinder, which means this
issue probably goes beyond _just_ printing the function name.

I have vague memories that Ard has been involved in the unwinder,
maybe he could comment on this problem? Maybe we need the unwinder
itself to do the correction? I also wonder whether we should only
do the correction if we detect that we're pointing at the first
instruction of a function, and the previous instruction in the
text segment was a BL.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[David Laight]

From: Russell King
> Sent: 21 March 2024 11:24
> 
> On Thu, Mar 21, 2024 at 10:22:30AM +0000, David Laight wrote:
> > How aggressively does the compiler optimise 'noreturn' functions?
> 
> I've seen cases where the compiler emits a BL instruction as the very
> last thing in the function, and nothing after it.

I've also seen the compiler defer generating a stack frame until
after an initial conditional.
That might mean you can get the BL in the middle of a function
but where the following instruction is for the 'no stack frame'
side of the branch.
That is very likely to break any stack offset calculations. 

> This is where the problem lies - because the link register value
> created by the BL instruction will point to the instruction after the
> BL which will _not_ part of the function that invoked the BL. That
> will probably cause issues for the ELF unwinder, which means this
> issue probably goes beyond _just_ printing the function name.

Isn't this already in the unwinder?
A BL itself isn't going to fault with PC = next-instruction.

For pretty much all code isn't *(LR-4) going to be BL?
On arm that is probably testable.
(It is pretty much impossible to detect a ACLL on x86.)
If it is a direct BL then you'd normally expect to the be
a call the function containing the current 'PC'.
The obvious exception is if there was a tail call, and printing
the called address would then be useful.
(It might help with leaf functions that don't generate a stack frame.)

I remember issues with the solaris sparc backtrace that used to
get confused by leaf functions...

> I have vague memories that Ard has been involved in the unwinder,
> maybe he could comment on this problem? Maybe we need the unwinder
> itself to do the correction? I also wonder whether we should only
> do the correction if we detect that we're pointing at the first
> instruction of a function, and the previous instruction in the
> text segment was a BL.

It might be enough to depend on whether the address is that of a
fault (where the instruction could be retried) or from a call/trap
instruction where it will be the following instruction.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Thu, Mar 21, 2024 at 12:07:51PM +0000, David Laight wrote:
> From: Russell King
> > Sent: 21 March 2024 11:24
> > 
> > On Thu, Mar 21, 2024 at 10:22:30AM +0000, David Laight wrote:
> > > How aggressively does the compiler optimise 'noreturn' functions?
> > 
> > I've seen cases where the compiler emits a BL instruction as the very
> > last thing in the function, and nothing after it.
> 
> I've also seen the compiler defer generating a stack frame until
> after an initial conditional.

.. which is why we pass -mno-sched-prolog to GCC.

> That might mean you can get the BL in the middle of a function
> but where the following instruction is for the 'no stack frame'
> side of the branch.
> That is very likely to break any stack offset calculations. 

No it can't. At any one point in the function, the stack has to be in
a well defined state, so that access to local variables can work, and
also the stack can be correctly unwound. If there exists a point in
the function body which can be reached where the stack could be in two
different states, then the stack can't be restored to the parent
context.

> > This is where the problem lies - because the link register value
> > created by the BL instruction will point to the instruction after the
> > BL which will _not_ part of the function that invoked the BL. That
> > will probably cause issues for the ELF unwinder, which means this
> > issue probably goes beyond _just_ printing the function name.
> 
> Isn't this already in the unwinder?
> A BL itself isn't going to fault with PC = next-instruction.

You are missing the fact that the PC can be the saved LR, and thus
can very well be the next instruction.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[David Laight]

From: Russell King
> Sent: 21 March 2024 12:23
...
> > That might mean you can get the BL in the middle of a function
> > but where the following instruction is for the 'no stack frame'
> > side of the branch.
> > That is very likely to break any stack offset calculations.
> 
> No it can't. At any one point in the function, the stack has to be in
> a well defined state, so that access to local variables can work, and
> also the stack can be correctly unwound. If there exists a point in
> the function body which can be reached where the stack could be in two
> different states, then the stack can't be restored to the parent
> context.

Actually you can get there with a function that has a lot of args.
So you can have:
	if (...) {
		push x
		bl func
		add %sp, #8
	}
	code;
which is fine.
But if 'func' is 'noreturn' then the 'add %sp, #8' can be discarded
and then the saved LR is that of 'code' - but the stack offset is wrong.

> > > This is where the problem lies - because the link register value
> > > created by the BL instruction will point to the instruction after the
> > > BL which will _not_ part of the function that invoked the BL. That
> > > will probably cause issues for the ELF unwinder, which means this
> > > issue probably goes beyond _just_ printing the function name.
> >
> > Isn't this already in the unwinder?
> > A BL itself isn't going to fault with PC = next-instruction.
> 
> You are missing the fact that the PC can be the saved LR, and thus
> can very well be the next instruction.

A PC from LR will always be the next instruction.
It is only the PC from a fault frame that is the current one.
The unwinder probably need to be told which one it has.
(Or add 4 the fault frame PC so that the unwinder can subtract
4 from it.)

At least (I don't think) there are any functions where the
called code is responsible for removing arguments.
That is a whole different bag of worms.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Thu, Mar 21, 2024 at 12:57:07PM +0000, David Laight wrote:
> From: Russell King
> > Sent: 21 March 2024 12:23
> ...
> > > That might mean you can get the BL in the middle of a function
> > > but where the following instruction is for the 'no stack frame'
> > > side of the branch.
> > > That is very likely to break any stack offset calculations.
> > 
> > No it can't. At any one point in the function, the stack has to be in
> > a well defined state, so that access to local variables can work, and
> > also the stack can be correctly unwound. If there exists a point in
> > the function body which can be reached where the stack could be in two
> > different states, then the stack can't be restored to the parent
> > context.
> 
> Actually you can get there with a function that has a lot of args.
> So you can have:
> 	if (...) {
> 		push x
> 		bl func
> 		add %sp, #8
> 	}
> 	code;
> which is fine.

No you can't.... and that isn't even Arm code. Arm doesn't use %sp.
Moreover, that "bl" will stomp over the link register, meaning this
function can not return.

> But if 'func' is 'noreturn' then the 'add %sp, #8' can be discarded
> and then the saved LR is that of 'code' - but the stack offset is wrong.

If func is noreturn, then the remainder of that path isn't expected
to be executed, so anything that happens after the "bl" is irrelevant.

> A PC from LR will always be the next instruction.
> It is only the PC from a fault frame that is the current one.

That sentence makes no sense to me, as I don't think it's even proper
English, so I can't parse it.

> The unwinder probably need to be told which one it has.
> (Or add 4 the fault frame PC so that the unwinder can subtract
> 4 from it.)

That's basically what I said.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[David Laight]

From: Russell King
> Sent: 21 March 2024 13:08
> 
> On Thu, Mar 21, 2024 at 12:57:07PM +0000, David Laight wrote:
> > From: Russell King
> > > Sent: 21 March 2024 12:23
> > ...
> > > > That might mean you can get the BL in the middle of a function
> > > > but where the following instruction is for the 'no stack frame'
> > > > side of the branch.
> > > > That is very likely to break any stack offset calculations.
> > >
> > > No it can't. At any one point in the function, the stack has to be in
> > > a well defined state, so that access to local variables can work, and
> > > also the stack can be correctly unwound. If there exists a point in
> > > the function body which can be reached where the stack could be in two
> > > different states, then the stack can't be restored to the parent
> > > context.
> >
> > Actually you can get there with a function that has a lot of args.
> > So you can have:
> > 	if (...) {
> > 		push x
> > 		bl func
> > 		add %sp, #8
> > 	}
> > 	code;
> > which is fine.
> 
> No you can't.... and that isn't even Arm code. Arm doesn't use %sp.
> Moreover, that "bl" will stomp over the link register, meaning this
> function can not return.

With 9+ arguments they spill to see https://godbolt.org/z/Yj3ovd8bY

Where the compiler generates:
f9:
        cmp     w0, 0
        ble     .L2
        sub     sp, sp, #32
        mov     w7, w0
        mov     w6, w0
        mov     w5, w0
        mov     w4, w0
        mov     w3, w0
        stp     x29, x30, [sp, 16]
        add     x29, sp, 16
        mov     w2, w0
        mov     w1, w0
        str     w0, [sp]
        bl      f
.L2:
        ret


A traceback from inside f() definitely needs to use LR-4
for the stack offset.

(arm64 doesn't seem to support -mno-sched-prolog).

I've failed to get different sized stack frames for the true/false
sides of the branch.
The compiler seems to pre-allocate the space for extra args rather
than using 'push' type instructions.
This was certainly better for some x86 cpu (p-pro?) but has now
gone out of fashion.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Thu, Mar 21, 2024 at 02:37:28PM +0000, David Laight wrote:
> From: Russell King
> > Sent: 21 March 2024 13:08
> > 
> > On Thu, Mar 21, 2024 at 12:57:07PM +0000, David Laight wrote:
> > > From: Russell King
> > > > Sent: 21 March 2024 12:23
> > > ...
> > > > > That might mean you can get the BL in the middle of a function
> > > > > but where the following instruction is for the 'no stack frame'
> > > > > side of the branch.
> > > > > That is very likely to break any stack offset calculations.
> > > >
> > > > No it can't. At any one point in the function, the stack has to be in
> > > > a well defined state, so that access to local variables can work, and
> > > > also the stack can be correctly unwound. If there exists a point in
> > > > the function body which can be reached where the stack could be in two
> > > > different states, then the stack can't be restored to the parent
> > > > context.
> > >
> > > Actually you can get there with a function that has a lot of args.
> > > So you can have:
> > > 	if (...) {
> > > 		push x
> > > 		bl func
> > > 		add %sp, #8
> > > 	}
> > > 	code;
> > > which is fine.
> > 
> > No you can't.... and that isn't even Arm code. Arm doesn't use %sp.
> > Moreover, that "bl" will stomp over the link register, meaning this
> > function can not return.
> 
> With 9+ arguments they spill to see https://godbolt.org/z/Yj3ovd8bY
> 
> Where the compiler generates:
> f9:
>         cmp     w0, 0
>         ble     .L2
>         sub     sp, sp, #32
>         mov     w7, w0
>         mov     w6, w0
>         mov     w5, w0
>         mov     w4, w0
>         mov     w3, w0
>         stp     x29, x30, [sp, 16]
>         add     x29, sp, 16
>         mov     w2, w0
>         mov     w1, w0
>         str     w0, [sp]
>         bl      f
> .L2:
>         ret

Don't show me Arm64 assembly when we're discussing Arm32.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[David Laight]

From: Russell King
> Sent: 21 March 2024 14:56
> 
> On Thu, Mar 21, 2024 at 02:37:28PM +0000, David Laight wrote:
> > From: Russell King
> > > Sent: 21 March 2024 13:08
> > >
> > > On Thu, Mar 21, 2024 at 12:57:07PM +0000, David Laight wrote:
> > > > From: Russell King
> > > > > Sent: 21 March 2024 12:23
> > > > ...
> > > > > > That might mean you can get the BL in the middle of a function
> > > > > > but where the following instruction is for the 'no stack frame'
> > > > > > side of the branch.
> > > > > > That is very likely to break any stack offset calculations.
> > > > >
> > > > > No it can't. At any one point in the function, the stack has to be in
> > > > > a well defined state, so that access to local variables can work, and
> > > > > also the stack can be correctly unwound. If there exists a point in
> > > > > the function body which can be reached where the stack could be in two
> > > > > different states, then the stack can't be restored to the parent
> > > > > context.
> > > >
> > > > Actually you can get there with a function that has a lot of args.
> > > > So you can have:
> > > > 	if (...) {
> > > > 		push x
> > > > 		bl func
> > > > 		add %sp, #8
> > > > 	}
> > > > 	code;
> > > > which is fine.
> > >
> > > No you can't.... and that isn't even Arm code. Arm doesn't use %sp.
> > > Moreover, that "bl" will stomp over the link register, meaning this
> > > function can not return.
> >
...
> 
> Don't show me Arm64 assembly when we're discussing Arm32.

Oops - I'd assumed no one did 32bit :-)
In any case it is much the same, see https://godbolt.org/z/7dcbKrs76

f4:
        push    {r3, lr}
        subs    r3, r0, #0
        ble     .L2
        mov     r2, r3
        mov     r1, r3
        bl      f
.L2:
        pop     {r3, pc}

f5:
        subs    r3, r0, #0
        ble     .L6
        push    {lr}
        sub     sp, sp, #12
        mov     r2, r3
        mov     r1, r3
        str     r3, [sp]
        bl      f
.L6:
        bx      lr

That is with -mno-sched-prolog but with 5+ args they spill to stack
and the %sp change is pulled into the conditional.

It does look like %lr is being saved (and for arm64 I think).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Thu, Mar 21, 2024 at 03:20:57PM +0000, David Laight wrote:
> From: Russell King
> > Sent: 21 March 2024 14:56
> > 
> > On Thu, Mar 21, 2024 at 02:37:28PM +0000, David Laight wrote:
> > > From: Russell King
> > > > Sent: 21 March 2024 13:08
> > > >
> > > > On Thu, Mar 21, 2024 at 12:57:07PM +0000, David Laight wrote:
> > > > > From: Russell King
> > > > > > Sent: 21 March 2024 12:23
> > > > > ...
> > > > > > > That might mean you can get the BL in the middle of a function
> > > > > > > but where the following instruction is for the 'no stack frame'
> > > > > > > side of the branch.
> > > > > > > That is very likely to break any stack offset calculations.
> > > > > >
> > > > > > No it can't. At any one point in the function, the stack has to be in
> > > > > > a well defined state, so that access to local variables can work, and
> > > > > > also the stack can be correctly unwound. If there exists a point in
> > > > > > the function body which can be reached where the stack could be in two
> > > > > > different states, then the stack can't be restored to the parent
> > > > > > context.
> > > > >
> > > > > Actually you can get there with a function that has a lot of args.
> > > > > So you can have:
> > > > > 	if (...) {
> > > > > 		push x
> > > > > 		bl func
> > > > > 		add %sp, #8
> > > > > 	}
> > > > > 	code;
> > > > > which is fine.
> > > >
> > > > No you can't.... and that isn't even Arm code. Arm doesn't use %sp.
> > > > Moreover, that "bl" will stomp over the link register, meaning this
> > > > function can not return.
> > >
> ...
> > 
> > Don't show me Arm64 assembly when we're discussing Arm32.
> 
> Oops - I'd assumed no one did 32bit :-)
> In any case it is much the same, see https://godbolt.org/z/7dcbKrs76
> 
> f4:
>         push    {r3, lr}
>         subs    r3, r0, #0
>         ble     .L2
>         mov     r2, r3
>         mov     r1, r3
>         bl      f
> .L2:
>         pop     {r3, pc}
> 
> f5:
>         subs    r3, r0, #0
>         ble     .L6
>         push    {lr}
>         sub     sp, sp, #12
>         mov     r2, r3
>         mov     r1, r3
>         str     r3, [sp]
>         bl      f
> .L6:
>         bx      lr
> 
> That is with -mno-sched-prolog but with 5+ args they spill to stack
> and the %sp change is pulled into the conditional.
> 
> It does look like %lr is being saved (and for arm64 I think).

I see nothing that contradicts anything I've said in your example
output.

You have been previously refering to a "bl" in the prologue, which is
what I thought you were going to give an example of. There is no "bl"
in the prologue of f5, the "ble" instruction is a normal branch for
less-than-or-equal. It's b + le not bl + e.

At .L6, there will be a difference in stack, but as f() is declared
as no-return, anything that comes after it is utterly irrelevant as
control is not expected to reach any following instruction via that
path. If it _were_ to, then in the example you give above, because
"lr" points at the bx lr instruction, the result would be to endlessly
spin executing bx lr instructions.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Ard Biesheuvel]

On Thu, 21 Mar 2024 at 12:24, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Mar 21, 2024 at 10:22:30AM +0000, David Laight wrote:
> > How aggressively does the compiler optimise 'noreturn' functions?
>
> I've seen cases where the compiler emits a BL instruction as the very
> last thing in the function, and nothing after it.
>
> This is where the problem lies - because the link register value
> created by the BL instruction will point to the instruction after the
> BL which will _not_ part of the function that invoked the BL. That
> will probably cause issues for the ELF unwinder, which means this
> issue probably goes beyond _just_ printing the function name.
>
> I have vague memories that Ard has been involved in the unwinder,
> maybe he could comment on this problem? Maybe we need the unwinder
> itself to do the correction? I also wonder whether we should only
> do the correction if we detect that we're pointing at the first
> instruction of a function, and the previous instruction in the
> text segment was a BL.
>

First of all, I consider this to be a defect in the toolchain. Given
that the compiler knows that the BL is not going to return, it should
simply emit a BRK instruction or similar after it. This would catch
inadvertent returns as well as eliminate this kind of confusion.

The ARM unwind format is not really suitable for asynchronous
unwinding, so unwinding across exceptions is always going to be hit
and miss. Also, we should consider what the unwind information
actually tells us: for debugging, it is useful to understand where we
came from, i.e., how we ended up in the situation where the backtrace
was triggered, but what it actually tells us is where we would have
gone next had execution not been interrupted. The latter notion is
important for things like reliable stacktrace and live patch, which
need to know where a task might be returning to.

Returning to the first instruction of a function is unusual, but in
the light of the above, I don't think we can assume that it means that
the call originated from the preceding function, even when it ends in
a BL instruction (although it would be highly likely, of course). The
tracers and other instrumentation might insert probes in the return
path, and I suspect that this may result in a similar situation in
some cases.

Given that this particular issue would just disappear if the compiler
would just insert a BRK after the BL, I'd prefer to explore first
whether we can get this fixed on the compiler side.

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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Thu, Mar 21, 2024 at 11:43:41PM +0100, Ard Biesheuvel wrote:
> Given that this particular issue would just disappear if the compiler
> would just insert a BRK after the BL, I'd prefer to explore first
> whether we can get this fixed on the compiler side.

Arm32 doesn't have a BRK instruction. What would be appropriate after
the no-return BL would be OS specific.

Maybe a branch to "abort" would be a good idea though.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[David Laight]

From: Russell King
> Sent: 22 March 2024 00:09
> 
> On Thu, Mar 21, 2024 at 11:43:41PM +0100, Ard Biesheuvel wrote:
> > Given that this particular issue would just disappear if the compiler
> > would just insert a BRK after the BL, I'd prefer to explore first
> > whether we can get this fixed on the compiler side.
> 
> Arm32 doesn't have a BRK instruction. What would be appropriate after
> the no-return BL would be OS specific.

It would need to depend on what was being compiled.
For the kernel it could be much the same as BUG().
(Probably without any extra data.)
I suspect that arm32 could use 'swi' in kernel space,
but you wouldn't want to use that in userspace.

Looks like armv5 has a bkpt instruction - could that be used?
Or does the kernel need to support armv4?

The last arm I wrote anything for was a strongarm.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Fri, Mar 22, 2024 at 09:24:20AM +0000, David Laight wrote:
> From: Russell King
> > Sent: 22 March 2024 00:09
> > 
> > On Thu, Mar 21, 2024 at 11:43:41PM +0100, Ard Biesheuvel wrote:
> > > Given that this particular issue would just disappear if the compiler
> > > would just insert a BRK after the BL, I'd prefer to explore first
> > > whether we can get this fixed on the compiler side.
> > 
> > Arm32 doesn't have a BRK instruction. What would be appropriate after
> > the no-return BL would be OS specific.
> 
> It would need to depend on what was being compiled.

Yes, but as for the rest...

> For the kernel it could be much the same as BUG().
> (Probably without any extra data.)
> I suspect that arm32 could use 'swi' in kernel space,
> but you wouldn't want to use that in userspace.
> 
> Looks like armv5 has a bkpt instruction - could that be used?
> Or does the kernel need to support armv4?
> 
> The last arm I wrote anything for was a strongarm.

Thank you David, but remember - I have programmed 32-bit Arm since 1992,
and wrote the majority of the 32-bit Arm kernel support. I think I know
what I'm walking about by now.

The compiler can't do the same as BUG() - that is a kernel specific
construct and not an architecture one. It is an undefined instruction
specifically chosen to be undefined on both 32-bit and 16-bit Arm ISAs.

As for your idea of using "swi" in kernel space, no that's never going
to happen - to shoe-horn that into the SWI exception path for the sake
of the compiler would be totally idiotic - it would cause userspace
performance regressions for something that never happens. Moreover,
with EABI the "comment" field in the "swi" instruction is ignored so
all SWIs under EABI are treated the same. So no, that's not going to
work without causing inefficiencies - again - for a case that will
likely never happen.

Whereas we already provide an abort() function because iirc the
compiler used to emit branches to that due to noreturn functions. If
correct, there's previous convention for doing this - and abort() is
still exists in the kernel and in userspace since it's part of ANSI
C. This would be a more reliable and portable solution, but probably
not for embedded platforms - and that's probably why it got removed.

There isn't going to be a single solution to this which satisfies
everyone, and I don't blame the compiler people for deciding to
basically give up with putting any instruction after a call to a
no-return function - because there isn't an instruction defined in
the architecture that _could_ be put there that would work everywhere.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[Jiangfeng Xiao]

On 2024/3/22 17:52, Russell King (Oracle) wrote:
> On Fri, Mar 22, 2024 at 09:24:20AM +0000, David Laight wrote:
>> From: Russell King
>>> Sent: 22 March 2024 00:09
>>>
>>> On Thu, Mar 21, 2024 at 11:43:41PM +0100, Ard Biesheuvel wrote:
>>>> Given that this particular issue would just disappear if the compiler
>>>> would just insert a BRK after the BL, I'd prefer to explore first
>>>> whether we can get this fixed on the compiler side.
>>>
>>> Arm32 doesn't have a BRK instruction. What would be appropriate after
>>> the no-return BL would be OS specific.
>>
>> It would need to depend on what was being compiled.
> 
> Yes, but as for the rest...
> 
>> For the kernel it could be much the same as BUG().
>> (Probably without any extra data.)
>> I suspect that arm32 could use 'swi' in kernel space,
>> but you wouldn't want to use that in userspace.
>>
>> Looks like armv5 has a bkpt instruction - could that be used?
>> Or does the kernel need to support armv4?
>>
>> The last arm I wrote anything for was a strongarm.
> 
> Thank you David, but remember - I have programmed 32-bit Arm since 1992,
> and wrote the majority of the 32-bit Arm kernel support. I think I know
> what I'm walking about by now.
> 
> The compiler can't do the same as BUG() - that is a kernel specific
> construct and not an architecture one. It is an undefined instruction
> specifically chosen to be undefined on both 32-bit and 16-bit Arm ISAs.
> 
> As for your idea of using "swi" in kernel space, no that's never going
> to happen - to shoe-horn that into the SWI exception path for the sake
> of the compiler would be totally idiotic - it would cause userspace
> performance regressions for something that never happens. Moreover,
> with EABI the "comment" field in the "swi" instruction is ignored so
> all SWIs under EABI are treated the same. So no, that's not going to
> work without causing inefficiencies - again - for a case that will
> likely never happen.
> 
> Whereas we already provide an abort() function because iirc the
> compiler used to emit branches to that due to noreturn functions. If
> correct, there's previous convention for doing this - and abort() is
> still exists in the kernel and in userspace since it's part of ANSI
> C. This would be a more reliable and portable solution, but probably
> not for embedded platforms - and that's probably why it got removed.
> 
> There isn't going to be a single solution to this which satisfies
> everyone, and I don't blame the compiler people for deciding to
> basically give up with putting any instruction after a call to a
> no-return function - because there isn't an instruction defined in
> the architecture that _could_ be put there that would work everywhere.
> 


If the compiler inserts (a branch to 'abort') behind (no-return BL)
that does not apply to ARM32 embedded platforms, do you think the
 "[PATCH v3] ARM: unwind: improve unwinders for noreturn case"
submitted the day before yesterday can be used as a
complementary solution?

2) we're unwinding a frame that has been created because of a branch,
   where the PC points at the next instruction _after_ that callsite.

When we hit the second type of frame, "pc-1" is closer to callsite,
and no problem is introduced. In addition, the issue of the 'noreturn'
can be solved.

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

RE: [PATCH v2] ARM: unwind: improve unwinders for noreturn case

[David Laight]

...
> Whereas we already provide an abort() function because iirc the
> compiler used to emit branches to that due to noreturn functions. If
> correct, there's previous convention for doing this - and abort() is
> still exists in the kernel and in userspace since it's part of ANSI
> C. This would be a more reliable and portable solution, but probably
> not for embedded platforms - and that's probably why it got removed.

Wouldn't you want it to do a 'bl abort' so that you could do a backtrace
to find out which 'noreturn' function had returned?

But that leaves you with another 'noreturn' function that you have
difficulty generating a backtrace from.

So you'd need a compiler option to specify what to put there.
I'd suspect linux would like something that generates an 'undefined
instruction' trap - since that would be expected to fault with the
saved PC pointing to the instruction itself (but architectures may vary).

'One size' definitely doesn't 'fit all' :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


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

[PATCH v3] ARM: unwind: improve unwinders for noreturn case

[Jiangfeng Xiao]

This is an off-by-one bug which is common in unwinders,
due to the fact that the address on the stack points
to the return address rather than the call address.

So, for example, when the last instruction of a function
is a function call (e.g., to a noreturn function), it can
cause the unwinder to incorrectly try to unwind from
the function after the callee.

foo:
...
    bl      bar
... end of function and thus next function ...

which results in LR pointing into the next function.

Fixed this by subtracting 1 from frmae->pc in the call frame
like ORC on x86 does.

Refer to the unwind_next_frame function in the unwind_orc.c

Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
Suggested-by: "Russell King (Oracle)" <linux@armlinux.org.uk>
Link: https://lkml.kernel.org/lkml/Zeg8wRYFemMjcCxG@shell.armlinux.org.uk/
Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com>
---
ChangeLog v1->v2
- stay printk("%s...", loglvl, ...)
ChangeLog v2->v3
- Redundant code is deleted to simplify the code
---
 arch/arm/kernel/unwind.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 9d21921..abfa8e9 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -514,6 +514,14 @@ int unwind_frame(struct stackframe *frame)
 	frame->sp = ctrl.vrs[SP];
 	frame->lr = ctrl.vrs[LR];
 	frame->pc = ctrl.vrs[PC];
+	/*
+	 * When the last instruction of a function is a function call
+	 * (e.g., to a noreturn function), it can cause the unwinder
+	 * incorrectly try to unwind from the function after the callee,
+	 * fixed this by subtracting 1 from frame->pc in the call frame
+	 * like ORC on x86 does.
+	 */
+	frame->pc = frame->pc - 1;
 	frame->lr_addr = ctrl.lr_addr;
 
 	return URC_OK;
-- 
1.8.5.6


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

Re: [PATCH v3] ARM: unwind: improve unwinders for noreturn case

[Russell King (Oracle)]

On Wed, Mar 20, 2024 at 11:41:34PM +0800, Jiangfeng Xiao wrote:
> This is an off-by-one bug which is common in unwinders,
> due to the fact that the address on the stack points
> to the return address rather than the call address.
> 
> So, for example, when the last instruction of a function
> is a function call (e.g., to a noreturn function), it can
> cause the unwinder to incorrectly try to unwind from
> the function after the callee.
> 
> foo:
> ...
>     bl      bar
> ... end of function and thus next function ...
> 
> which results in LR pointing into the next function.
> 
> Fixed this by subtracting 1 from frmae->pc in the call frame
> like ORC on x86 does.
> 
> Refer to the unwind_next_frame function in the unwind_orc.c

This came in while I was still replying to your previous reply, so
I'm going to ignore this. Please allow at least 24 hours between
postings, and please allow discussion to finish before posting a
new version - give your reviewers adequate time to compose a reply
bearing in mind that timezones might get in the way, but also making
supper (as is the case in this instance) may cause several hour delay
in reply.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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