public inbox for linuxppc-dev@ozlabs.org 
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm•com>
To: Yinan Liu <yinan@linux•alibaba.com>
Cc: linuxppc-dev@lists•ozlabs.org,
	Sachin Sant <sachinp@linux•ibm.com>,
	Steven Rostedt <rostedt@goodmis•org>
Subject: Re: [powerpc] ftrace warning kernel/trace/ftrace.c:2068 with code-patching selftests
Date: Wed, 26 Jan 2022 14:37:09 +0000	[thread overview]
Message-ID: <YfFclROd+0/61q2d@FVFF77S0Q05N> (raw)
In-Reply-To: <0fa0daec-881a-314b-e28b-3828e80bbd90@linux.alibaba.com>

Hi,

Steve pointed me at this thread over IRC -- I'm not subscribed to this list so
grabbed a copy of the thread thus far via b4.

On Tue, Jan 25, 2022 at 11:20:27AM +0800, Yinan Liu wrote:
> > Yeah, I think it's time to opt in, instead of opting out.

I agree this must be opt-in rather than opt-out.

However, I think most architectures were broken (in at least some
configurations) by commit:

  72b3942a173c387b ("scripts: ftrace - move the sort-processing in ftrace_init")

... and so I don't think this fix is correct as-is, and we might want to revert
that or at least mark is as BROKEN for now.

More on that below.

> > 
> > Something like this:
> > 
> > -- Steve
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index c2724d986fa0..5256ebe57451 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -82,6 +82,7 @@ config ARM
> >   	select HAVE_EBPF_JIT if !CPU_ENDIAN_BE32
> >   	select HAVE_CONTEXT_TRACKING
> >   	select HAVE_C_RECORDMCOUNT
> > +	select HAVE_BUILDTIME_MCOUNT_SORT
> >   	select HAVE_DEBUG_KMEMLEAK if !XIP_KERNEL
> >   	select HAVE_DMA_CONTIGUOUS if MMU
> >   	select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU

IIUC the 32-bit arm kernel can be relocated at boot time, so I don't believe
this is correct, and I believe any relocatable arm kernel has been broken since
htat was introduced.

> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index c4207cf9bb17..7996548b2b27 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -166,6 +166,7 @@ config ARM64
> >   	select HAVE_ASM_MODVERSIONS
> >   	select HAVE_EBPF_JIT
> >   	select HAVE_C_RECORDMCOUNT
> > +	select HAVE_BUILDTIME_MCOUNT_SORT
> >   	select HAVE_CMPXCHG_DOUBLE
> >   	select HAVE_CMPXCHG_LOCAL
> >   	select HAVE_CONTEXT_TRACKING

The arm64 kernel is relocatable by default, and has been broken since the
build-time sort was introduced -- I see ftrace test failures, and the
CONFIG_FTRACE_SORT_STARTUP_TEST screams at boot time.

> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 7399327d1eff..46080dea5dba 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -186,6 +186,7 @@ config X86
> >   	select HAVE_CONTEXT_TRACKING_OFFSTACK	if HAVE_CONTEXT_TRACKING
> >   	select HAVE_C_RECORDMCOUNT
> >   	select HAVE_OBJTOOL_MCOUNT		if STACK_VALIDATION
> > +	select HAVE_BUILDTIME_MCOUNT_SORT

Isn't x86 relocatable in some configurations (e.g. for KASLR)?

I can't see how the sort works for those cases, because the mcount_loc entries
are absolute, and either:

* The sorted entries will get overwritten by the unsorted relocation entries,
  and won't be sorted.

* The sorted entries won't get overwritten, but then the absolute address will
  be wrong since they hadn't been relocated.

How does that work?

Thanks,
Mark.

> >   	select HAVE_DEBUG_KMEMLEAK
> >   	select HAVE_DMA_CONTIGUOUS
> >   	select HAVE_DYNAMIC_FTRACE
> > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > index 752ed89a293b..7e5b92090faa 100644
> > --- a/kernel/trace/Kconfig
> > +++ b/kernel/trace/Kconfig
> > @@ -70,10 +70,16 @@ config HAVE_C_RECORDMCOUNT
> >   	help
> >   	  C version of recordmcount available?
> > +config HAVE_BUILDTIME_MCOUNT_SORT
> > +       bool
> > +       help
> > +         An architecture selects this if it sorts the mcount_loc section
> > +	 at build time.
> > +
> >   config BUILDTIME_MCOUNT_SORT
> >          bool
> >          default y
> > -       depends on BUILDTIME_TABLE_SORT && !S390
> > +       depends on HAVE_BUILDTIME_MCOUNT_SORT
> >          help
> >            Sort the mcount_loc section at build time.
> 
> LGTM. This will no longer destroy ftrace on other architectures.
> Those arches that we are not sure about can test and enable this function by
> themselves.
> 
> 
> Best regards
> --yinan
> 

  reply	other threads:[~2022-01-26 14:43 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-24  9:19 [powerpc] ftrace warning kernel/trace/ftrace.c:2068 with code-patching selftests Sachin Sant
2022-01-24 12:15 ` Yinan Liu
2022-01-24 16:45   ` Steven Rostedt
2022-01-25  3:20     ` Yinan Liu
2022-01-26 14:37       ` Mark Rutland [this message]
2022-01-27 11:46         ` Mark Rutland
2022-01-27 12:03           ` Ard Biesheuvel
2022-01-27 12:20             ` Mark Rutland
2022-01-27 12:22               ` Ard Biesheuvel
2022-01-27 12:59                 ` Mark Rutland
2022-01-27 13:07                   ` Ard Biesheuvel
2022-01-27 13:24                     ` Mark Rutland
2022-01-27 13:59                       ` Ard Biesheuvel
2022-01-27 14:54                         ` Mark Rutland
2022-01-27 15:01                           ` Ard Biesheuvel
2022-01-27 12:04           ` Sven Schnelle
2022-01-27 12:27             ` Mark Rutland
2022-01-27 12:46               ` Steven Rostedt
2022-01-27 13:08                 ` Mark Rutland
2022-01-27 13:16                   ` Sven Schnelle
2022-01-27 13:33                     ` Mark Rutland
2022-01-27 13:55                       ` Steven Rostedt
2022-01-27 14:56                         ` Mark Rutland
2022-01-27 16:41           ` Kees Cook
2022-01-25  4:00     ` Sachin Sant
2022-01-25 14:28       ` Steven Rostedt

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=YfFclROd+0/61q2d@FVFF77S0Q05N \
    --to=mark.rutland@arm$(echo .)com \
    --cc=linuxppc-dev@lists$(echo .)ozlabs.org \
    --cc=rostedt@goodmis$(echo .)org \
    --cc=sachinp@linux$(echo .)ibm.com \
    --cc=yinan@linux$(echo .)alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox