From: Laura Abbott <hidden> Date: 2011-09-19 22:13:16
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 <redacted>
---
arch/arm/lib/div64.S | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
From: Nicolas Pitre <nico@fluxnic.net> Date: 2011-09-19 23:23:00
On Mon, 19 Sep 2011, Laura Abbott wrote:
quoted hunk
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 <redacted>
---
arch/arm/lib/div64.S | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
From: Laura Abbott <hidden> Date: 2011-09-20 01:55:41
On Mon, September 19, 2011 4:22 pm, Nicolas Pitre wrote:
On Mon, 19 Sep 2011, Laura Abbott wrote:
quoted
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 <redacted>
---
arch/arm/lib/div64.S | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
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.
From: Nicolas Pitre <nico@fluxnic.net> Date: 2011-09-20 13:27:07
On Mon, 19 Sep 2011, Laura Abbott wrote:
On Mon, September 19, 2011 4:22 pm, Nicolas Pitre wrote:
quoted
On Mon, 19 Sep 2011, Laura Abbott wrote:
quoted
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 <redacted>
---
arch/arm/lib/div64.S | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
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.
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:
Acked-by: Nicolas Pitre <redacted>
Nicolas
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.
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?
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:
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
From: Dave Martin <hidden> Date: 2011-09-21 11:39:22
On Mon, Sep 19, 2011 at 06:55:39PM -0700, Laura Abbott wrote:
On Mon, September 19, 2011 4:22 pm, Nicolas Pitre wrote:
quoted
On Mon, 19 Sep 2011, Laura Abbott wrote:
quoted
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 <redacted>
---
arch/arm/lib/div64.S | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
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.
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
From: Russell King - ARM Linux <hidden> Date: 2011-09-21 11:56:16
On Wed, Sep 21, 2011 at 12:39:09PM +0100, Dave Martin wrote:
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.
From: Dave Martin <hidden> Date: 2011-09-21 13:33:18
On Wed, Sep 21, 2011 at 12:55:53PM +0100, Russell King - ARM Linux wrote:
On Wed, Sep 21, 2011 at 12:39:09PM +0100, Dave Martin wrote:
quoted
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.
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.
From: Jon Medhurst (Tixy) <hidden> Date: 2011-09-22 08:11:27
On Wed, 2011-09-21 at 12:55 +0100, Russell King - ARM Linux wrote:
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.
--
Tixy
On 22 September 2011 08:28, Jon Medhurst (Tixy) [off-list ref] wrote:
On Wed, 2011-09-21 at 12:55 +0100, Russell King - ARM Linux wrote:
quoted
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.
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.
--
Catalin
From: Jon Medhurst (Tixy) <hidden> Date: 2011-09-22 11:06:57
On Thu, 2011-09-22 at 10:48 +0100, Catalin Marinas wrote:
On 22 September 2011 08:28, Jon Medhurst (Tixy) [off-list ref] wrote:
quoted
On Wed, 2011-09-21 at 12:55 +0100, Russell King - ARM Linux wrote:
quoted
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.
Are people most likely to place kprobes on the first instruction of a
function?
I believe that is the usual case.
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?
--
Tixy
On Thu, Sep 22, 2011 at 12:06:46PM +0100, Jon Medhurst (Tixy) wrote:
On Thu, 2011-09-22 at 10:48 +0100, Catalin Marinas wrote:
quoted
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.
--
Catalin
From: Jon Medhurst (Tixy) <hidden> Date: 2011-09-22 12:13:10
On Thu, 2011-09-22 at 12:57 +0100, Catalin Marinas wrote:
On Thu, Sep 22, 2011 at 12:06:46PM +0100, Jon Medhurst (Tixy) wrote:
quoted
On Thu, 2011-09-22 at 10:48 +0100, Catalin Marinas wrote:
quoted
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.
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.
--
Tixy
On Thu, Sep 22, 2011 at 01:13:01PM +0100, Jon Medhurst (Tixy) wrote:
On Thu, 2011-09-22 at 12:57 +0100, Catalin Marinas wrote:
quoted
On Thu, Sep 22, 2011 at 12:06:46PM +0100, Jon Medhurst (Tixy) wrote:
quoted
On Thu, 2011-09-22 at 10:48 +0100, Catalin Marinas wrote:
quoted
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.
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 <catalin.marinas@arm.com>
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 <catalin.marinas@arm.com>
---
arch/arm/kernel/unwind.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
From: Jon Medhurst (Tixy) <hidden> Date: 2011-09-22 13:19:48
On Thu, 2011-09-22 at 14:00 +0100, Catalin Marinas wrote:
quoted hunk
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 <catalin.marinas@arm.com>
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 <catalin.marinas@arm.com>
---
arch/arm/kernel/unwind.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
@@ -293,6 +293,16 @@ int unwind_frame(struct stackframe *frame)return-URC_FAILURE;}+/*+*Checkforbacktraceonthefirstinstructionofafunction.The+*prologuehasnotbeenexecutedyetandtheunwindinginformationis+*notvalid.AssumethatthereturnaddressisinLR.+*/+if(idx.addr==frame->pc){+frame->pc=frame->lr;+returnURC_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 :-)
--
Tixy