[PATCH] ftrace: Do not reference symbols in sections without size

[PATCH] ftrace: Do not reference symbols in sections without size

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Starting with binutils 2.36, sections were being removed if they had weak
functions that were optimized out. Unfortunately, these weak functions would
leave references to mcount/fentry calls, that would make recordmcount fail
to find the symbol that matched the call to fentry.

Before returning the symbol of the section to create the mcount location,
check if that section size is greater than zero. If it has no size, skip
referencing that mcount call location.

Link: https://lore.kernel.org/lkml/YCafKVSTX9MxDBMd@kroah.com/

Cc: stable@vger.kernel.org
[ Backport as far as it can go ]
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 scripts/recordmcount.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index f9b19524da11..1a29f290092d 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -562,7 +562,8 @@ static char const * __has_rel_mcount(Elf_Shdr const *const relhdr, /* reltype */
 	if (w(txthdr->sh_type) != SHT_PROGBITS ||
 	    !(_w(txthdr->sh_flags) & SHF_EXECINSTR))
 		return NULL;
-	return txtname;
+	/* If the section has no size, then it wont be available for reference */
+	return shdr0->sh_size ? txtname : NULL;
 }
 
 static char const *has_rel_mcount(Elf_Shdr const *const relhdr,
-- 
2.25.4

Re: [PATCH] ftrace: Do not reference symbols in sections without size

[Josh Poimboeuf]

On Mon, Feb 15, 2021 at 04:44:46PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Starting with binutils 2.36, sections were being removed if they had weak
> functions that were optimized out. Unfortunately, these weak functions would
> leave references to mcount/fentry calls, that would make recordmcount fail
> to find the symbol that matched the call to fentry.

Binutils 2.36 isn't removing sections, just section *symbols*.

> Before returning the symbol of the section to create the mcount location,
> check if that section size is greater than zero. If it has no size, skip
> referencing that mcount call location.

How does this even work?  The .text.unlikely section isn't empty:

  # readelf -SW kernel/kexec_file.o
  
  Section Headers:
    [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
    [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
    [ 1] .text             PROGBITS        0000000000000000 000040 001acf 00  AX  0   0  1
    [ 2] .rela.text        RELA            0000000000000000 0049e0 001b30 18   I 27   1  8
    [ 3] .data             PROGBITS        0000000000000000 001b20 000e80 00  WA  0   0 32
    [ 4] .rela.data        RELA            0000000000000000 006510 0010e0 18   I 27   3  8
    [ 5] .bss              NOBITS          0000000000000000 0029a0 000060 00  WA  0   0 32
    [ 6] .rodata           PROGBITS        0000000000000000 0029a0 000b80 00   A  0   0 32
    [ 7] .rodata.str1.1    PROGBITS        0000000000000000 003520 0001c6 01 AMS  0   0  1
    [ 8] .text.unlikely    PROGBITS        0000000000000000 0036e6 000038 00  AX  0   0  1

-- 
Josh

Re: [PATCH] ftrace: Do not reference symbols in sections without size

[Steven Rostedt]

On Mon, 15 Feb 2021 18:05:04 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Mon, Feb 15, 2021 at 04:44:46PM -0500, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > Starting with binutils 2.36, sections were being removed if they had weak
> > functions that were optimized out. Unfortunately, these weak functions would
> > leave references to mcount/fentry calls, that would make recordmcount fail
> > to find the symbol that matched the call to fentry.  
> 
> Binutils 2.36 isn't removing sections, just section *symbols*.
> 
> > Before returning the symbol of the section to create the mcount location,
> > check if that section size is greater than zero. If it has no size, skip
> > referencing that mcount call location.  
> 
> How does this even work?  The .text.unlikely section isn't empty:
> 
>   # readelf -SW kernel/kexec_file.o

Bah, because I messed up, and realized I was using the first section (shdr0)
and not the one it was being used for.

Strange, that when I applied this to the latest kernel on a my build
system (binutils 2.35), it still created all the necessary mcount
locations??

I'll look more into this.

Thanks for pointing this out.

-- Steve



>   
>   Section Headers:
>     [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
>     [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
>     [ 1] .text             PROGBITS        0000000000000000 000040 001acf 00  AX  0   0  1
>     [ 2] .rela.text        RELA            0000000000000000 0049e0 001b30 18   I 27   1  8
>     [ 3] .data             PROGBITS        0000000000000000 001b20 000e80 00  WA  0   0 32
>     [ 4] .rela.data        RELA            0000000000000000 006510 0010e0 18   I 27   3  8
>     [ 5] .bss              NOBITS          0000000000000000 0029a0 000060 00  WA  0   0 32
>     [ 6] .rodata           PROGBITS        0000000000000000 0029a0 000b80 00   A  0   0 32
>     [ 7] .rodata.str1.1    PROGBITS        0000000000000000 003520 0001c6 01 AMS  0   0  1
>     [ 8] .text.unlikely    PROGBITS        0000000000000000 0036e6 000038 00  AX  0   0  1
> 

Re: [PATCH] ftrace: Do not reference symbols in sections without size

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

On Mon, 15 Feb 2021 20:06:39 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> Strange, that when I applied this to the latest kernel on a my build
> system (binutils 2.35), it still created all the necessary mcount
> locations??

I know why it worked. If you are using the latest gcc on the latest
mainline, it will not even bother with recordmcount, and will just
create the __mcount_loc sections, as latest gcc knows about ftrace.

(this is what I get for working on a company holiday)

Since this is a toolchain issue, perhaps the correct thing to do is to
backport to stable the changes to have it build with -mrecord-mcount if
the build system enables it.

If you are using the lastest compilers to build stable releases, and
that's causing issues, then you should have the stable releases use the
latest kernel compiler options.

Greg,

Can you test the following two backports. It does change the semantics
of what is built, but then again if you are using a newer compiler to
build stable kernels, that can change things too.

96f60dfa5819a ("trace: Use -mcount-record for dynamic ftrace")
07d0408120216 ("tracing: Avoid calling cc-option -mrecord-mcount for every Makefile")

I attached the backports to 4.4. (just compiled tested, I'll test them more tomorrow)

Thanks!

-- Steve

[-- Attachment #2: 0001-trace-Use-mcount-record-for-dynamic-ftrace.patch --]
[-- Type: text/x-patch, Size: 1596 bytes --]

From f9a03bb58aa222824b3041efbf62488af693feaa Mon Sep 17 00:00:00 2001
From: Andi Kleen <ak@linux.intel.com>
Date: Mon, 27 Nov 2017 13:34:13 -0800
Subject: [PATCH 1/2] trace: Use -mcount-record for dynamic ftrace

gcc 5 supports a new -mcount-record option to generate ftrace
tables directly. This avoids the need to run record_mcount
manually.

Use this option when available.

So far doesn't use -mcount-nop, which also exists now.

This is needed to make ftrace work with LTO because the
normal record-mcount script doesn't run over the link
time output.

It should also improve build times slightly in the general
case.
Link: http://lkml.kernel.org/r/20171127213423.27218-12-andi@firstfloor.org

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 scripts/Makefile.build | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 42aef001dfdd..fff1452cb76e 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -221,6 +221,11 @@ cmd_modversions_c =								\
 endif
 
 ifdef CONFIG_FTRACE_MCOUNT_RECORD
+# gcc 5 supports generating the mcount tables directly
+ifneq ($(call cc-option,-mrecord-mcount,y),y)
+KBUILD_CFLAGS += -mrecord-mcount
+else
+# else do it all manually
 ifdef BUILD_C_RECORDMCOUNT
 ifeq ("$(origin RECORDMCOUNT_WARN)", "command line")
   RECORDMCOUNT_FLAGS = -w
@@ -250,6 +255,7 @@ cmd_record_mcount =						\
 		$(sub_cmd_record_mcount)			\
 	fi;
 endif
+endif
 
 define rule_cc_o_c
 	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \
-- 
2.30.1


[-- Attachment #3: 0002-tracing-Avoid-calling-cc-option-mrecord-mcount-for-e.patch --]
[-- Type: text/x-patch, Size: 2340 bytes --]

From 76a3bbb0b0b193ac4dcd3c005cc6abcaddaadfe6 Mon Sep 17 00:00:00 2001
From: Vasily Gorbik <gor@linux.ibm.com>
Date: Mon, 6 Aug 2018 15:17:44 +0200
Subject: [PATCH 2/2] tracing: Avoid calling cc-option -mrecord-mcount for
 every Makefile

Currently if CONFIG_FTRACE_MCOUNT_RECORD is enabled -mrecord-mcount
compiler flag support is tested for every Makefile.

Top 4 cc-option usages:
    511 -mrecord-mcount
     11  -fno-stack-protector
      9 -Wno-override-init
      2 -fsched-pressure

To address that move cc-option from scripts/Makefile.build to top Makefile
and export CC_USING_RECORD_MCOUNT to be used in original place.

While doing that also add -mrecord-mcount to CC_FLAGS_FTRACE (if gcc
actually supports it).

Link: http://lkml.kernel.org/r/patch-2.thread-aa7b8d.git-de935bace15a.your-ad-here.call-01533557518-ext-9465@work.hours

Acked-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 Makefile               | 7 +++++++
 scripts/Makefile.build | 7 ++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 0057587d2cbe..9f2123555429 100644
--- a/Makefile
+++ b/Makefile
@@ -760,6 +760,13 @@ ifdef CONFIG_FUNCTION_TRACER
 ifndef CC_FLAGS_FTRACE
 CC_FLAGS_FTRACE := -pg
 endif
+ifdef CONFIG_FTRACE_MCOUNT_RECORD
+  # gcc 5 supports generating the mcount tables directly
+  ifeq ($(call cc-option-yn,-mrecord-mcount),y)
+    CC_FLAGS_FTRACE	+= -mrecord-mcount
+    export CC_USING_RECORD_MCOUNT := 1
+  endif
+endif
 export CC_FLAGS_FTRACE
 ifdef CONFIG_HAVE_FENTRY
 CC_USING_FENTRY	:= $(call cc-option, -mfentry -DCC_USING_FENTRY)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index fff1452cb76e..673ab54c0af6 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -221,11 +221,8 @@ cmd_modversions_c =								\
 endif
 
 ifdef CONFIG_FTRACE_MCOUNT_RECORD
-# gcc 5 supports generating the mcount tables directly
-ifneq ($(call cc-option,-mrecord-mcount,y),y)
-KBUILD_CFLAGS += -mrecord-mcount
-else
-# else do it all manually
+ifndef CC_USING_RECORD_MCOUNT
+# compiler will not generate __mcount_loc use recordmcount or recordmcount.pl
 ifdef BUILD_C_RECORDMCOUNT
 ifeq ("$(origin RECORDMCOUNT_WARN)", "command line")
   RECORDMCOUNT_FLAGS = -w
-- 
2.30.1

Re: [PATCH] ftrace: Do not reference symbols in sections without size

[Greg Kroah-Hartman]

On Mon, Feb 15, 2021 at 09:00:57PM -0500, Steven Rostedt wrote:
> On Mon, 15 Feb 2021 20:06:39 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Strange, that when I applied this to the latest kernel on a my build
> > system (binutils 2.35), it still created all the necessary mcount
> > locations??
> 
> I know why it worked. If you are using the latest gcc on the latest
> mainline, it will not even bother with recordmcount, and will just
> create the __mcount_loc sections, as latest gcc knows about ftrace.
> 
> (this is what I get for working on a company holiday)
> 
> Since this is a toolchain issue, perhaps the correct thing to do is to
> backport to stable the changes to have it build with -mrecord-mcount if
> the build system enables it.
> 
> If you are using the lastest compilers to build stable releases, and
> that's causing issues, then you should have the stable releases use the
> latest kernel compiler options.
> 
> Greg,
> 
> Can you test the following two backports. It does change the semantics
> of what is built, but then again if you are using a newer compiler to
> build stable kernels, that can change things too.
> 
> 96f60dfa5819a ("trace: Use -mcount-record for dynamic ftrace")
> 07d0408120216 ("tracing: Avoid calling cc-option -mrecord-mcount for every Makefile")
> 
> I attached the backports to 4.4. (just compiled tested, I'll test them more tomorrow)
> 

Yes, they build here for me!

Thanks for this.

Should I also queue these up for 4.9 and 4.14 which do not have these
commits in them either (but somehow do not show the problem, yet)?

thanks,

greg k-h

Re: [PATCH] ftrace: Do not reference symbols in sections without size

[Steven Rostedt]

On Tue, 16 Feb 2021 12:04:06 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> Thanks for this.
> 
> Should I also queue these up for 4.9 and 4.14 which do not have these
> commits in them either (but somehow do not show the problem, yet)?

This bothers me. I want to know exactly why this is a problem.

That said, it is fine to backport those patches, and I would include 4.9
and 4.14, as I would think you have a similar requirement that we have in
the stable-rt trees. That is you shouldn't experience a regression going
from an older kernel to a newer one because the older one had a fix
backported to it that a newer one did not. Basically the same rationale that
all fixes go into Linus's tree before backporting. We do the same on the
stable-rt, where all fixes go in all maintained stable trees that are newer
than the one you are backporting to.

Although, it does allow more to be traced than what recordmcount enables.
But hopefully it doesn't cause any issues. Maybe I should do some ftrace
testing before you go and release any of those stables with those patches.

I'm looking to see if this new "feature" of binutils isn't causing trouble
elsewhere. I'm thinking that ftrace is just the canary here.

-- Steve

Re: [PATCH] ftrace: Do not reference symbols in sections without size

[Josh Poimboeuf]

On Tue, Feb 16, 2021 at 09:51:21AM -0500, Steven Rostedt wrote:
> On Tue, 16 Feb 2021 12:04:06 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > Thanks for this.
> > 
> > Should I also queue these up for 4.9 and 4.14 which do not have these
> > commits in them either (but somehow do not show the problem, yet)?
>
> This bothers me. I want to know exactly why this is a problem.

I actually see the same problem with 4.9 and 4.14, using the same
config.

It's very config-specific.  Something has to convince the toolchain to
not reference those two weak functions by section.

> That said, it is fine to backport those patches, and I would include 4.9
> and 4.14, as I would think you have a similar requirement that we have in
> the stable-rt trees. That is you shouldn't experience a regression going
> from an older kernel to a newer one because the older one had a fix
> backported to it that a newer one did not. Basically the same rationale that
> all fixes go into Linus's tree before backporting. We do the same on the
> stable-rt, where all fixes go in all maintained stable trees that are newer
> than the one you are backporting to.
> 
> Although, it does allow more to be traced than what recordmcount enables.
> But hopefully it doesn't cause any issues. Maybe I should do some ftrace
> testing before you go and release any of those stables with those patches.
> 
> I'm looking to see if this new "feature" of binutils isn't causing trouble
> elsewhere. I'm thinking that ftrace is just the canary here.

It already caused quite a bit of trouble with objtool (as did a previous
similar change by the Clang assembler).

-- 
Josh

Re: [PATCH] ftrace: Do not reference symbols in sections without size

[Greg Kroah-Hartman]

On Tue, Feb 16, 2021 at 09:45:39AM -0600, Josh Poimboeuf wrote:
> On Tue, Feb 16, 2021 at 09:51:21AM -0500, Steven Rostedt wrote:
> > On Tue, 16 Feb 2021 12:04:06 +0100
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > > Thanks for this.
> > > 
> > > Should I also queue these up for 4.9 and 4.14 which do not have these
> > > commits in them either (but somehow do not show the problem, yet)?
> >
> > This bothers me. I want to know exactly why this is a problem.
> 
> I actually see the same problem with 4.9 and 4.14, using the same
> config.

Ok, that's good to know.

> It's very config-specific.  Something has to convince the toolchain to
> not reference those two weak functions by section.

I've queued up all 3 patches (Steven collapsed 2 into 1 it seems) for
4.4.y, 4.9.y, and 4.14.y now.

> > That said, it is fine to backport those patches, and I would include 4.9
> > and 4.14, as I would think you have a similar requirement that we have in
> > the stable-rt trees. That is you shouldn't experience a regression going
> > from an older kernel to a newer one because the older one had a fix
> > backported to it that a newer one did not. Basically the same rationale that
> > all fixes go into Linus's tree before backporting. We do the same on the
> > stable-rt, where all fixes go in all maintained stable trees that are newer
> > than the one you are backporting to.
> > 
> > Although, it does allow more to be traced than what recordmcount enables.
> > But hopefully it doesn't cause any issues. Maybe I should do some ftrace
> > testing before you go and release any of those stables with those patches.
> > 
> > I'm looking to see if this new "feature" of binutils isn't causing trouble
> > elsewhere. I'm thinking that ftrace is just the canary here.
> 
> It already caused quite a bit of trouble with objtool (as did a previous
> similar change by the Clang assembler).

I feel like we go through this every major binutils release.  Why isn't
"build the Linux kernel" part of the binutils regression test suite by
now?  :)

thanks,

greg k-h

Re: [PATCH] ftrace: Do not reference symbols in sections without size

[Steven Rostedt]

On Tue, 16 Feb 2021 16:54:58 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Feb 16, 2021 at 09:45:39AM -0600, Josh Poimboeuf wrote:
> > On Tue, Feb 16, 2021 at 09:51:21AM -0500, Steven Rostedt wrote:  
> > > On Tue, 16 Feb 2021 12:04:06 +0100
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > >   
> > > > Thanks for this.
> > > > 
> > > > Should I also queue these up for 4.9 and 4.14 which do not have these
> > > > commits in them either (but somehow do not show the problem, yet)?  
> > >
> > > This bothers me. I want to know exactly why this is a problem.  
> > 
> > I actually see the same problem with 4.9 and 4.14, using the same
> > config.  
> 

Interesting. I removed the "-mrecordmcount" option from 5.10.16, and I'm
currently building it, and so far it has no issues. :-/

This is on a fresh install of archlinux using binutils 2.36-3.

> Ok, that's good to know.
> 
> > It's very config-specific.  Something has to convince the toolchain to
> > not reference those two weak functions by section.  
> 
> I've queued up all 3 patches (Steven collapsed 2 into 1 it seems) for
> 4.4.y, 4.9.y, and 4.14.y now.

Which were the three patches. And if I collapsed it into 1, that was by
mistake :-/  I was doing the work on the archlinux VM and was using quilt
to fix up the backport (as I usually do), but I was using vim instead of my
normal emacs (yes I'm one of those), and may have screwed things up.

> 
> > > That said, it is fine to backport those patches, and I would include 4.9
> > > and 4.14, as I would think you have a similar requirement that we have in
> > > the stable-rt trees. That is you shouldn't experience a regression going
> > > from an older kernel to a newer one because the older one had a fix
> > > backported to it that a newer one did not. Basically the same rationale that
> > > all fixes go into Linus's tree before backporting. We do the same on the
> > > stable-rt, where all fixes go in all maintained stable trees that are newer
> > > than the one you are backporting to.
> > > 
> > > Although, it does allow more to be traced than what recordmcount enables.
> > > But hopefully it doesn't cause any issues. Maybe I should do some ftrace
> > > testing before you go and release any of those stables with those patches.
> > > 
> > > I'm looking to see if this new "feature" of binutils isn't causing trouble
> > > elsewhere. I'm thinking that ftrace is just the canary here.  
> > 
> > It already caused quite a bit of trouble with objtool (as did a previous
> > similar change by the Clang assembler).  
> 
> I feel like we go through this every major binutils release.  Why isn't
> "build the Linux kernel" part of the binutils regression test suite by
> now?  :)

Perhaps it builds the latest kernel fine? As I said, I'm currently testing
a 5.10 build (couldn't get the 4.4 to boot, so I wanted to see if the issue
is on 5.10 as that's the kernel they were using).

-- Steve