Message-ID: <CAK7LNAQaLc6aDK85qQtPHoCkQSGyL-TxXjpgJTfehe2Q1=jMSA@mail.gmail.com>
Date: Tue, 16 Jul 2024 17:33:30 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Zheng Yejian <zhengyejian1@...wei.com>
Cc: rostedt@...dmis.org, mhiramat@...nel.org, mark.rutland@....com, 
	mpe@...erman.id.au, npiggin@...il.com, christophe.leroy@...roup.eu, 
	naveen.n.rao@...ux.ibm.com, tglx@...utronix.de, mingo@...hat.com, 
	bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com, 
	mcgrof@...nel.org, mathieu.desnoyers@...icios.com, nathan@...nel.org, 
	nicolas@...sle.eu, kees@...nel.org, james.clark@....com, 
	kent.overstreet@...ux.dev, yhs@...com, jpoimboe@...nel.org, 
	peterz@...radead.org, linux-kernel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, 
	linux-modules@...r.kernel.org, linux-kbuild@...r.kernel.org, 
	bpf@...r.kernel.org
Subject: Re: [PATCH 2/6] kallsyms: Emit symbol at the holes in the text

On Thu, Jun 13, 2024 at 10:36 PM Zheng Yejian <zhengyejian1@...wei.com> wrote:
>
> When a weak type function is overridden, its symbol will be removed
> from the symbol table, but its code will not be removed. Besides,
> due to lacking of size for kallsyms, kernel compute function size by
> substracting its symbol address from its next symbol address (see
> kallsyms_lookup_size_offset()). These will cause that size of some
> function is computed to be larger than it actually is, just because
> symbol of its following weak function is removed.
>
> This issue also causes multiple __fentry__ locations to be counted in
> the some function scope, and eventually causes ftrace_location() to find
> wrong __fentry__ location. It was reported in
> Link: https://lore.kernel.org/all/20240607115211.734845-1-zhengyejian1@huawei.com/
>
> Peter suggested to change scipts/kallsyms.c to emit readily
> identifiable symbol names for all the weak junk, eg:
>
>   __weak_junk_NNNNN
>
> The name of this kind symbol needs some discussion, but it's temporarily
> called "__hole_symbol_XXXXX" in this patch:
> 1. Pass size info to scripts/kallsyms  (see mksysmap());
> 2. Traverse sorted function symbols, if one function address plus its
>    size less than next function address, it means there's a hole, then
>    emit a symbol "__hole_symbol_XXXXX" there which type is 't'.
>
> After this patch, the effect is as follows:
>
>   $ cat /proc/kallsyms | grep -A 3 do_one_initcall
>   ffffffff810021e0 T do_one_initcall
>   ffffffff8100245e t __hole_symbol_XXXXX
>   ffffffff810024a0 t __pfx_rootfs_init_fs_context
>
> Suggested-by: Peter Zijlstra <peterz@...radead.org>
> Signed-off-by: Zheng Yejian <zhengyejian1@...wei.com>



With my quick test, "t__hole_symbol_XXXXX" was encoded
into the following 10-byte stream.

.byte 0x09, 0x94, 0xbf, 0x18, 0xf3, 0x3d, 0xce, 0xd1, 0xd1, 0x58



Now "t__hole_symbol_XXXXX" is the most common symbol name.
However, 10 byte is consumed for every instance of
"t__hole_symbol_XXXXX".

This is much less efficient thanI had expected,
although I did not analyze the logic of this inefficiency.







> ---
>  scripts/kallsyms.c      | 101 +++++++++++++++++++++++++++++++++++++++-
>  scripts/link-vmlinux.sh |   4 +-
>  scripts/mksysmap        |   2 +-
>  3 files changed, 102 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 6559a9802f6e..5c4cde864a04 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -35,6 +35,7 @@
>  struct sym_entry {
>         struct sym_entry *next;
>         unsigned long long addr;
> +       unsigned long long size;
>         unsigned int len;
>         unsigned int seq;
>         unsigned int start_pos;
> @@ -74,6 +75,7 @@ static int token_profit[0x10000];
>  static unsigned char best_table[256][2];
>  static unsigned char best_table_len[256];
>
> +static const char hole_symbol[] = "__hole_symbol_XXXXX";
>
>  static void usage(void)
>  {
> @@ -130,8 +132,16 @@ static struct sym_entry *read_symbol(FILE *in, char **buf, size_t *buf_len)
>         size_t len;
>         ssize_t readlen;
>         struct sym_entry *sym;
> +       unsigned long long size = 0;
>
>         errno = 0;
> +       /*
> +        * Example of expected symbol format:
> +        * 1. symbol with size info:
> +        *    ffffffff81000070 00000000000001d7 T __startup_64
> +        * 2. symbol without size info:
> +        *    0000000002a00000 A text_size
> +        */
>         readlen = getline(buf, buf_len, in);
>         if (readlen < 0) {
>                 if (errno) {
> @@ -145,9 +155,24 @@ static struct sym_entry *read_symbol(FILE *in, char **buf, size_t *buf_len)
>                 (*buf)[readlen - 1] = 0;
>
>         addr = strtoull(*buf, &p, 16);
> +       if (*buf == p || *p++ != ' ') {
> +               fprintf(stderr, "line format error: unable to parse address\n");
> +               exit(EXIT_FAILURE);
> +       }
> +
> +       if (*p == '0') {
> +               char *str = p;
>
> -       if (*buf == p || *p++ != ' ' || !isascii((type = *p++)) || *p++ != ' ') {
> -               fprintf(stderr, "line format error\n");
> +               size = strtoull(str, &p, 16);
> +               if (str == p || *p++ != ' ') {
> +                       fprintf(stderr, "line format error: unable to parse size\n");
> +                       exit(EXIT_FAILURE);
> +               }
> +       }
> +
> +       type = *p++;
> +       if (!isascii(type) || *p++ != ' ') {
> +               fprintf(stderr, "line format error: unable to parse type\n");
>                 exit(EXIT_FAILURE);
>         }
>
> @@ -182,6 +207,7 @@ static struct sym_entry *read_symbol(FILE *in, char **buf, size_t *buf_len)
>                 exit(EXIT_FAILURE);
>         }
>         sym->addr = addr;
> +       sym->size = size;
>         sym->len = len;
>         sym->sym[0] = type;
>         strcpy(sym_name(sym), name);
> @@ -795,6 +821,76 @@ static void sort_symbols(void)
>         qsort(table, table_cnt, sizeof(table[0]), compare_symbols);
>  }
>
> +static int may_exist_hole_after_symbol(const struct sym_entry *se)


The return type should be bool.



> +{
> +       char type = se->sym[0];
> +
> +       /* Only check text symbol or weak symbol */
> +       if (type != 't' && type != 'T' &&
> +           type != 'w' && type != 'W')
> +               return 0;
> +       /* Symbol without size has no hole */
> +       return se->size != 0;
> +}
> +
> +static struct sym_entry *gen_hole_symbol(unsigned long long addr)
> +{
> +       struct sym_entry *sym;
> +       static size_t len = sizeof(hole_symbol);
> +
> +       /* include type field */
> +       sym = malloc(sizeof(*sym) + len + 1);
> +       if (!sym) {
> +               fprintf(stderr, "unable to allocate memory for hole symbol\n");
> +               exit(EXIT_FAILURE);
> +       }
> +       sym->addr = addr;
> +       sym->size = 0;
> +       sym->len = len;
> +       sym->sym[0] = 't';
> +       strcpy(sym_name(sym), hole_symbol);
> +       sym->percpu_absolute = 0;
> +       return sym;
> +}
> +
> +static void emit_hole_symbols(void)
> +{
> +       unsigned int i, pos, nr_emit;
> +       struct sym_entry **new_table;
> +       unsigned int new_cnt;
> +
> +       nr_emit = 0;
> +       for (i = 0; i < table_cnt - 1; i++) {
> +               if (may_exist_hole_after_symbol(table[i]) &&
> +                   table[i]->addr + table[i]->size < table[i+1]->addr)
> +                       nr_emit++;
> +       }
> +       if (!nr_emit)
> +               return;
> +
> +       new_cnt = table_cnt + nr_emit;
> +       new_table = malloc(sizeof(*new_table) * new_cnt);


Do you need to allocate another huge table?

You can use realloc() to append the room for nr_emit
if you iterate the table in the reverse order.







> +       if (!new_table) {
> +               fprintf(stderr, "unable to allocate memory for new table\n");
> +               exit(EXIT_FAILURE);
> +       }
> +
> +       pos = 0;
> +       for (i = 0; i < table_cnt; i++) {
> +               unsigned long long addr;
> +
> +               new_table[pos++] = table[i];
> +               if ((i == table_cnt - 1) || !may_exist_hole_after_symbol(table[i]))
> +                       continue;
> +               addr = table[i]->addr + table[i]->size;
> +               if (addr < table[i+1]->addr)
> +                       new_table[pos++] = gen_hole_symbol(addr);
> +       }
> +       free(table);
> +       table = new_table;
> +       table_cnt = new_cnt;
> +}
> +
>  static void make_percpus_absolute(void)
>  {
>         unsigned int i;
> @@ -854,6 +950,7 @@ int main(int argc, char **argv)
>         if (absolute_percpu)
>                 make_percpus_absolute();
>         sort_symbols();
> +       emit_hole_symbols();
>         if (base_relative)
>                 record_relative_base();
>         optimize_token_table();
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index 518c70b8db50..8e1373902bfe 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -189,11 +189,11 @@ kallsyms_step()
>  }
>
>  # Create map file with all symbols from ${1}
> -# See mksymap for additional details
> +# See mksysmap for additional details
>  mksysmap()
>  {
>         info NM ${2}
> -       ${NM} -n "${1}" | sed -f "${srctree}/scripts/mksysmap" > "${2}"
> +       ${NM} -nS "${1}" | sed -f "${srctree}/scripts/mksysmap" > "${2}"
>  }
>
>  sorttable()
> diff --git a/scripts/mksysmap b/scripts/mksysmap
> index c12723a04655..7a4415f21143 100755
> --- a/scripts/mksysmap
> +++ b/scripts/mksysmap
> @@ -2,7 +2,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  #
>  # sed script to filter out symbols that are not needed for System.map,
> -# or not suitable for kallsyms. The input should be 'nm -n <file>'.
> +# or not suitable for kallsyms. The input should be 'nm -nS <file>'.
>  #
>  # System.map is used by module-init tools and some debugging
>  # tools to retrieve the actual addresses of symbols in the kernel.
> --
> 2.25.1
>
>


-- 
Best Regards
Masahiro Yamada

Powered by blists - more mailing lists