Re: [PATCH v2] arm64: ftrace: use function_nocfi for _mcount as well

Hi Mark, Sami,

On Tue, 5 Oct 2021 at 21:05, Mark Rutland [off-list ref] wrote:

On Tue, Oct 05, 2021 at 08:20:02AM -0700, Sami Tolvanen wrote:

quoted

Hi Sumit,

On Tue, Oct 5, 2021 at 5:37 AM Sumit Garg [off-list ref] wrote:

quoted

Commit 800618f955a9 ("arm64: ftrace: use function_nocfi for ftrace_call")
only fixed address of ftrace_call but address of _mcount needs to be
fixed as well. Use function_nocfi() to get the actual address of _mcount
function as with CONFIG_CFI_CLANG, the compiler replaces function pointers
with jump table addresses which breaks dynamic ftrace as the address of
_mcount is replaced with the address of _mcount.cfi_jt.

This problem won't apply where the toolchain implements
-fpatchable-function-entry as we'll use that in preference to regular -pg,
i.e. this won't show up with recent versions of clang.

Fixes: 9186ad8e66bab6a1 ("arm64: allow CONFIG_CFI_CLANG to be selected")
Signed-off-by: Sumit Garg <redacted>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---

Changes in v2:
- Added fixes tag.
- Extended commit description.
- Picked up Mark's ack.

 arch/arm64/include/asm/ftrace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 91fa4baa1a93..347b0cc68f07 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h

@@ -15,7 +15,7 @@
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 #define ARCH_SUPPORTS_FTRACE_OPS 1
 #else
-#define MCOUNT_ADDR            ((unsigned long)_mcount)
+#define MCOUNT_ADDR            ((unsigned long)function_nocfi(_mcount))
 #endif

 /* The BL at the callsite's adjusted rec->ip */
--

2.17.1

Clang >= 10 supports -fpatchable-function-entry and CFI requires Clang
12, so I assume this is only an issue if
CONFIG_DYNAMIC_FTRACE_WITH_REGS is explicitly disabled?

I don't believe it's possible to disable explicitly, since
DYNAMIC_FTRACE_WITH_REGS isn't user selectable, and is def bool y,
depending on HAVE_DYNAMIC_FTRACE_WITH_REGS.

Ah, I see.

Sumit, have you actually seen a problem, or was this found by
inspection?

Actually I have seen this ftrace problem with the android11-5.4-lts
kernel and AOSP master user-space on db845c. The reason being kernel
v5.4 LTS doesn't support ftrace with -fpatchable-function-entry on
arm64.

With the mainline, I haven't tried to reproduce this issue but it was
rather by inspection that this needs to be fixed as well.

If this isn't an issue in practice, we could add the funciton_nocfi()
for consistency, but we should make that clear in the commit message,
and drop the fixes tag.

Sure, let me drop the fixes tag and update the commit description in
v3 as mainline only enabled CFI_CLANG for arm64 when
"-fpatchable-function-entry" is supported.

-Sumit

Thanks,
Mark.