From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] arm64: fix unwind_frame() for filtered out fn for function graph tracing
Date: Wed, 13 Sep 2017 03:42:19 +0100 [thread overview]
Message-ID: <20170913024218.GB12411@arm.com> (raw)
In-Reply-To: <59B7AED4.1070106@arm.com>
On Tue, Sep 12, 2017 at 10:54:28AM +0100, James Morse wrote:
> Hi Pratyush,
>
> On 01/09/17 06:48, Pratyush Anand wrote:
> > do_task_stat() calls get_wchan(), which further does unbind_frame().
> > unbind_frame() restores frame->pc to original value in case function
> > graph tracer has modified a return address (LR) in a stack frame to hook
> > a function return. However, if function graph tracer has hit a filtered
> > function, then we can't unwind it as ftrace_push_return_trace() has
> > biased the index(frame->graph) with a 'huge negative'
> > offset(-FTRACE_NOTRACE_DEPTH).
> >
> > Moreover, arm64 stack walker defines index(frame->graph) as unsigned
> > int, which can not compare a -ve number.
> >
> > Similar problem we can have with calling of walk_stackframe() from
> > save_stack_trace_tsk() or dump_backtrace().
> >
> > This patch fixes unwind_frame() to test the index for -ve value and
> > restore index accordingly before we can restore frame->pc.
>
> I've just spotted arm64's profile_pc, which does this:
> From arch/arm64/kernel/time.c:profile_pc():
> > #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > frame.graph = -1; /* no task info */
> > #endif
>
> Is this another elaborate way of hitting this problem?
>
> I guess the options are skip any return-address restore in the unwinder if
> frame.graph is -1. (and profile_pc may have a bug here). Or, put
> current->curr_ret_stack in there.
>
> profile_pc() always passes tsk=NULL, so the unwinder assumes its current...
> kernel/profile.c pulls the pt_regs from a per-cpu irq_regs variable, that is
> updated by handle_IPI ... so it looks like this should always be current...
Hmmm... is profile_pc the *only* case where frame->graph isn't equal to
tsk->curr_ret_stack in unwind_frame? If so, maybe unwind_frame should just
use that, and we could kill the graph member of struct stackframe completely?
Will
WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon@arm.com>
To: James Morse <james.morse@arm.com>
Cc: Pratyush Anand <panand@redhat.com>,
linux-arm-kernel@lists.infradead.org,
Catalin Marinas <catalin.marinas@arm.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] arm64: fix unwind_frame() for filtered out fn for function graph tracing
Date: Wed, 13 Sep 2017 03:42:19 +0100 [thread overview]
Message-ID: <20170913024218.GB12411@arm.com> (raw)
In-Reply-To: <59B7AED4.1070106@arm.com>
On Tue, Sep 12, 2017 at 10:54:28AM +0100, James Morse wrote:
> Hi Pratyush,
>
> On 01/09/17 06:48, Pratyush Anand wrote:
> > do_task_stat() calls get_wchan(), which further does unbind_frame().
> > unbind_frame() restores frame->pc to original value in case function
> > graph tracer has modified a return address (LR) in a stack frame to hook
> > a function return. However, if function graph tracer has hit a filtered
> > function, then we can't unwind it as ftrace_push_return_trace() has
> > biased the index(frame->graph) with a 'huge negative'
> > offset(-FTRACE_NOTRACE_DEPTH).
> >
> > Moreover, arm64 stack walker defines index(frame->graph) as unsigned
> > int, which can not compare a -ve number.
> >
> > Similar problem we can have with calling of walk_stackframe() from
> > save_stack_trace_tsk() or dump_backtrace().
> >
> > This patch fixes unwind_frame() to test the index for -ve value and
> > restore index accordingly before we can restore frame->pc.
>
> I've just spotted arm64's profile_pc, which does this:
> From arch/arm64/kernel/time.c:profile_pc():
> > #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > frame.graph = -1; /* no task info */
> > #endif
>
> Is this another elaborate way of hitting this problem?
>
> I guess the options are skip any return-address restore in the unwinder if
> frame.graph is -1. (and profile_pc may have a bug here). Or, put
> current->curr_ret_stack in there.
>
> profile_pc() always passes tsk=NULL, so the unwinder assumes its current...
> kernel/profile.c pulls the pt_regs from a per-cpu irq_regs variable, that is
> updated by handle_IPI ... so it looks like this should always be current...
Hmmm... is profile_pc the *only* case where frame->graph isn't equal to
tsk->curr_ret_stack in unwind_frame? If so, maybe unwind_frame should just
use that, and we could kill the graph member of struct stackframe completely?
Will
next prev parent reply other threads:[~2017-09-13 2:42 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-01 5:48 [PATCH v2] arm64: fix unwind_frame() for filtered out fn for function graph tracing Pratyush Anand
2017-09-01 5:48 ` Pratyush Anand
2017-09-12 9:54 ` James Morse
2017-09-12 9:54 ` James Morse
2017-09-13 2:42 ` Will Deacon [this message]
2017-09-13 2:42 ` Will Deacon
2017-09-13 4:59 ` Pratyush Anand
2017-09-13 4:59 ` Pratyush Anand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170913024218.GB12411@arm.com \
--to=will.deacon@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.