* [PATCH v2] arm64: ftrace: use function_nocfi for _mcount as well
@ 2021-10-05 12:36 Sumit Garg
2021-10-05 15:20 ` Sami Tolvanen
0 siblings, 1 reply; 6+ messages in thread
From: Sumit Garg @ 2021-10-05 12:36 UTC (permalink / raw)
To: linux-arm-kernel
Cc: rostedt, mingo, catalin.marinas, will, samitolvanen, ben.dai,
mark.rutland, nathan, keescook, ndesaulniers, daniel.thompson,
linux-kernel, llvm, Sumit Garg
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 <sumit.garg@linaro.org>
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: ftrace: use function_nocfi for _mcount as well
2021-10-05 12:36 [PATCH v2] arm64: ftrace: use function_nocfi for _mcount as well Sumit Garg
@ 2021-10-05 15:20 ` Sami Tolvanen
2021-10-05 15:35 ` Mark Rutland
0 siblings, 1 reply; 6+ messages in thread
From: Sami Tolvanen @ 2021-10-05 15:20 UTC (permalink / raw)
To: Sumit Garg
Cc: linux-arm-kernel, Steven Rostedt, Ingo Molnar, Catalin Marinas,
Will Deacon, ben.dai, Mark Rutland, Nathan Chancellor, Kees Cook,
Nick Desaulniers, daniel.thompson, LKML, llvm
Hi Sumit,
On Tue, Oct 5, 2021 at 5:37 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> 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 <sumit.garg@linaro.org>
> 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?
Nevertheless, the patch looks good to me.
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Sami
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: ftrace: use function_nocfi for _mcount as well
2021-10-05 15:20 ` Sami Tolvanen
@ 2021-10-05 15:35 ` Mark Rutland
2021-10-06 5:35 ` Sumit Garg
0 siblings, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2021-10-05 15:35 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Sumit Garg, linux-arm-kernel, Steven Rostedt, Ingo Molnar,
Catalin Marinas, Will Deacon, ben.dai, Nathan Chancellor,
Kees Cook, Nick Desaulniers, daniel.thompson, LKML, llvm
On Tue, Oct 05, 2021 at 08:20:02AM -0700, Sami Tolvanen wrote:
> Hi Sumit,
>
> On Tue, Oct 5, 2021 at 5:37 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > 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 <sumit.garg@linaro.org>
> > 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.
Sumit, have you actually seen a problem, or was this found by
inspection?
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.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: ftrace: use function_nocfi for _mcount as well
2021-10-05 15:35 ` Mark Rutland
@ 2021-10-06 5:35 ` Sumit Garg
2021-10-08 8:05 ` Will Deacon
0 siblings, 1 reply; 6+ messages in thread
From: Sumit Garg @ 2021-10-06 5:35 UTC (permalink / raw)
To: Mark Rutland, Sami Tolvanen
Cc: linux-arm-kernel, Steven Rostedt, Ingo Molnar, Catalin Marinas,
Will Deacon, ben.dai, Nathan Chancellor, Kees Cook,
Nick Desaulniers, Daniel Thompson, LKML, llvm
Hi Mark, Sami,
On Tue, 5 Oct 2021 at 21:05, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Oct 05, 2021 at 08:20:02AM -0700, Sami Tolvanen wrote:
> > Hi Sumit,
> >
> > On Tue, Oct 5, 2021 at 5:37 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > 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 <sumit.garg@linaro.org>
> > > 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: ftrace: use function_nocfi for _mcount as well
2021-10-06 5:35 ` Sumit Garg
@ 2021-10-08 8:05 ` Will Deacon
2021-10-11 12:53 ` Sumit Garg
0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2021-10-08 8:05 UTC (permalink / raw)
To: Sumit Garg
Cc: Mark Rutland, Sami Tolvanen, linux-arm-kernel, Steven Rostedt,
Ingo Molnar, Catalin Marinas, ben.dai, Nathan Chancellor,
Kees Cook, Nick Desaulniers, Daniel Thompson, LKML, llvm
Hi Sumit,
On Wed, Oct 06, 2021 at 11:05:52AM +0530, Sumit Garg wrote:
> On Tue, 5 Oct 2021 at 21:05, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Oct 05, 2021 at 08:20:02AM -0700, Sami Tolvanen wrote:
> > > On Tue, Oct 5, 2021 at 5:37 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > 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.
Did you post a v3? Just want to make sure I didn't miss it.
Will
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: ftrace: use function_nocfi for _mcount as well
2021-10-08 8:05 ` Will Deacon
@ 2021-10-11 12:53 ` Sumit Garg
0 siblings, 0 replies; 6+ messages in thread
From: Sumit Garg @ 2021-10-11 12:53 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Sami Tolvanen, linux-arm-kernel, Steven Rostedt,
Ingo Molnar, Catalin Marinas, ben.dai, Nathan Chancellor,
Kees Cook, Nick Desaulniers, Daniel Thompson, LKML, llvm
Hi Will,
On Fri, 8 Oct 2021 at 13:35, Will Deacon <will@kernel.org> wrote:
>
> Hi Sumit,
>
> On Wed, Oct 06, 2021 at 11:05:52AM +0530, Sumit Garg wrote:
> > On Tue, 5 Oct 2021 at 21:05, Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Tue, Oct 05, 2021 at 08:20:02AM -0700, Sami Tolvanen wrote:
> > > > On Tue, Oct 5, 2021 at 5:37 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > > 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.
>
> Did you post a v3? Just want to make sure I didn't miss it.
>
Apologies for the delay, here [1] it is.
[1] https://lkml.org/lkml/2021/10/11/485
-Sumit
> Will
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-11 12:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-05 12:36 [PATCH v2] arm64: ftrace: use function_nocfi for _mcount as well Sumit Garg
2021-10-05 15:20 ` Sami Tolvanen
2021-10-05 15:35 ` Mark Rutland
2021-10-06 5:35 ` Sumit Garg
2021-10-08 8:05 ` Will Deacon
2021-10-11 12:53 ` Sumit Garg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).