Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
To: Mark Brown <broonie@kernel.org>
Cc: mark.rutland@arm.com, jpoimboe@redhat.com, jthierry@redhat.com,
	catalin.marinas@arm.com, will@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable
Date: Thu, 1 Apr 2021 13:40:23 -0500	[thread overview]
Message-ID: <2a56fe4b-9929-0d8b-aa49-c2b1c1b82b79@linux.microsoft.com> (raw)
In-Reply-To: <20210401182810.GO4758@sirena.org.uk>

On 4/1/21 1:28 PM, Mark Brown wrote:
> On Thu, Apr 01, 2021 at 12:43:25PM -0500, Madhavan T. Venkataraman wrote:
> 
>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>>>> +	{ (unsigned long) &ftrace_graph_call, 0 },
>>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>>> +	{ (unsigned long) ftrace_graph_caller, 0 },
> 
>>> It's weird that we take the address of ftrace_graph_call but not the
>>> other functions - we should be consistent or explain why.  It'd probably
>>> also look nicer to not nest the ifdefs, the dependencies in Kconfig will
>>> ensure we only get things when we should.
> 
>> I have explained it in the comment in the FTRACE trampoline right above
>> ftrace_graph_call().
> 
> Ah, right - it's a result of it being an inner label.  I'd suggest
> putting a brief note right at that line of code explaining this (eg,
> "Inner label, not a function"), it wasn't confusing due to the use of
> that symbol but rather due to it being different from everything else
> in the list and that's kind of lost in the main comment.
> 

OK, So, I will add a note in the main comment above the list. I will add the
comment line you have suggested at the exact line.

>> So, it is only defined if CONFIG_FUNCTION_GRAPH_TRACER is defined. I can address
>> this as well as your comment by defining another label whose name is more meaningful
>> to our use:
> 
>> +SYM_INNER_LABEL(ftrace_trampoline, SYM_L_GLOBAL) // checked by the unwinder
>> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
>>         nop                             // If enabled, this will be replaced
>>                                         // "b ftrace_graph_caller"
>> #endif
> 
> I'm not sure we need to bother with that, you'd still need the & I think.

I think we need to bother with that. If CONFIG_FUNCTION_GRAPH_TRACER is not on but
CONFIG_DYNAMIC_FTRACE_WITH_REGS is, then ftrace_graph_call() will not occur in the stack
trace taken from a tracer function. The unwinder still needs to recognize an ftrace frame.
I don't want to assume ftrace_common_return which is the label that currently follows
the above code. So, we need a different label outside the above ifdef.

As for the &, I needed it because ftrace_graph_call is currently defined elsewhere as:

extern unsigned long ftrace_graph_call;

I did not want to change that.

Thanks,

Madhavan