Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace
On 2/25/21 5:58 AM, Mark Rutland wrote:
> On Wed, Feb 24, 2021 at 01:34:09PM -0600, Madhavan T. Venkataraman wrote:
>> On 2/24/21 6:17 AM, Mark Rutland wrote:
>>> On Tue, Feb 23, 2021 at 12:12:43PM -0600, madve...@linux.microsoft.com
>>> wrote:
From: "Madhavan T. Venkataraman"
Termination
===
Currently, the unwinder terminates when both the FP (frame pointer)
and the PC (return address) of a frame are 0. But a frame could get
corrupted and zeroed. There needs to be a better check.
The following special terminating frame and function have been
defined for this purpose:
const u64arm64_last_frame[2] __attribute__ ((aligned (16)));
void arm64_last_func(void)
{
}
So, set the FP to arm64_last_frame and the PC to arm64_last_func in
the bottom most frame.
>>>
>>> My expectation was that we'd do this per-task, creating an empty frame
>>> record (i.e. with fp=NULL and lr=NULL) on the task's stack at the
>>> instant it was created, and chaining this into x29. That way the address
>>> is known (since it can be derived from the task), and the frame will
>>> also implicitly check that the callchain terminates on the task stack
>>> without loops. That also means that we can use it to detect the entry
>>> code going wrong (e.g. if the SP gets corrupted), since in that case the
>>> entry code would place the record at a different location.
>>
>> That is exactly what this is doing. arm64_last_frame[] is a marker frame
>> that contains fp=0 and pc=0.
>
> Almost! What I meant was that rather that each task should have its own
> final/marker frame record on its task task rather than sharing a common
> global variable.
>
> That way a check for that frame record implicitly checks that a task
> started at the expected location *on that stack*, which catches
> additional stack corruption cases (e.g. if we left data on the stack
> prior to an EL0 entry).
>
Ok. I will think about this.
> [...]
>
>>> ... I reckon once we've moved the last of the exception triage out to C
>>> this will be relatively simple, since all of the exception handlers will
>>> look like:
>>>
>>> | SYM_CODE_START_LOCAL(elX_exception)
>>> | kernel_entry X
>>> | mov x0, sp
>>> | bl elX_exception_handler
>>> | kernel_exit X
>>> | SYM_CODE_END(elX_exception)
>>>
>>> ... and so we just need to identify the set of elX_exception functions
>>> (which we'll never expect to take exceptions from directly). We could be
>>> strict and reject unwinding into arbitrary bits of the entry code (e.g.
>>> if we took an unexpected exception), and only permit unwinding to the
>>> BL.
>>>
It can do this if the FP and PC are also recorded elsewhere in the
pt_regs for comparison. Currently, the FP is also stored in
regs->regs[29]. The PC is stored in regs->pc. However, regs->pc can
be changed by lower level functions.
Create a new field, pt_regs->orig_pc, and record the return address
PC there. With this, the unwinder can validate the exception frame
and set a flag so that the caller of the unwinder can know when
an exception frame is encountered.
>>>
>>> I don't understand the case you're trying to solve here. When is
>>> regs->pc changed in a way that's problematic?
>>>
>>
>> For instance, I used a test driver in which the driver calls a function
>> pointer which is NULL. The low level fault handler sends a signal to the
>> task. Looks like it changes regs->pc for this.
>
> I'm struggling to follow what you mean here.
>
> If the kernel branches to NULL, the CPU should raise an instruction
> abort from the current EL, and our handling of that should terminate the
> thread via die_kernel_fault(), without returning to the faulting
> context, and without altering the PC in the faulting context.
>
> Signals are delivered to userspace and alter the userspace PC, not a
> kernel PC, so this doesn't seem relevant. Do you mean an exception fixup
> handler rather than a signal?
>
>> When I dump the stack from the low level handler, the comparison with
>> regs->pc does not work. But comparison with regs->orig_pc works.
>
> As above, I'm struggling with this; could you share a concrete example?
>
Actually, my bad. I needed the orig_pc because of something that my test
driver did. And, it slipped my mind entirely.
Thanks for pointing it out. I will fix this in my resend.
Thanks again for your comments.
I am currently studying probing/tracing. As soon as I have a solution for that,
I will send out the next version. I look forward to the in-depth review.
Thanks,
Madhavan
Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace
On Wed, Feb 24, 2021 at 01:34:09PM -0600, Madhavan T. Venkataraman wrote:
> On 2/24/21 6:17 AM, Mark Rutland wrote:
> > On Tue, Feb 23, 2021 at 12:12:43PM -0600, madve...@linux.microsoft.com
> > wrote:
> >> From: "Madhavan T. Venkataraman"
> >>Termination
> >>===
> >>
> >>Currently, the unwinder terminates when both the FP (frame pointer)
> >>and the PC (return address) of a frame are 0. But a frame could get
> >>corrupted and zeroed. There needs to be a better check.
> >>
> >>The following special terminating frame and function have been
> >>defined for this purpose:
> >>
> >>const u64arm64_last_frame[2] __attribute__ ((aligned (16)));
> >>
> >>void arm64_last_func(void)
> >>{
> >>}
> >>
> >>So, set the FP to arm64_last_frame and the PC to arm64_last_func in
> >>the bottom most frame.
> >
> > My expectation was that we'd do this per-task, creating an empty frame
> > record (i.e. with fp=NULL and lr=NULL) on the task's stack at the
> > instant it was created, and chaining this into x29. That way the address
> > is known (since it can be derived from the task), and the frame will
> > also implicitly check that the callchain terminates on the task stack
> > without loops. That also means that we can use it to detect the entry
> > code going wrong (e.g. if the SP gets corrupted), since in that case the
> > entry code would place the record at a different location.
>
> That is exactly what this is doing. arm64_last_frame[] is a marker frame
> that contains fp=0 and pc=0.
Almost! What I meant was that rather that each task should have its own
final/marker frame record on its task task rather than sharing a common
global variable.
That way a check for that frame record implicitly checks that a task
started at the expected location *on that stack*, which catches
additional stack corruption cases (e.g. if we left data on the stack
prior to an EL0 entry).
[...]
> > ... I reckon once we've moved the last of the exception triage out to C
> > this will be relatively simple, since all of the exception handlers will
> > look like:
> >
> > | SYM_CODE_START_LOCAL(elX_exception)
> > | kernel_entry X
> > | mov x0, sp
> > | bl elX_exception_handler
> > | kernel_exit X
> > | SYM_CODE_END(elX_exception)
> >
> > ... and so we just need to identify the set of elX_exception functions
> > (which we'll never expect to take exceptions from directly). We could be
> > strict and reject unwinding into arbitrary bits of the entry code (e.g.
> > if we took an unexpected exception), and only permit unwinding to the
> > BL.
> >
> >>It can do this if the FP and PC are also recorded elsewhere in the
> >>pt_regs for comparison. Currently, the FP is also stored in
> >>regs->regs[29]. The PC is stored in regs->pc. However, regs->pc can
> >>be changed by lower level functions.
> >>
> >>Create a new field, pt_regs->orig_pc, and record the return address
> >>PC there. With this, the unwinder can validate the exception frame
> >>and set a flag so that the caller of the unwinder can know when
> >>an exception frame is encountered.
> >
> > I don't understand the case you're trying to solve here. When is
> > regs->pc changed in a way that's problematic?
> >
>
> For instance, I used a test driver in which the driver calls a function
> pointer which is NULL. The low level fault handler sends a signal to the
> task. Looks like it changes regs->pc for this.
I'm struggling to follow what you mean here.
If the kernel branches to NULL, the CPU should raise an instruction
abort from the current EL, and our handling of that should terminate the
thread via die_kernel_fault(), without returning to the faulting
context, and without altering the PC in the faulting context.
Signals are delivered to userspace and alter the userspace PC, not a
kernel PC, so this doesn't seem relevant. Do you mean an exception fixup
handler rather than a signal?
> When I dump the stack from the low level handler, the comparison with
> regs->pc does not work. But comparison with regs->orig_pc works.
As above, I'm struggling with this; could you share a concrete example?
Thanks,
Mark.
Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace
On 2/24/21 6:17 AM, Mark Rutland wrote:
> Hi Madhavan,
>
> As Mark Brown says, I think this needs to be split into several
> patches. i have some comments on the general approach, but I'll save
> in-depth review until this has been split.
>
OK.
> On Tue, Feb 23, 2021 at 12:12:43PM -0600, madve...@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman"
>>
>> Unwinder changes
>>
>>
>> Termination
>> ===
>>
>> Currently, the unwinder terminates when both the FP (frame pointer)
>> and the PC (return address) of a frame are 0. But a frame could get
>> corrupted and zeroed. There needs to be a better check.
>>
>> The following special terminating frame and function have been
>> defined for this purpose:
>>
>> const u64arm64_last_frame[2] __attribute__ ((aligned (16)));
>>
>> void arm64_last_func(void)
>> {
>> }
>>
>> So, set the FP to arm64_last_frame and the PC to arm64_last_func in
>> the bottom most frame.
>
> My expectation was that we'd do this per-task, creating an empty frame
> record (i.e. with fp=NULL and lr=NULL) on the task's stack at the
> instant it was created, and chaining this into x29. That way the address
> is known (since it can be derived from the task), and the frame will
> also implicitly check that the callchain terminates on the task stack
> without loops. That also means that we can use it to detect the entry
> code going wrong (e.g. if the SP gets corrupted), since in that case the
> entry code would place the record at a different location.
>
That is exactly what this is doing. arm64_last_frame[] is a marker frame
that contains fp=0 and pc=0.
>>
>> Exception/Interrupt detection
>> =
>>
>> An EL1 exception renders the stack trace unreliable as it can happen
>> anywhere including the frame pointer prolog and epilog. The
>> unwinder needs to be able to detect the exception on the stack.
>>
>> Currently, the EL1 exception handler sets up pt_regs on the stack
>> and chains pt_regs->stackframe with the other frames on the stack.
>> But, the unwinder does not know where this exception frame is in
>> the stack trace.
>>
>> Set the LSB of the exception frame FP to allow the unwinder to
>> detect the exception frame. When the unwinder detects the frame,
>> it needs to make sure that it is really an exception frame and
>> not the result of any stack corruption.
>
> I'm not keen on messing with the encoding of the frame record as this
> will break external unwinders (e.g. using GDB on a kernel running under
> QEMU). I'd rather that we detected the exception boundary based on the
> LR, similar to what we did in commit:
>
OK. I will take a look at the commit you mentioned.
> 7326749801396105 ("arm64: unwind: reference pt_regs via embedded stack
> frame")
>
> ... I reckon once we've moved the last of the exception triage out to C
> this will be relatively simple, since all of the exception handlers will
> look like:
>
> | SYM_CODE_START_LOCAL(elX_exception)
> | kernel_entry X
> | mov x0, sp
> | bl elX_exception_handler
> | kernel_exit X
> | SYM_CODE_END(elX_exception)
>
> ... and so we just need to identify the set of elX_exception functions
> (which we'll never expect to take exceptions from directly). We could be
> strict and reject unwinding into arbitrary bits of the entry code (e.g.
> if we took an unexpected exception), and only permit unwinding to the
> BL.
>
>> It can do this if the FP and PC are also recorded elsewhere in the
>> pt_regs for comparison. Currently, the FP is also stored in
>> regs->regs[29]. The PC is stored in regs->pc. However, regs->pc can
>> be changed by lower level functions.
>>
>> Create a new field, pt_regs->orig_pc, and record the return address
>> PC there. With this, the unwinder can validate the exception frame
>> and set a flag so that the caller of the unwinder can know when
>> an exception frame is encountered.
>
> I don't understand the case you're trying to solve here. When is
> regs->pc changed in a way that's problematic?
>
For instance, I used a test driver in which the driver calls a function
pointer which is NULL. The low level fault handler sends a signal to the
task. Looks like it changes regs->pc for this. When I dump the stack
from the low level handler, the comparison with regs->pc does not work.
But comparison with regs->orig_pc works.
>> Unwinder return value
>> =
>>
>> Currently, the unwinder returns -EINVAL for stack trace termination
>> as well as stack trace error. Return -ENOENT for stack trace
>> termination and -EINVAL for error to disambiguate. This idea has
>> been borrowed from Mark Brown.
>
> IIRC Mark Brown already has a patch for this (and it could be queued on
> its own if it hasn't already been).
>
I
Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace
On 2/24/21 6:33 AM, Mark Brown wrote: > On Tue, Feb 23, 2021 at 01:20:49PM -0600, Madhavan T. Venkataraman wrote: >> On 2/23/21 1:02 PM, Mark Brown wrote: >>> On Tue, Feb 23, 2021 at 12:12:43PM -0600, madve...@linux.microsoft.com >>> wrote: > Reliable stack trace function = Implement arch_stack_walk_reliable(). This function walks the stack like the existing stack trace functions with a couple of additional checks: > >>> Again, this should be at least one separate patch. How does this ensure >>> that we don't have any issues with any of the various probe mechanisms? >>> If there's no need to explicitly check anything that should be called >>> out in the changelog. > >> I am trying to do this in an incremental fashion. I have to study the probe >> mechanisms a little bit more before I can come up with a solution. But >> if you want to see that addressed in this patch set, I could do that. >> It will take a little bit of time. That is all. > > Handling of the probes stuff seems like it's critical to reliable stack > walk so we shouldn't claim to have support for reliable stack walk > without it. If it was a working implementation we could improve that'd > be one thing but this would be buggy which is a different thing. > OK. I will address the probe stuff in my resend. + (void) on_accessible_stack(task, stackframe, ); > >>> Shouldn't we return NULL if we are not on an accessible stack? > >> The prev_fp has already been checked by the unwinder in the previous >> frame. That is why I don't check the return value. If that is acceptable, >> I will add a comment. > > TBH if you're adding the comment it seems like you may as well add the > check, it's not like it's expensive and it means there's no possibility > that some future change could result in this assumption being broken. > OK. I will add the check. Thanks. Madhavan
Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace
On Tue, Feb 23, 2021 at 01:20:49PM -0600, Madhavan T. Venkataraman wrote: > On 2/23/21 1:02 PM, Mark Brown wrote: > > On Tue, Feb 23, 2021 at 12:12:43PM -0600, madve...@linux.microsoft.com > > wrote: > >> Reliable stack trace function > >> = > >> > >> Implement arch_stack_walk_reliable(). This function walks the stack like > >> the existing stack trace functions with a couple of additional checks: > > Again, this should be at least one separate patch. How does this ensure > > that we don't have any issues with any of the various probe mechanisms? > > If there's no need to explicitly check anything that should be called > > out in the changelog. > I am trying to do this in an incremental fashion. I have to study the probe > mechanisms a little bit more before I can come up with a solution. But > if you want to see that addressed in this patch set, I could do that. > It will take a little bit of time. That is all. Handling of the probes stuff seems like it's critical to reliable stack walk so we shouldn't claim to have support for reliable stack walk without it. If it was a working implementation we could improve that'd be one thing but this would be buggy which is a different thing. > >> + (void) on_accessible_stack(task, stackframe, ); > > Shouldn't we return NULL if we are not on an accessible stack? > The prev_fp has already been checked by the unwinder in the previous > frame. That is why I don't check the return value. If that is acceptable, > I will add a comment. TBH if you're adding the comment it seems like you may as well add the check, it's not like it's expensive and it means there's no possibility that some future change could result in this assumption being broken. signature.asc Description: PGP signature
Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace
Hi Madhavan,
As Mark Brown says, I think this needs to be split into several
patches. i have some comments on the general approach, but I'll save
in-depth review until this has been split.
On Tue, Feb 23, 2021 at 12:12:43PM -0600, madve...@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman"
>
> Unwinder changes
>
>
> Termination
> ===
>
> Currently, the unwinder terminates when both the FP (frame pointer)
> and the PC (return address) of a frame are 0. But a frame could get
> corrupted and zeroed. There needs to be a better check.
>
> The following special terminating frame and function have been
> defined for this purpose:
>
> const u64arm64_last_frame[2] __attribute__ ((aligned (16)));
>
> void arm64_last_func(void)
> {
> }
>
> So, set the FP to arm64_last_frame and the PC to arm64_last_func in
> the bottom most frame.
My expectation was that we'd do this per-task, creating an empty frame
record (i.e. with fp=NULL and lr=NULL) on the task's stack at the
instant it was created, and chaining this into x29. That way the address
is known (since it can be derived from the task), and the frame will
also implicitly check that the callchain terminates on the task stack
without loops. That also means that we can use it to detect the entry
code going wrong (e.g. if the SP gets corrupted), since in that case the
entry code would place the record at a different location.
>
> Exception/Interrupt detection
> =
>
> An EL1 exception renders the stack trace unreliable as it can happen
> anywhere including the frame pointer prolog and epilog. The
> unwinder needs to be able to detect the exception on the stack.
>
> Currently, the EL1 exception handler sets up pt_regs on the stack
> and chains pt_regs->stackframe with the other frames on the stack.
> But, the unwinder does not know where this exception frame is in
> the stack trace.
>
> Set the LSB of the exception frame FP to allow the unwinder to
> detect the exception frame. When the unwinder detects the frame,
> it needs to make sure that it is really an exception frame and
> not the result of any stack corruption.
I'm not keen on messing with the encoding of the frame record as this
will break external unwinders (e.g. using GDB on a kernel running under
QEMU). I'd rather that we detected the exception boundary based on the
LR, similar to what we did in commit:
7326749801396105 ("arm64: unwind: reference pt_regs via embedded stack frame")
... I reckon once we've moved the last of the exception triage out to C
this will be relatively simple, since all of the exception handlers will
look like:
| SYM_CODE_START_LOCAL(elX_exception)
| kernel_entry X
| mov x0, sp
| bl elX_exception_handler
| kernel_exit X
| SYM_CODE_END(elX_exception)
... and so we just need to identify the set of elX_exception functions
(which we'll never expect to take exceptions from directly). We could be
strict and reject unwinding into arbitrary bits of the entry code (e.g.
if we took an unexpected exception), and only permit unwinding to the
BL.
> It can do this if the FP and PC are also recorded elsewhere in the
> pt_regs for comparison. Currently, the FP is also stored in
> regs->regs[29]. The PC is stored in regs->pc. However, regs->pc can
> be changed by lower level functions.
>
> Create a new field, pt_regs->orig_pc, and record the return address
> PC there. With this, the unwinder can validate the exception frame
> and set a flag so that the caller of the unwinder can know when
> an exception frame is encountered.
I don't understand the case you're trying to solve here. When is
regs->pc changed in a way that's problematic?
> Unwinder return value
> =
>
> Currently, the unwinder returns -EINVAL for stack trace termination
> as well as stack trace error. Return -ENOENT for stack trace
> termination and -EINVAL for error to disambiguate. This idea has
> been borrowed from Mark Brown.
IIRC Mark Brown already has a patch for this (and it could be queued on
its own if it hasn't already been).
Thanks,
Mark.
>
> Reliable stack trace function
> =
>
> Implement arch_stack_walk_reliable(). This function walks the stack like
> the existing stack trace functions with a couple of additional checks:
>
> Return address check
>
>
> For each frame, check the return address to see if it is a
> proper kernel text address. If not, return -EINVAL.
>
> Exception frame check
> -
>
> Check each frame to see if it is an EL1 exception frame. If it is,
> return -EINVAL.
>
> Signed-off-by: Madhavan T. Venkataraman
>
Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace
On 2/23/21 1:02 PM, Mark Brown wrote:
> On Tue, Feb 23, 2021 at 12:12:43PM -0600, madve...@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman"
>>
>> Unwinder changes
>>
>
> This is making several different changes so should be split into a patch
> series - for example the change to terminate on a specific function
> pointer rather than NULL and the changes to the exception/interupt
> detection should be split. Please see submitting-patches.rst for some
> discussion about how to split things up. In general if you've got a
> changelog enumerating a number of different changes in a patch that's a
> warning sign that it might be good split things up.
>
Will do.
> You should also copy the architecture maintainers (Catalin and Will) on
> any arch/arm64 submissions.
>
Will do when I resubmit.
>> Unwinder return value
>> =
>>
>> Currently, the unwinder returns -EINVAL for stack trace termination
>> as well as stack trace error. Return -ENOENT for stack trace
>> termination and -EINVAL for error to disambiguate. This idea has
>> been borrowed from Mark Brown.
>
> You could just include my patch for this in your series.
>
OK.
>> Reliable stack trace function
>> =
>>
>> Implement arch_stack_walk_reliable(). This function walks the stack like
>> the existing stack trace functions with a couple of additional checks:
>>
>> Return address check
>>
>>
>> For each frame, check the return address to see if it is a
>> proper kernel text address. If not, return -EINVAL.
>>
>> Exception frame check
>> -
>>
>> Check each frame to see if it is an EL1 exception frame. If it is,
>> return -EINVAL.
>
> Again, this should be at least one separate patch. How does this ensure
> that we don't have any issues with any of the various probe mechanisms?
> If there's no need to explicitly check anything that should be called
> out in the changelog.
>
I am trying to do this in an incremental fashion. I have to study the probe
mechanisms a little bit more before I can come up with a solution. But
if you want to see that addressed in this patch set, I could do that.
It will take a little bit of time. That is all.
> Since all these changes are mixed up this is a fairly superficial
> review of the actual code.
>
Understood. I will split things up and we can take it from there.
>> +static notrace struct pt_regs *get_frame_regs(struct task_struct *task,
>> + struct stackframe *frame)
>> +{
>> +unsigned long stackframe, regs_start, regs_end;
>> +struct stack_info info;
>> +
>> +stackframe = frame->prev_fp;
>> +if (!stackframe)
>> +return NULL;
>> +
>> +(void) on_accessible_stack(task, stackframe, );
>
> Shouldn't we return NULL if we are not on an accessible stack?
>
The prev_fp has already been checked by the unwinder in the previous
frame. That is why I don't check the return value. If that is acceptable,
I will add a comment.
>> +static notrace int update_frame(struct task_struct *task,
>> +struct stackframe *frame)
>
> This function really needs some documentation, the function is just
> called update_frame() which doesn't say what sort of updates it's
> supposed to do and most of the checks aren't explained, not all of them
> are super obvious.
>
I will add the documentation as well as try think of a better name.
>> +{
>> +unsigned long lsb = frame->fp & 0xf;
>> +unsigned long fp = frame->fp & ~lsb;
>> +unsigned long pc = frame->pc;
>> +struct pt_regs *regs;
>> +
>> +frame->exception_frame = false;
>> +
>> +if (fp == (unsigned long) arm64_last_frame &&
>> +pc == (unsigned long) arm64_last_func)
>> +return -ENOENT;
>> +
>> +if (!lsb)
>> +return 0;
>> +if (lsb != 1)
>> +return -EINVAL;
>> +
>> +/*
>> + * This looks like an EL1 exception frame.
>
> For clarity it would be good to spell out the properties of an EL1
> exception frame. It is not clear to me why we don't reference the frame
> type information the unwinder already records as part of these checks.
>
> In general, especially for the bits specific to reliable stack trace, I
> think we want to err on the side of verbosity here so that it is crystal
> clear what all the checks are supposed to be doing and it's that much
> easier to tie everything through to the requirements document.
OK. I will improve the documentation.
Madhavan
Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace
On Tue, Feb 23, 2021 at 12:12:43PM -0600, madve...@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman"
>
> Unwinder changes
>
This is making several different changes so should be split into a patch
series - for example the change to terminate on a specific function
pointer rather than NULL and the changes to the exception/interupt
detection should be split. Please see submitting-patches.rst for some
discussion about how to split things up. In general if you've got a
changelog enumerating a number of different changes in a patch that's a
warning sign that it might be good split things up.
You should also copy the architecture maintainers (Catalin and Will) on
any arch/arm64 submissions.
> Unwinder return value
> =
>
> Currently, the unwinder returns -EINVAL for stack trace termination
> as well as stack trace error. Return -ENOENT for stack trace
> termination and -EINVAL for error to disambiguate. This idea has
> been borrowed from Mark Brown.
You could just include my patch for this in your series.
> Reliable stack trace function
> =
>
> Implement arch_stack_walk_reliable(). This function walks the stack like
> the existing stack trace functions with a couple of additional checks:
>
> Return address check
>
>
> For each frame, check the return address to see if it is a
> proper kernel text address. If not, return -EINVAL.
>
> Exception frame check
> -
>
> Check each frame to see if it is an EL1 exception frame. If it is,
> return -EINVAL.
Again, this should be at least one separate patch. How does this ensure
that we don't have any issues with any of the various probe mechanisms?
If there's no need to explicitly check anything that should be called
out in the changelog.
Since all these changes are mixed up this is a fairly superficial
review of the actual code.
> +static notrace struct pt_regs *get_frame_regs(struct task_struct *task,
> + struct stackframe *frame)
> +{
> + unsigned long stackframe, regs_start, regs_end;
> + struct stack_info info;
> +
> + stackframe = frame->prev_fp;
> + if (!stackframe)
> + return NULL;
> +
> + (void) on_accessible_stack(task, stackframe, );
Shouldn't we return NULL if we are not on an accessible stack?
> +static notrace int update_frame(struct task_struct *task,
> + struct stackframe *frame)
This function really needs some documentation, the function is just
called update_frame() which doesn't say what sort of updates it's
supposed to do and most of the checks aren't explained, not all of them
are super obvious.
> +{
> + unsigned long lsb = frame->fp & 0xf;
> + unsigned long fp = frame->fp & ~lsb;
> + unsigned long pc = frame->pc;
> + struct pt_regs *regs;
> +
> + frame->exception_frame = false;
> +
> + if (fp == (unsigned long) arm64_last_frame &&
> + pc == (unsigned long) arm64_last_func)
> + return -ENOENT;
> +
> + if (!lsb)
> + return 0;
> + if (lsb != 1)
> + return -EINVAL;
> +
> + /*
> + * This looks like an EL1 exception frame.
For clarity it would be good to spell out the properties of an EL1
exception frame. It is not clear to me why we don't reference the frame
type information the unwinder already records as part of these checks.
In general, especially for the bits specific to reliable stack trace, I
think we want to err on the side of verbosity here so that it is crystal
clear what all the checks are supposed to be doing and it's that much
easier to tie everything through to the requirements document.
signature.asc
Description: PGP signature
