When the last instruction of a noreturn function is a call
to another function, the return address falls outside
of the function boundary. This seems to cause kernel
to interrupt the backtrace.
My testcase is as follow:
```
static volatile size_t unconst = 0;
/*
check_object_size
__check_object_size
check_kernel_text_object
usercopy_abort("kernel text", ...)
*/
void test_usercopy_kernel(void)
{
check_object_size(schedule, unconst + PAGE_SIZE, 1);
}
static int __init test_usercopy_init(void)
{
test_usercopy_kernel();
return 0;
}
static void __exit test_usercopy_exit(void)
{
}
module_init(test_usercopy_init);
module_exit(test_usercopy_exit);
MODULE_LICENSE("GPL");
```
Running the testcase cause kernel oops,
and then the call stack is incorrect:
```
usercopy: Kernel memory exposure attempt detected from kernel text
Kernel BUG at usercopy_abort+0x98/0x9c
Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
Modules linked in: test_usercopy(O+) usbcore usb_common
CPU: 0 PID: 609 Comm: insmod Tainted: G O 5.10.0 #11
Hardware name: Hisilicon A9
PC is at usercopy_abort+0x98/0x9c
LR is at usercopy_abort+0x98/0x9c
[...]
(usercopy_abort) from (memfd_fcntl+0x0/0x654)
(memfd_fcntl) from (0xef7368f8)
Code: e1a01004 e58dc004 e58de000 eb108378 (e7f001f2)
---[ end trace e5fdc684259b0883 ]---
Kernel panic - not syncing: Fatal exception
```
Why Does the kernel backtrace cause errors?
Because usercopy_abort is marked as __noreturn.
You can see the related disassembling:
```
c02f24ac <__check_object_size>:
static_key_count():
[...]
check_kernel_text_object():
linux/mm/usercopy.c:125
usercopy_abort("kernel text", NULL, to_user, ptr - textlow, n);
c02f26c4: e3040110 movw r0, #16656 ; 0x4110
c02f26c8: e58d5000 str r5, [sp]
c02f26cc: e0443003 sub r3, r4, r3
c02f26d0: e1a02007 mov r2, r7
c02f26d4: e34c0096 movt r0, #49302 ; 0xc096
c02f26d8: ebffff4c bl c02f2410 <usercopy_abort>
c02f26dc <memfd_fcntl>:
memfd_fcntl():
[...]
```
The last instruction of __check_object_size is a call to usercopy_abort,
which is a noreturn function, the return address falls outside of the
__check_object_size function boundary, the return address falls into
the next function memfd_fcntl, therefore,
an error occurs when the kernel backtrace.
Delete __noreturn from usercopy_abort,
the correct call stack is as follow:
```
(usercopy_abort) from (__check_object_size+0x170/0x234)
(__check_object_size) from (test_usercopy_init+0x8/0xc0 [test_usercopy])
(test_usercopy_init [test_usercopy]) from (do_one_initcall+0xac/0x204)
(do_one_initcall) from (do_init_module+0x44/0x1c8)
(do_init_module) from (load_module+0x1d48/0x2434)
(load_module) from (sys_finit_module+0xc0/0xf4)
(sys_finit_module) from (ret_fast_syscall+0x0/0x50)
```
Fixes: b394d468e7d7 ("usercopy: Enhance and rename report_usercopy()")
Signed-off-by: Jiangfeng Xiao <[email protected]>
---
include/linux/uaccess.h | 2 +-
mm/usercopy.c | 2 +-
tools/objtool/noreturns.h | 1 -
3 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 3064314..c37af70 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -437,7 +437,7 @@ static inline void user_access_restore(unsigned long flags) { }
#endif
#ifdef CONFIG_HARDENED_USERCOPY
-void __noreturn usercopy_abort(const char *name, const char *detail,
+void usercopy_abort(const char *name, const char *detail,
bool to_user, unsigned long offset,
unsigned long len);
#endif
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 83c164a..ca1b22e 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -83,7 +83,7 @@ static noinline int check_stack_object(const void *obj, unsigned long len)
* kmem_cache_create_usercopy() function to create the cache (and
* carefully audit the whitelist range).
*/
-void __noreturn usercopy_abort(const char *name, const char *detail,
+void usercopy_abort(const char *name, const char *detail,
bool to_user, unsigned long offset,
unsigned long len)
{
diff --git a/tools/objtool/noreturns.h b/tools/objtool/noreturns.h
index 1685d7e..c7e341c 100644
--- a/tools/objtool/noreturns.h
+++ b/tools/objtool/noreturns.h
@@ -40,7 +40,6 @@
NORETURN(snp_abort)
NORETURN(start_kernel)
NORETURN(stop_this_cpu)
-NORETURN(usercopy_abort)
NORETURN(x86_64_start_kernel)
NORETURN(x86_64_start_reservations)
NORETURN(xen_cpu_bringup_again)
--
1.8.5.6
On Mon, Mar 04, 2024 at 04:15:07PM +0100, Jann Horn wrote:
> On Mon, Mar 4, 2024 at 3:02 AM Jiangfeng Xiao <[email protected]> wrote:
> > When the last instruction of a noreturn function is a call
> > to another function, the return address falls outside
> > of the function boundary. This seems to cause kernel
> > to interrupt the backtrace.
FWIW, all email from huawei.com continues to get eaten by anti-spam
checking. I've reported this a few times -- it'd be really nice if the
domain configuration could get fixed.
> [...]
> > Delete __noreturn from usercopy_abort,
>
> This sounds like the actual bug is in the backtracing logic? I don't
> think removing __noreturn annotations from an individual function is a
> good fix, since the same thing can happen with other __noreturn
> functions depending on what choices the compiler makes.
Yeah, NAK. usercopy_abort() doesn't return. It ends with BUG().
--
Kees Cook
On 2024/3/4 23:15, Jann Horn wrote:
> On Mon, Mar 4, 2024 at 3:02 AM Jiangfeng Xiao <[email protected]> wrote:
>> When the last instruction of a noreturn function is a call
>> to another function, the return address falls outside
>> of the function boundary. This seems to cause kernel
>> to interrupt the backtrace.
> [...]
>> Delete __noreturn from usercopy_abort,
>
> This sounds like the actual bug is in the backtracing logic? I don't
> think removing __noreturn annotations from an individual function is a
> good fix, since the same thing can happen with other __noreturn
> functions depending on what choices the compiler makes.
> .
>
Yes, you make a point. This may be a bug is in the backtracing logic, but
the kernel backtracing always parses symbols using (lr) instead of (lr-4).
This may be due to historical reasons or more comprehensive considerations.
In addition, modifying the implementation logic of the kernel backtracing
has a great impact. Therefore, I do not dare to modify the implementation
logic of the kernel backtracing.
Not all noreturn functions are ended with calling other functions.
Therefore, only a few individual functions may have the same problem.
In addition, deleting '__noreturn' from usercopy_abort does not
change the internal behavior of usercopy_abort function.
Therefore, there is no risk. Deleting '__noreturn' from usercopy_abort
is the solution that I can think of with minimal impact and minimum risk.
If you will submit a better patch to solve this problem,
I would like to learn from you. Thank you.
On 2024/3/5 10:54, Jiangfeng Xiao wrote:
>
>
> On 2024/3/4 23:15, Jann Horn wrote:
>> On Mon, Mar 4, 2024 at 3:02 AM Jiangfeng Xiao <[email protected]> wrote:
>>> When the last instruction of a noreturn function is a call
>>> to another function, the return address falls outside
>>> of the function boundary. This seems to cause kernel
>>> to interrupt the backtrace.
>> [...]
>>> Delete __noreturn from usercopy_abort,
>>
>> This sounds like the actual bug is in the backtracing logic? I don't
>> think removing __noreturn annotations from an individual function is a
>> good fix, since the same thing can happen with other __noreturn
>> functions depending on what choices the compiler makes.
>> .
>>
> Yes, you make a point. This may be a bug is in the backtracing logic, but
> the kernel backtracing always parses symbols using (lr) instead of (lr-4).
> This may be due to historical reasons or more comprehensive considerations.
> In addition, modifying the implementation logic of the kernel backtracing
> has a great impact. Therefore, I do not dare to modify the implementation
> logic of the kernel backtracing.
>
> Not all noreturn functions are ended with calling other functions.
> Therefore, only a few individual functions may have the same problem.
> In addition, deleting '__noreturn' from usercopy_abort does not
> change the internal behavior of usercopy_abort function.
> Therefore, there is no risk. Deleting '__noreturn' from usercopy_abort
> is the solution that I can think of with minimal impact and minimum risk.
>
> If you will submit a better patch to solve this problem,
> I would like to learn from you. Thank you.
>
What are your suggestions on modifying the kernel backtracing logic
or deleting '__noretrun'? If you insist on modifying the kernel
backtracing logic and persuade me with good reasons, I can also try
to submit the patch for modifying the kernel backtracing logic
to the community.
On 2024/3/5 1:40, Kees Cook wrote:
> On Mon, Mar 04, 2024 at 04:15:07PM +0100, Jann Horn wrote:
>> On Mon, Mar 4, 2024 at 3:02 AM Jiangfeng Xiao <[email protected]> wrote:
>>> When the last instruction of a noreturn function is a call
>>> to another function, the return address falls outside
>>> of the function boundary. This seems to cause kernel
>>> to interrupt the backtrace.
>
> FWIW, all email from huawei.com continues to get eaten by anti-spam
> checking. I've reported this a few times -- it'd be really nice if the
> domain configuration could get fixed.
>
>> [...]
>>> Delete __noreturn from usercopy_abort,
>>
>> This sounds like the actual bug is in the backtracing logic? I don't
>> think removing __noreturn annotations from an individual function is a
>> good fix, since the same thing can happen with other __noreturn
>> functions depending on what choices the compiler makes.
>
> Yeah, NAK. usercopy_abort() doesn't return. It ends with BUG().
>
When the user directly or indirectly calls usercopy_abort,
the final call stack is incorrect, and the
code where the problem occurs cannot be located.
In this case, the user will be frustrated.
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.
On Mon, Mar 4, 2024 at 3:02 AM Jiangfeng Xiao <xiaojiangfeng@huaweicom> wrote:
> When the last instruction of a noreturn function is a call
> to another function, the return address falls outside
> of the function boundary. This seems to cause kernel
> to interrupt the backtrace.
[...]
> Delete __noreturn from usercopy_abort,
This sounds like the actual bug is in the backtracing logic? I don't
think removing __noreturn annotations from an individual function is a
good fix, since the same thing can happen with other __noreturn
functions depending on what choices the compiler makes.
On Tue, Mar 05, 2024 at 11:31:06AM +0800, Jiangfeng Xiao wrote:
>
>
> On 2024/3/5 1:40, Kees Cook wrote:
> > On Mon, Mar 04, 2024 at 04:15:07PM +0100, Jann Horn wrote:
> >> On Mon, Mar 4, 2024 at 3:02 AM Jiangfeng Xiao <[email protected]> wrote:
> >>> When the last instruction of a noreturn function is a call
> >>> to another function, the return address falls outside
> >>> of the function boundary. This seems to cause kernel
> >>> to interrupt the backtrace.
> >
> > FWIW, all email from huawei.com continues to get eaten by anti-spam
> > checking. I've reported this a few times -- it'd be really nice if the
> > domain configuration could get fixed.
> >
> >> [...]
> >>> Delete __noreturn from usercopy_abort,
> >>
> >> This sounds like the actual bug is in the backtracing logic? I don't
> >> think removing __noreturn annotations from an individual function is a
> >> good fix, since the same thing can happen with other __noreturn
> >> functions depending on what choices the compiler makes.
> >
> > Yeah, NAK. usercopy_abort() doesn't return. It ends with BUG().
> >
> When the user directly or indirectly calls usercopy_abort,
> the final call stack is incorrect, and the
> code where the problem occurs cannot be located.
> In this case, the user will be frustrated.
Can you please give an example of this?
> 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.
--
Kees Cook
> >> 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/[email protected]
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)
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!
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
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)
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/[email protected]
> 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)
> .
>
On 2024/3/5 17:32, Kees Cook wrote:
> On Tue, Mar 05, 2024 at 11:31:06AM +0800, Jiangfeng Xiao wrote:
>>
>>
>> On 2024/3/5 1:40, Kees Cook wrote:
>>> On Mon, Mar 04, 2024 at 04:15:07PM +0100, Jann Horn wrote:
>>>> On Mon, Mar 4, 2024 at 3:02 AM Jiangfeng Xiao <[email protected]> wrote:
>>>>> When the last instruction of a noreturn function is a call
>>>>> to another function, the return address falls outside
>>>>> of the function boundary. This seems to cause kernel
>>>>> to interrupt the backtrace.
>>>
>>> FWIW, all email from huawei.com continues to get eaten by anti-spam
>>> checking. I've reported this a few times -- it'd be really nice if the
>>> domain configuration could get fixed.
>>>
>>>> [...]
>>>>> Delete __noreturn from usercopy_abort,
>>>>
>>>> This sounds like the actual bug is in the backtracing logic? I don't
>>>> think removing __noreturn annotations from an individual function is a
>>>> good fix, since the same thing can happen with other __noreturn
>>>> functions depending on what choices the compiler makes.
>>>
>>> Yeah, NAK. usercopy_abort() doesn't return. It ends with BUG().
>>>
>> When the user directly or indirectly calls usercopy_abort,
>> the final call stack is incorrect, and the
>> code where the problem occurs cannot be located.
>> In this case, the user will be frustrated.
>
> Can you please give an example of this?
The main configurations of my kernel are as follows:
CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE is enabled.
(This config uses the compilation parameter -O2.)
CONFIG_RELOCATABLE is disabled.
(This config uses the compilation option -fpic.)
You can use the following kernel module testcase
to reproduce this problem on the ARM32 platform.
```
#include <linux/module.h>
#include <linux/sched.h>
static volatile size_t unconst = 0;
/*
check_object_size
__check_object_size
check_kernel_text_object
usercopy_abort("kernel text", ...)
*/
void test_usercopy_kernel(void)
{
check_object_size(schedule, unconst + PAGE_SIZE, 1);
}
static int __init test_usercopy_init(void)
{
test_usercopy_kernel();
return 0;
}
static void __exit test_usercopy_exit(void)
{
}
module_init(test_usercopy_init);
module_exit(test_usercopy_exit);
MODULE_LICENSE("GPL");
```
>
>> 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.
>
Thank you for providing this information.
I'll go back to further understand how __noreturn is used
in features such as KCFI and livepatching.
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 <[email protected]>
Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
Signed-off-by: Jiangfeng Xiao <[email protected]>
---
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
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 <[email protected]>
Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
Signed-off-by: Jiangfeng Xiao <[email protected]>
---
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
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 <[email protected]>
> Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
> Signed-off-by: Jiangfeng Xiao <[email protected]>
> ---
> 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!
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 <[email protected]>
>> Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
>> Signed-off-by: Jiangfeng Xiao <[email protected]>
>> ---
>> 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
>>
>>
>
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 <[email protected]>
Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/
Suggested-by: "Russell King (Oracle)" <[email protected]>
Link: https://lkml.kernel.org/lkml/[email protected]/
Signed-off-by: Jiangfeng Xiao <[email protected]>
---
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
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!
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)
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)
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!
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)
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)
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)
On Thu, 21 Mar 2024 at 12:24, Russell King (Oracle)
<[email protected]> 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.
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!
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!
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!
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!
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!
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.
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!
On 2024/3/6 1:58, Josh Poimboeuf wrote:
> Adding ARM folks -- see
> https://lkml.kernel.org/lkml/[email protected]
> 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?
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)
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!
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.
..
> 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)