Hi Steve,
I noticed that ftrace does not currently support functions built with
the -ffunction-sections option as they end up in .text.<function_name>
ELF sections, never making into the __mcount_loc section.
I modified the recordmcount scripts to handle such .text. section
prefixes and this appears to work on x86_64, ppc64le, and s390x -- at
least for simple modules, including the "Test trace_printk from module"
ftrace self-test.
That said, I did notice 90ad4052e85c ("kbuild: avoid conflict between
-ffunction-sections and -pg on gcc-4.7") which indicates that the kernel
still supports versions of gcc which may not play well with ftrace and
-ffunction-sections.
With that limitation in mind, can we support ftracing of functions in
such sections for compiler versions that do support it? (fwiw, gcc
v4.8.5 seems happy) And then if so, what additional testing or coding
would need to be done to be confident that it is safe? Is matching on
".text.*" too inclusive?
Thanks,
-- Joe
Joe Lawrence (1):
scripts/recordmcount.{c,pl}: support -ffunction-sections .text.*
section names
scripts/recordmcount.c | 2 +-
scripts/recordmcount.pl | 13 +++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
--
1.8.3.1
When building with -ffunction-sections, the compiler will place each
function into its own ELF section, prefixed with ".text". For example,
a simple test module with functions test_module_do_work() and
test_module_wq_func():
% objdump --section-headers test_module.o | awk '/\.text/{print $2}'
.text
.text.test_module_do_work
.text.test_module_wq_func
.init.text
.exit.text
Adjust the recordmcount scripts to look for ".text" as a section name
prefix. This will ensure that those functions will be included in the
__mcount_loc relocations:
% objdump --reloc --section __mcount_loc test_module.o
OFFSET TYPE VALUE
0000000000000000 R_X86_64_64 .text.test_module_do_work
0000000000000008 R_X86_64_64 .text.test_module_wq_func
0000000000000010 R_X86_64_64 .init.text
Signed-off-by: Joe Lawrence <[email protected]>
---
scripts/recordmcount.c | 2 +-
scripts/recordmcount.pl | 13 +++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 895c40e8679f..a50a2aa963ad 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -397,7 +397,7 @@ static uint32_t w2nat(uint16_t const x)
static int
is_mcounted_section_name(char const *const txtname)
{
- return strcmp(".text", txtname) == 0 ||
+ return strncmp(".text", txtname, 5) == 0 ||
strcmp(".init.text", txtname) == 0 ||
strcmp(".ref.text", txtname) == 0 ||
strcmp(".sched.text", txtname) == 0 ||
diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index f599031260d5..68841d01162c 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -142,6 +142,11 @@ my %text_sections = (
".text.unlikely" => 1,
);
+# Acceptable section-prefixes to record.
+my %text_section_prefixes = (
+ ".text." => 1,
+);
+
# Note: we are nice to C-programmers here, thus we skip the '||='-idiom.
$objdump = 'objdump' if (!$objdump);
$objcopy = 'objcopy' if (!$objcopy);
@@ -519,6 +524,14 @@ while (<IN>) {
# Only record text sections that we know are safe
$read_function = defined($text_sections{$1});
+ if (!$read_function) {
+ foreach my $prefix (keys %text_section_prefixes) {
+ if (substr($1, 0, length $prefix) eq $prefix) {
+ $read_function = 1;
+ last;
+ }
+ }
+ }
# print out any recorded offsets
update_funcs();
--
1.8.3.1
On 11/20/2018 03:19 PM, Joe Lawrence wrote:
> Hi Steve,
>
> I noticed that ftrace does not currently support functions built with
> the -ffunction-sections option as they end up in .text.<function_name>
> ELF sections, never making into the __mcount_loc section.
>
> I modified the recordmcount scripts to handle such .text. section
> prefixes and this appears to work on x86_64, ppc64le, and s390x -- at
> least for simple modules, including the "Test trace_printk from module"
> ftrace self-test.
>
> That said, I did notice 90ad4052e85c ("kbuild: avoid conflict between
> -ffunction-sections and -pg on gcc-4.7") which indicates that the kernel
> still supports versions of gcc which may not play well with ftrace and
> -ffunction-sections.
>
> With that limitation in mind, can we support ftracing of functions in
> such sections for compiler versions that do support it? (fwiw, gcc
> v4.8.5 seems happy) And then if so, what additional testing or coding
> would need to be done to be confident that it is safe? Is matching on
> ".text.*" too inclusive?
>
Gentle ping... I took a dive through the rhkl-archives and found a few
older discussions:
[PATCH] scripts/recordmcount.pl: Support build with -ffunction-sections.
https://lore.kernel.org/lkml/CAFbHwiRtBaHkpZqTm6VZ=fCJcyu+dsdpo_kxMHy1egce=rTuyA@mail.gmail.com/
and related LWN article:
The source of the e1000e corruption bug
https://lwn.net/Articles/304105/
Catching up with those, I assume that this has never been implemented in
the past due to fear of ftrace modifying a potentially freed section
(and bricking NICs in the process :(
Looking through the kernel sources (like Will in 2008) I don't see any
code jumping out at me that frees code other than .init. However a
quick code inspection is no guarantee.
Assuming the same use-after-free reservation holds true today:
1: Is there any reasonable way to mark code sections (pages?) as
in-use to avoid memory freeing mechanisms from releasing them? The
logic for .init is mostly arch-specific, so there could be many
different ways random arches may try to pull this off.
2: Would/could it be safer to restrict __mcount_loc detection of
".text.*" sections to modules? The recordmcount.pl script already
knows about is_module... that information could be provided to
recordmcount.c as well for consideration.
-- Joe
On Tue, 27 Nov 2018 15:27:14 -0500
Joe Lawrence <[email protected]> wrote:
> Gentle ping... I took a dive through the rhkl-archives and found a few
> older discussions:
Thanks for the reminder, my INBOX is totally out of control with
Plumbers followed by Turkey Day.
>
> [PATCH] scripts/recordmcount.pl: Support build with -ffunction-sections.
> https://lore.kernel.org/lkml/CAFbHwiRtBaHkpZqTm6VZ=fCJcyu+dsdpo_kxMHy1egce=rTuyA@mail.gmail.com/
>
> and related LWN article:
>
> The source of the e1000e corruption bug
> https://lwn.net/Articles/304105/
>
> Catching up with those, I assume that this has never been implemented in
> the past due to fear of ftrace modifying a potentially freed section
> (and bricking NICs in the process :(
Actually, we have a lot more safe guards against that today.
>
> Looking through the kernel sources (like Will in 2008) I don't see any
> code jumping out at me that frees code other than .init. However a
> quick code inspection is no guarantee.
>
> Assuming the same use-after-free reservation holds true today:
>
> 1: Is there any reasonable way to mark code sections (pages?) as
> in-use to avoid memory freeing mechanisms from releasing them? The
> logic for .init is mostly arch-specific, so there could be many
> different ways random arches may try to pull this off.
>
> 2: Would/could it be safer to restrict __mcount_loc detection of
> ".text.*" sections to modules? The recordmcount.pl script already
> knows about is_module... that information could be provided to
> recordmcount.c as well for consideration.
I'm fine with just applying your patch. Today, for x86, there's a gcc
option that adds the __mcount_loc automatically without doing any
whitelisting (it doesn't run recordmcount.*). It just adds anything that
is traced, thus it has to work for all possible cases now.
-- Steve
On 11/27/2018 09:09 PM, Steven Rostedt wrote:
> On Tue, 27 Nov 2018 15:27:14 -0500
> Joe Lawrence <[email protected]> wrote:
>
>> Gentle ping... I took a dive through the rhkl-archives and found a few
>> older discussions:
>
> Thanks for the reminder, my INBOX is totally out of control with
> Plumbers followed by Turkey Day.
>
> [ ... snip ... ]
>
> I'm fine with just applying your patch. Today, for x86, there's a gcc
> option that adds the __mcount_loc automatically without doing any
> whitelisting (it doesn't run recordmcount.*). It just adds anything that
> is traced, thus it has to work for all possible cases now.
>
Ah right, -mcount-record ... I must have been using an older gcc w/o
-mcount-record in my testing. Thanks for taking a look and merging.
Regards,
-- Joe