Message-ID: <20250102232609.529842248@goodmis.org>
Date: Thu, 02 Jan 2025 18:26:09 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org,
 linux-kbuild@...r.kernel.org,
 bpf <bpf@...r.kernel.org>
Cc: Masami Hiramatsu <mhiramat@...nel.org>,
 Mark Rutland <mark.rutland@....com>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Peter Zijlstra <peterz@...radead.org>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 Masahiro Yamada <masahiroy@...nel.org>,
 Nathan Chancellor <nathan@...nel.org>,
 Nicolas Schier <nicolas@...sle.eu>,
 Zheng Yejian <zhengyejian1@...wei.com>,
 Martin  Kelly <martin.kelly@...wdstrike.com>,
 Christophe Leroy <christophe.leroy@...roup.eu>,
 Josh Poimboeuf <jpoimboe@...hat.com>
Subject: [PATCH v2 00/16] scripts/sorttable: ftrace: Remove place holders for weak functions in available_filter_functions

This series removes the place holder __ftrace_invalid_address___ from
the available_filter_functions file.

The first 13 patches clean up the scripts/sorttable.c code. It was
a copy from recordmcount.c which is very hard to maintain. That's
because it uses macro helpers and places the code in a header file
sorttable.h to handle both the 64 bit and 32 bit version of  the Elf
structures. It also has _r()/r()/r2() wrappers around accessing the
data which will read the 64 bit or 32 bit version of the data as well
as handle endianess. If the wrong wrapper is used, an invalid value
will result, and this has been a cause for bugs in the past. In fact
the new ORC code doesn't even use it. That's fine because ORC is only
for 64 bit x86 which is the default parsing.

Instead of having a bunch of macros defined and then include the code
twice from a header, the Elf structures are each wrapped in a union.
The union holds the 64 bit and 32 bit version of the needed structure.
To access the values, helper function pointers are used instead of
defining a function. For example, instead of having:

In sorttable.h:

 #undef Elf_Ehdr
 #undef Elf_Shdr

 #ifdef SORTTABLE_64
 # define Elf_Ehdr		Elf64_Ehdr
 # define Elf_Shdr		Elf64_Shdr
 [..]
 # define _r			r8
 #else
 # define Elf_Ehdr		Elf32_Ehdr
 # define Elf_Shdr		Elf32_Shdr
 [..]
 # define _r			r
 #endif

 [..]
 Elf_Shdr *s, *shdr = (Elf_Shdr *)((char *)ehdr + _r(&ehdr->e_shoff));

In sorttable.c:

 #include "sorttable.h"
 #define SORTTABLE_64
 #include "sorttable.h"

Using the Unions we have:

 typedef union {
	Elf32_Ehdr	e32;
	Elf64_Ehdr	e64;
 } Elf_Ehdr;

 typedef union {
	Elf32_Shdr	e32;
	Elf64_Shdr	e64;
 } Elf_Shdr;

 [..]

 static uint64_t ehdr64_shoff(Elf_Ehdr *ehdr)
 {
	return r8(&ehdr->e64.e_shoff);
 }

 static uint64_t ehdr32_shoff(Elf_Ehdr *ehdr)
 {
	return r(&ehdr->e32.e_shoff);
 }

 [..]
 static uint64_t (*ehdr_shoff)(Elf_Ehdr *ehdr);
 [..]

	switch (ehdr->e32.e_ident[EI_CLASS]) {
	case ELFCLASS32:
		[..]
		ehdr_shoff	= ehdr32_shoff;
		[..]
	case ELFCLASS65:
		[..]
		ehdr_shoff	= ehdr64_shoff;
		[..]

 shdr_start = (Elf_Shdr *)((char *)ehdr + ehdr_shoff(ehdr));

The code may be a little more verbose, but the meat of the code is easier to
read, and the conversion functions live in the helper functions to make
it easier to have the fields read the proper way.

This makes the code easier to maintain, and for this purpose easier
to extend. Which is the last patch of the series.

The last 3 patches work to remove the weak functions from the mcount_loc
section.

The first of the 3 replaces the hack to get the __start_mcount_loc and
__stop_mcount_loc values from the symbol table. It replaces calling
popen() on a grep of System.map with looking at the actual symbol
values in the elf format.

The next patch adds the option "-s <file>" to sorttable.c. Now this code
is called by:

  ${NM} -S vmlinux > .tmp_vmlinux.nm-sort
  ${objtree}/scripts/sorttable -s .tmp_vmlinux.nm-sort ${1}

Where the file created by "nm -S" is read, recording the address
and the associated sizes of each function. It then is sorted, and
before sorting the mcount_loc table, it is scanned to make sure
all symbols in the mcounc_loc are within the boundaries of the functions
defined by nm. If they are not, they are zeroed out, as they are most
likely weak functions (I don't know what else they would be).

The last patch adds a ftrace_mcount_skip initdata variable that is
updated by sorttable.c to tell ftrace to skip over the first entries
in the mcount_loc table as they are invalid.

Before:
    
 ~# grep __ftrace_invalid_address___ /sys/kernel/tracing/available_filter_functions | wc -l
 556

After:

 ~# grep __ftrace_invalid_address___ /sys/kernel/tracing/available_filter_functions | wc -l
 0

Ideally, I would have moved the __start_mcount_loc to not have to
modify the kernel, but that would require a bit more work in the
architecture dependent code, as it requires reading the Elf_Rel*
entries.

This is also independent on fixing kallsyms to be able to know the real
size of functions instead of just using the value that goes to the
next function. One solution is to add place holders for weak functions.
But this doesn't help the mcount_loc issue as these entries need to
be removed at boot up and searching kallsyms will have an non-negligible
overhead.

Changes since v1: https://lore.kernel.org/all/20250102185845.928488650@goodmis.org/

- Replaced the last patch with 3 patches.

  The first of the 3 patches removed the hack of reading System.map
  with properly reading the Elf symbol table to find start_mcount_loc
  and stop_mcount_loc.

  The second patch adds the call to "nm -S vmlinux" to get the sizes
  of each function.

  The previous last patch would just check the zeroed out values and compare
  them to kaslr_offset(). Instead, this time, the last patch adds a new
  ftrace_mcount_skip that is used to simply skip over the first entries
  that the sorttable.c moved to the beginning, as they were the weak functions
  that were found.


Steven Rostedt (16):
      scripts/sorttable: Remove unused macro defines
      scripts/sorttable: Remove unused write functions
      scripts/sorttable: Remove unneeded Elf_Rel
      scripts/sorttable: Have the ORC code use the _r() functions to read
      scripts/sorttable: Make compare_extable() into two functions
      scripts/sorttable: Convert Elf_Ehdr to union
      scripts/sorttable: Replace Elf_Shdr Macro with a union
      scripts/sorttable: Convert Elf_Sym MACRO over to a union
      scripts/sorttable: Add helper functions for Elf_Ehdr
      scripts/sorttable: Add helper functions for Elf_Shdr
      scripts/sorttable: Add helper functions for Elf_Sym
      scripts/sorttable: Use uint64_t for mcount sorting
      scripts/sorttable: Move code from sorttable.h into sorttable.c
      scripts/sorttable: Get start/stop_mcount_loc from ELF file directly
      scripts/sorttable: Zero out weak functions in mcount_loc table
      scripts/sorttable: ftrace: Do not add weak functions to available_filter_functions

----
 kernel/trace/ftrace.c   |  16 +-
 scripts/link-vmlinux.sh |   4 +-
 scripts/sorttable.c     | 834 ++++++++++++++++++++++++++++++++++++++++++++----
 scripts/sorttable.h     | 497 -----------------------------
 4 files changed, 794 insertions(+), 557 deletions(-)
 delete mode 100644 scripts/sorttable.h

Powered by blists - more mailing lists