On Mon, 5 Jun 2017 13:16:18 +0200
Marcin Nowakowski <marcin.nowakowski@xxxxxxxxxx> wrote:
> Hi Steven,
>
> On 23.05.2017 21:32, Steven Rostedt wrote:
> > On Tue, 23 May 2017 12:56:43 +0200
> > Marcin Nowakowski <marcin.nowakowski@xxxxxxxxxx> wrote:
> >
> >> Since introduction of tracing for init functions the in_kernel_space()
> >> check is no longer correct, as it ignores the init sections. As a
> >> result, when probes are inserted (and disabled) in the init functions,
> >> a branch instruction is inserted instead of a nop, which is likely to
> >> result in random crashes during boot.
> >>
> >> Remove the MIPS-specific in_kernel_space() method and replace it with
> >> a generic core_kernel_text() that also checks for init sections during
> >> system boot stage.
> >>
> >> Fixes: 42c269c88dc1 ("ftrace: Allow for function tracing to record
> >> init functions on boot up") Signed-off-by: Marcin Nowakowski
> >> <marcin.nowakowski@xxxxxxxxxx> ---
> >> arch/mips/kernel/ftrace.c | 24 +++++-------------------
> >> 1 file changed, 5 insertions(+), 19 deletions(-)
> >>
> >>
> >
> > Thanks for doing this.
>
> There's one issue with this patch that I haven't noticed initially -
> while this fixes the MIPS boot issue, it introduces a problem due to use
> of core_kernel_text in the function graph trace path ... as the
> core_kernel_text function is traced as well, we end up with a recursive
> call to core_kernel_text from its function graph handler, something that
> shows up in a backtrace as:
>
> [ 2.972075] Call Trace:
> [ 2.972111]
> [ 2.976731] [<80506584>] ftrace_return_to_handler+0x50/0x128
> [ 2.983379] [<8045478c>] core_kernel_text+0x10/0x1b8
> [ 2.989146] [<804119b8>] prepare_ftrace_return+0x6c/0x114
> [ 2.995402] [<80411b2c>] ftrace_graph_caller+0x20/0x44
> [ 3.001362] [<80411b60>] return_to_handler+0x10/0x30
> [ 3.007159] [<80411b50>] return_to_handler+0x0/0x30
> [ 3.012827] [<80411b50>] return_to_handler+0x0/0x30
> [ 3.018621] [<804e589c>] ftrace_ops_no_ops+0x114/0x1bc
> [ 3.024602] [<8045478c>] core_kernel_text+0x10/0x1b8
> [ 3.030377] [<8045478c>] core_kernel_text+0x10/0x1b8
> [ 3.036140] [<8045478c>] core_kernel_text+0x10/0x1b8
> [ 3.041915] [<804e589c>] ftrace_ops_no_ops+0x114/0x1bc
> [ 3.047923] [<8045478c>] core_kernel_text+0x10/0x1b8
> [ 3.053682] [<804119b8>] prepare_ftrace_return+0x6c/0x114
> [ 3.059938] [<80411b2c>] ftrace_graph_caller+0x20/0x44
> (and many many more repetitions before we run out of stack)
>
> Would a simple patch like:
>
> --- a/kernel/extable.c
> +++ b/kernel/extable.c
> @@ -69,7 +69,7 @@ static inline int init_kernel_text(unsigned long addr)
> return 0;
> }
>
> -int core_kernel_text(unsigned long addr)
> +int notrace core_kernel_text(unsigned long addr)
> {
> if (addr >= (unsigned long)_stext &&
> addr < (unsigned long)_etext)
>
>
> be acceptable here to solve this problem? It needs to modify the generic
> kernel code due to a dependency that is specific to MIPS only, so it
> might not be the best solution, but an alternative is to add restore a
> copy of that method in arch/mips - which doesn't seem very nice either.
> The core_kernel_text function is not something that is extremely
> valuable to be traced IMO anyway, so it shouldn't be a big loss.
>
> What's your opinion?
> If this change is OK I'll send out a patch
>
I somehow missed this email :-/
Anyway, I already replied to the patch that made the change.
-- Steve
