arm: Add unwinding annotations for 64bit division functions
Subsystems (2)
PATCH 1/1 arm: Add unwinding annotations for 64bit division functions
The 64bit division functions never had unwinding annotations added. This prevents a backtrace from being printed within the function and if a division by 0 occurs. Add the annotations.
--- arch/arm/lib/div64.S | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
arch/arm/lib/div64.S +8 -0
@@ -13,6 +13,7 @@ */ #include <linux/linkage.h> +#include <asm/unwind.h> #ifdef __ARMEB__ #define xh r0
@@ -44,6 +45,7 @@ */ ENTRY(__do_div64) +UNWIND(.fnstart) @ Test for easy paths first. subs ip, r4, #1
@@ -189,7 +191,12 @@ ENTRY(__do_div64) moveq yh, xh moveq xh, #0 moveq pc, lr +UNWIND(.fnend) +UNWIND(.fnstart) +UNWIND(.pad #4) +UNWIND(.save {lr}) +Ldiv0_64: @ Division by 0: str lr, [sp, #-8]! bl __div0
@@ -200,4 +207,5 @@ ENTRY(__do_div64) mov xh, #0 ldr pc, [sp], #8 +UNWIND(.fnend) ENDPROC(__do_div64)
Signature · 1 line
1.7.3.3
Quoted 30 lines · Laura Abbott
The 64bit division functions never had unwinding annotations added. This prevents a backtrace from being printed within the function and if a division by 0 occurs. Add the annotations.
Signed-off-by Laura Abbott <[email protected]>--- arch/arm/lib/div64.S | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)arch/arm/lib/div64.S +2
-0@@ -13,6 +13,7 @@ */ #include <linux/linkage.h> +#include <asm/unwind.h> #ifdef __ARMEB__ #define xh r0@@ -44,6 +45,7 @@ */ ENTRY(__do_div64) +UNWIND(.fnstart) @ Test for easy paths first. subs ip, r4, #1
arch/arm/lib/div64.S ENTRY(__do_div64) · line 199
Quoted 9 lines
arch/arm/lib/div64.S +5
-0@@ -189,7 +191,12 @@ ENTRY(__do_div64) moveq yh, xh moveq xh, #0 moveq pc, lr +UNWIND(.fnend) +UNWIND(.fnstart) +UNWIND(.pad #4) +UNWIND(.save {lr}) +Ldiv0_64:
Why this phony fnend+fnstart here?
Nicolas
Quoted 37 lines · Nicolas Pitre
Quoted 35 lines · Laura Abbott
The 64bit division functions never had unwinding annotations added. This prevents a backtrace from being printed within the function and if a division by 0 occurs. Add the annotations.
Signed-off-by Laura Abbott <[email protected]>--- arch/arm/lib/div64.S | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)arch/arm/lib/div64.S +3
-0@@ -13,6 +13,7 @@ */ #include <linux/linkage.h> +#include <asm/unwind.h> #ifdef __ARMEB__ #define xh r0@@ -44,6 +45,7 @@ */ ENTRY(__do_div64) +UNWIND(.fnstart) @ Test for easy paths first. subs ip, r4, #1@@ -189,7 +191,12 @@ ENTRY(__do_div64) moveq yh, xh moveq xh, #0 moveq pc, lr +UNWIND(.fnend)
Quoted 7 lines
arch/arm/lib/div64.S ENTRY(__do_div64) · line 199
Quoted 4 lines
arch/arm/lib/div64.S +4
-0+UNWIND(.fnstart) +UNWIND(.pad #4) +UNWIND(.save {lr}) +Ldiv0_64:Why this phony fnend+fnstart here?
If a division by 0 occurs, we need to be able to access the saved LR on the stack which is setup right before calling the __div0 function. This can't go at the top of __do_div64 because if we try to do a backtrace from within __do_div64 the annotation won't be correct as the LR was never saved on the stack. (yes, __do_div64 is all register math but it's still possible to take a prefetch abort in the middle of that function. Taking a prefetch abort in the middle of __do_div64 is what found this issue in the first place.) This setup handles both cases correctly.
Quoted 3 lines
Nicolas
Laura
Quoted 5 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Nicolas Pitre
Acked-byQuoted 46 lines · Laura Abbott
Quoted 37 lines · Nicolas Pitre
Quoted 35 lines · Laura Abbott
The 64bit division functions never had unwinding annotations added. This prevents a backtrace from being printed within the function and if a division by 0 occurs. Add the annotations.
Signed-off-by Laura Abbott <[email protected]>--- arch/arm/lib/div64.S | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)arch/arm/lib/div64.S +3
-0@@ -13,6 +13,7 @@ */ #include <linux/linkage.h> +#include <asm/unwind.h> #ifdef __ARMEB__ #define xh r0@@ -44,6 +45,7 @@ */ ENTRY(__do_div64) +UNWIND(.fnstart) @ Test for easy paths first. subs ip, r4, #1@@ -189,7 +191,12 @@ ENTRY(__do_div64) moveq yh, xh moveq xh, #0 moveq pc, lr +UNWIND(.fnend)Quoted 7 lines
arch/arm/lib/div64.S ENTRY(__do_div64) · line 199
Quoted 4 lines
arch/arm/lib/div64.S +4
-0+UNWIND(.fnstart) +UNWIND(.pad #4) +UNWIND(.save {lr}) +Ldiv0_64:Why this phony fnend+fnstart here?
Quoted 5 lines
If a division by 0 occurs, we need to be able to access the saved LR on the stack which is setup right before calling the
__div0function. This can't go at the top of__do_div64because if we try to do a backtrace from within__do_div64the annotation won't be correct as the LR was never saved on the stack.
I suppose the debug infrastructure always assume the same stack frame for the whole function, hence you can't put that .pad and .save right before the call to __div0 without the .fnstart and have it effective only there?
If so then:
Nicolas
Catalin Marinas
Acked-byQuoted 22 lines · Nicolas Pitre
Quoted 15 lines · Laura Abbott
Quoted 7 lines · Nicolas Pitre
Quoted 6 lines · Laura Abbott
arch/arm/lib/div64.S +1
-0@@ -189,7 +191,12 @@ ENTRY(__do_div64) moveq yh, xh moveq xh, #0 moveq pc, lr +UNWIND(.fnend)Quoted 6 lines
arch/arm/lib/div64.S ENTRY(__do_div64) · line 199
Quoted 4 lines
arch/arm/lib/div64.S +4
-0+UNWIND(.fnstart) +UNWIND(.pad #4) +UNWIND(.save {lr}) +Ldiv0_64:Why this phony fnend+fnstart here?
Quoted 5 lines
If a division by 0 occurs, we need to be able to access the saved LR on the stack which is setup right before calling the
__div0function. This can't go at the top of__do_div64because if we try to do a backtrace from within__do_div64the annotation won't be correct as the LR was never saved on the stack.
Quoted 4 lines
I suppose the debug infrastructure always assume the same stack frame for the whole function, hence you can't put that .pad and .save right before the call to
__div0without the .fnstart and have it effective only there?
An element in the unwinding table contains a pointer to the .fnstart location and a set of instruction bytecodes (like .save) to the next .fnend statement. If more 4 bytecodes (to fit in an int), it contains an index to another table with more bytecodes. The elements in the unwinding table are sorted by the .fnstart address.
So I think in the above case if we don't have the .fnend/.fnstart for Ldiv0_64 we would get all the instructions in the __do_div64 unwinding information. I don't think the unwinding bytecode is that smart to allow other tricks like checking some conditions.
Unless anyone has a better idea to specify this:
Quoted 46 lines · Laura Abbott
Quoted 37 lines · Nicolas Pitre
Quoted 35 lines · Laura Abbott
The 64bit division functions never had unwinding annotations added. This prevents a backtrace from being printed within the function and if a division by 0 occurs. Add the annotations.
Signed-off-by Laura Abbott <[email protected]>--- arch/arm/lib/div64.S | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)arch/arm/lib/div64.S +3
-0@@ -13,6 +13,7 @@ */ #include <linux/linkage.h> +#include <asm/unwind.h> #ifdef __ARMEB__ #define xh r0@@ -44,6 +45,7 @@ */ ENTRY(__do_div64) +UNWIND(.fnstart) @ Test for easy paths first. subs ip, r4, #1@@ -189,7 +191,12 @@ ENTRY(__do_div64) moveq yh, xh moveq xh, #0 moveq pc, lr +UNWIND(.fnend)Quoted 7 lines
arch/arm/lib/div64.S ENTRY(__do_div64) · line 199
Quoted 4 lines
arch/arm/lib/div64.S +4
-0+UNWIND(.fnstart) +UNWIND(.pad #4) +UNWIND(.save {lr}) +Ldiv0_64:Why this phony fnend+fnstart here?
Quoted 8 lines
If a division by 0 occurs, we need to be able to access the saved LR on the stack which is setup right before calling the
__div0function. This can't go at the top of__do_div64because if we try to do a backtrace from within__do_div64the annotation won't be correct as the LR was never saved on the stack. (yes,__do_div64is all register math but it's still possible to take a prefetch abort in the middle of that function. Taking a prefetch abort in the middle of__do_div64is what found this issue in the first place.) This setup handles both cases correctly.
Talking to Catalin a bit more, it sounds like prefetch aborts should not happen in kernel code, and data aborts should not happen when accessing the kernel stack. If either of these happens, there's no guarantee that the kernel will be able to recover from the situation fully, so backtracing may not be the primary concern.
The kinds of situation that the backtracer _can_ cope with are:
a) explicit, synchonous backtrace requests (BUG() etc. within C functions)
b) aborts when accessing random memory (but not the stack) outside the function progolgue/epilogue sequences (i.e., so that the state the unwind info described actually matches reality)
If you get a data abort when accessing the kernel stack, the backtracer probably won't work at all, because it relies on the same stack. Faults during prologue/epilogue sequences can't be handled correctly because the unwind annotations don't descirbe the interim state during these sequences. There is no general way to use the unwinder annotations to describe fully how the state evolves during prologue/epilogue sequences.
So, having thought about it, it may be that having the extra .fnstart/ .fnend pair doesn't really help that much after all.
For this code, we do want the unwind state to be properly recorded for the "bl __div0" call site after Ldiv0_64; but we probably don't need to worry about backtracing from any other point in this code -- partly becuase the probably of needing to do it is very low, and partly because we will likely fail to produce a backtrace in this situation. If this is a reasonable assumption, then a single unwind annotation block should be sufficient:
ENTRY(__do_div64)
UNWIND(.fnstart)
subs ip, r4, #1 @ ... moveq pc, lr
UNWIND(.pad #4)
UNWIND(.save {lr}
stmfd sp!, {r0, lr}
@ ...
bl __div0
@ ...UNWIND(.fnend)
The location of the .pad and .save is of course not meaningful for the unwind data: only their respective order, and the fact that they appear between the .fnstart/.fnend directives. Putting them next to the stmfd just makes the code easier to read.
The extra annotations in the original patch are harmless, though.
Cheers
---Dave
Quoted 3 lines · Dave Martin
Talking to Catalin a bit more, it sounds like prefetch aborts should not happen in kernel code, and data aborts should not happen when accessing the kernel stack.
No faults should happen in kernel code, except for:
1. instructions specifically marked in the exception table, which are used to access user memory.
2. instructions causing an 'undefined instruction' exception.
Standard ARM instructions like 'add', 'mov' etc should _never_ fault, and if they do that means your core isn't executing ARM instructions correctly (eg, the hardware design is faulty.)
Instructions such as VFP, kprobes tracing, etc are expected fault locations, and those are fairly well controlled where they can be placed. With things like ftrace, it certainly is the case that the unwinder can theoretically be called from almost anywhere in a function.
So I suggest that this does need to be fixed, and you can't rely on "prefetch aborts should not happen". That's true of prefetch aborts but not of other aborts.
Quoted 15 lines · Russell King - ARM Linux
Quoted 3 lines · Dave Martin
Talking to Catalin a bit more, it sounds like prefetch aborts should not happen in kernel code, and data aborts should not happen when accessing the kernel stack.
No faults should happen in kernel code, except for:
1. instructions specifically marked in the exception table, which are used to access user memory.
2. instructions causing an 'undefined instruction' exception.Standard ARM instructions like 'add', 'mov' etc should _never_ fault, and if they do that means your core isn't executing ARM instructions correctly (eg, the hardware design is faulty.)
Quoted 8 lines
Instructions such as VFP, kprobes tracing, etc are expected fault locations, and those are fairly well controlled where they can be placed. With things like ftrace, it certainly is the case that the unwinder can theoretically be called from almost anywhere in a function.
So I suggest that this does need to be fixed, and you can't rely on "prefetch aborts should not happen". That's true of prefetch aborts but not of other aborts.
The important thing for the unwinder is that it can't cope well with faults happening in the save/restore sequences at function entry and exit, and we may not cope well with functions which don't have a simple SAVE, EXECUTE, RESTORE, RETURN structure.
My gut feeling is that neither (1) or (2) should happen in those sequences, and VFP faults should not happen in these sequences because the kernel should not contain VFP code except in particular controlled locations.
For things like kprobes which allow a trap to be set at a function's entry point we do have a problem: if we try to backtrace from this point, the backtracer will see we are in that function and will assume that the function's state saving code has already executed. It might be simple to work around this particular case by making the unwinder intelligent enough to realise that if backtracing from the first instruction of a function, none of the function's state save code can have executed yet.
Quoted 4 lines · Russell King - ARM Linux
Instructions such as VFP, kprobes tracing, etc are expected fault locations, and those are fairly well controlled where they can be placed. With things like ftrace, it certainly is the case that the unwinder can theoretically be called from almost anywhere in a function.
Actually, kprobes can be places on any instruction in the kernel that isn't in the section .kprobes.text.
I also strongly suspect that stack unwinding won't happen correctly across the boundary between the kprobes handling code and the function which was probed - there's an awful lot of stack jiggery pokery going on there.
Signature · 1 line
Tixy
Quoted 6 lines · Jon Medhurst (Tixy)
Quoted 4 lines · Russell King - ARM Linux
Instructions such as VFP, kprobes tracing, etc are expected fault locations, and those are fairly well controlled where they can be placed. With things like ftrace, it certainly is the case that the unwinder can theoretically be called from almost anywhere in a function.
Quoted 7 lines
Actually, kprobes can be places on any instruction in the kernel that isn't in the section .kprobes.text.
I also strongly suspect that stack unwinding won't happen correctly across the boundary between the kprobes handling code and the function which was probed - there's an awful lot of stack jiggery pokery going on there.
Are people most likely to place kprobes on the first instruction of a function? We could improve things a bit in the unwinder and assume that if the fault address is the same as the .fnstart address, the return value is always in LR and the SP not affected (that's unwinding bytecode 0xb0). For a few instructions into the function prologue we can't reliably get the unwinding information.
Signature · 1 line
Catalin
Quoted 10 lines · Catalin Marinas
Quoted 9 lines · Jon Medhurst (Tixy)
Quoted 4 lines · Russell King - ARM Linux
Instructions such as VFP, kprobes tracing, etc are expected fault locations, and those are fairly well controlled where they can be placed. With things like ftrace, it certainly is the case that the unwinder can theoretically be called from almost anywhere in a function.
Actually, kprobes can be places on any instruction in the kernel that isn't in the section .kprobes.text.
Quoted 7 lines
Quoted 4 lines
I also strongly suspect that stack unwinding won't happen correctly across the boundary between the kprobes handling code and the function which was probed - there's an awful lot of stack jiggery pokery going on there.
Are people most likely to place kprobes on the first instruction of a function?
I believe that is the usual case.
Quoted 5 lines
We could improve things a bit in the unwinder and assume that if the fault address is the same as the .fnstart address, the return value is always in LR and the SP not affected (that's unwinding bytecode
0xb0). For a few instructions into the function prologue we can't reliably get the unwinding information.
That would help make it possible to unwind out of kprobes handlers to the probed function. The kprobes code itself would need work as well, and possibly the undef handler. Do we think it is worthwhile to do this?
Signature · 1 line
Tixy
Quoted 11 lines · Jon Medhurst (Tixy)
Quoted 5 lines · Catalin Marinas
We could improve things a bit in the unwinder and assume that if the fault address is the same as the .fnstart address, the return value is always in LR and the SP not affected (that's unwinding bytecode
0xb0). For a few instructions into the function prologue we can't reliably get the unwinding information.That would help make it possible to unwind out of kprobes handlers to the probed function. The kprobes code itself would need work as well, and possibly the undef handler. Do we think it is worthwhile to do this?
Does kprobes need to trace beyond the probed function? If not, you get the address of the probed function via pt_regs anyway, so no need for unwinding beyond that.
Signature · 1 line
Catalin
Quoted 8 lines · Catalin Marinas
Quoted 7 lines · Jon Medhurst (Tixy)
Quoted 5 lines · Catalin Marinas
We could improve things a bit in the unwinder and assume that if the fault address is the same as the .fnstart address, the return value is always in LR and the SP not affected (that's unwinding bytecode
0xb0). For a few instructions into the function prologue we can't reliably get the unwinding information.
Quoted 8 lines
Quoted 4 lines
That would help make it possible to unwind out of kprobes handlers to the probed function. The kprobes code itself would need work as well, and possibly the undef handler. Do we think it is worthwhile to do this?
Does kprobes need to trace beyond the probed function? If not, you get the address of the probed function via
pt_regsanyway, so no need for unwinding beyond that.
To be honest, I'm not very sure how kprobes get used in the real world. Though, if stack unwinding from their handlers currently doesn't work and people had a usecase for it, we would expect them to complain.
Signature · 1 line
Tixy
Quoted 14 lines · Jon Medhurst (Tixy)
Quoted 13 lines · Catalin Marinas
Quoted 11 lines · Jon Medhurst (Tixy)
Quoted 5 lines · Catalin Marinas
We could improve things a bit in the unwinder and assume that if the fault address is the same as the .fnstart address, the return value is always in LR and the SP not affected (that's unwinding bytecode
0xb0). For a few instructions into the function prologue we can't reliably get the unwinding information.That would help make it possible to unwind out of kprobes handlers to the probed function. The kprobes code itself would need work as well, and possibly the undef handler. Do we think it is worthwhile to do this?
Quoted 7 lines
Quoted 3 lines
Does kprobes need to trace beyond the probed function? If not, you get the address of the probed function via
pt_regsanyway, so no need for unwinding beyond that.To be honest, I'm not very sure how kprobes get used in the real world. Though, if stack unwinding from their handlers currently doesn't work and people had a usecase for it, we would expect them to complain.
The unwinding fix should be simple (I haven't tested it yet):
8<-----------------------------
ARM: Ignore the unwinding information for the first instruction in a function
From: Catalin Marinas <[email protected]>
When backtracing from the first instruction of a function, the prologue has not been executed and the unwinding information is not valid. This patch checks for this case and just assumes that the return address is in LR.
--- arch/arm/kernel/unwind.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
arch/arm/kernel/unwind.c +10 -0
@@ -293,6 +293,16 @@ int unwind_frame(struct stackframe *frame)return -URC_FAILURE ; } +/* +* Check for backtrace on the first instruction of a function. The +* prologue has not been executed yet and the unwinding information is +* not valid. Assume that the return address is in LR. +*/ +if (idx.addr == frame->pc) { + frame->pc = frame->lr; +return URC_OK ; + } + ctrl.vrs[FP ] = frame->fp; ctrl.vrs[SP ] = frame->sp; ctrl.vrs[LR ] = frame->lr;
Signature · 1 line
Catalin
Quoted 29 lines · Catalin Marinas
The unwinding fix should be simple (I haven't tested it yet):
8<-----------------------------
ARM: Ignore the unwinding information for the first instruction in a functionFrom: Catalin Marinas <[email protected]>
When backtracing from the first instruction of a function, the prologue has not been executed and the unwinding information is not valid. This patch checks for this case and just assumes that the return address is in LR.
Signed-off-by Catalin Marinas <[email protected]>--- arch/arm/kernel/unwind.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)arch/arm/kernel/unwind.c +3
-0@@ -293,6 +293,16 @@ int unwind_frame(struct stackframe *frame)return -URC_FAILURE ; } +/* +* Check for backtrace on the first instruction of a function. The +* prologue has not been executed yet and the unwinding information is
arch/arm/kernel/unwind.c int unwind_frame(struct stackframe *frame) · line 308
Quoted 10 lines
arch/arm/kernel/unwind.c +7
-0+* not valid. Assume that the return address is in LR. +*/ +if (idx.addr == frame->pc) { + frame->pc = frame->lr; +return URC_OK ; + } + ctrl.vrs[FP ] = frame->fp; ctrl.vrs[SP ] = frame->sp; ctrl.vrs[LR ] = frame->lr;
I've never looked at the unwinding code before but the one comment I would make is: does the patch work with Thumb code? I.e. does bit zero of idx.addr, frame->pc or frame->lr ever get set to indicate Thumb state? And if so, they had better all get set otherwise it won't work :-)
Signature · 1 line
Tixy