Re: [PATCH v4] scripts/link-vmlinux.sh: Add alias to duplicate symbols for kallsyms

[Francis Laniel]

Hi.

Le mercredi 20 septembre 2023, 21:04:42 EEST Alessandro Carminati a écrit :
> Hello Francis,
> 
> Thanks a lot for the review.

You are welcome.
I also tested it and it works well:
root@vm-amd64:~# grep ' name_show' /proc/kallsyms | head -6
ffffffff810fa070 t name_show
ffffffff810fa070 t name_show@kernel_irq_irqdesc_c_264
ffffffff815e67c0 t name_show
ffffffff815e67c0 t name_show@drivers_pnp_card_c_186
ffffffff81728bb0 t name_show
ffffffff81728bb0 t name_show@drivers_gpu_drm_i915_gt_sysfs_engines_c_26

> Il giorno mer 20 set 2023 alle ore 12:53 Francis Laniel
> 
> <flaniel@xxxxxxxxxxxxxxxxxxx> ha scritto:
> > Hi.
> > 
> > Le mardi 19 septembre 2023, 22:39:48 EEST Alessandro Carminati (Red Hat) a
> > 
> > écrit :
> > > It is not uncommon for drivers or modules related to similar peripherals
> > > to have symbols with the exact same name.
> > > While this is not a problem for the kernel's binary itself, it becomes
> > > an
> > > issue when attempting to trace or probe specific functions using
> > > infrastructure like ftrace or kprobe.
> > > 
> > > The tracing subsystem relies on the `nm -n vmlinux` output, which
> > > provides
> > > symbol information from the kernel's ELF binary. However, when multiple
> > > symbols share the same name, the standard nm output does not
> > > differentiate
> > > between them. This can lead to confusion and difficulty when trying to
> > > probe the intended symbol.
> > > 
> > >  ~ # cat /proc/kallsyms | grep " name_show"
> > >  ffffffff8c4f76d0 t name_show
> > >  ffffffff8c9cccb0 t name_show
> > >  ffffffff8cb0ac20 t name_show
> > >  ffffffff8cc728c0 t name_show
> > >  ffffffff8ce0efd0 t name_show
> > >  ffffffff8ce126c0 t name_show
> > >  ffffffff8ce1dd20 t name_show
> > >  ffffffff8ce24e70 t name_show
> > >  ffffffff8d1104c0 t name_show
> > >  ffffffff8d1fe480 t name_show
> > > 
> > > kas_alias addresses this challenge by enhancing symbol names with
> > > meaningful suffixes generated from the source file and line number
> > > during the kernel build process.
> > > These newly generated aliases provide tracers with the ability to
> > > comprehend the symbols they are interacting with when utilizing the
> > > ftracefs interface.
> > > This approach may also allow for the probing by name of previously
> > > inaccessible symbols.
> > > 
> > >  ~ # cat /proc/kallsyms | grep gic_mask_irq
> > >  ffffd15671e505ac t gic_mask_irq
> > >  ffffd15671e505ac t gic_mask_irq@drivers_irqchip_irq_gic_c_167
> > >  ffffd15671e532a4 t gic_mask_irq
> > >  ffffd15671e532a4 t gic_mask_irq@drivers_irqchip_irq_gic_v3_c_407
> > >  ~ #
> > > 
> > > Changes from v1:
> > > - Integrated changes requested by Masami to exclude symbols with
> > > prefixes
> > > 
> > >   "_cfi" and "_pfx".
> > > 
> > > - Introduced a small framework to handle patterns that need to be
> > > excluded
> > > 
> > >   from the alias production.
> > > 
> > > - Excluded other symbols using the framework.
> > > - Introduced the ability to discriminate between text and data symbols.
> > > - Added two new config symbols in this version:
> > > CONFIG_KALLSYMS_ALIAS_DATA,
> > > 
> > >   which allows data for data, and CONFIG_KALLSYMS_ALIAS_DATA_ALL, which
> > >   excludes all filters and provides an alias for each duplicated symbol.
> > > 
> > > https://lore.kernel.org/all/20230711151925.1092080-1-alessandro.carminat
> > > i@gm ail.com/
> > > 
> > > Changes from v2:
> > > - Alias tags are created by querying DWARF information from the vmlinux.
> > > - The filename + line number is normalized and appended to the original
> > > 
> > >   name.
> > > 
> > > - The tag begins with '@' to indicate the symbol source.
> > > - Not a change, but worth mentioning, since the alias is added to the
> > > 
> > >   existing list, the old duplicated name is preserved, and the livepatch
> > >   way of dealing with duplicates is maintained.
> > > 
> > > - Acknowledging the existence of scenarios where inlined functions
> > > 
> > >   declared in header files may result in multiple copies due to compiler
> > >   behavior, though it is not actionable as it does not pose an
> > >   operational
> > >   issue.
> > > 
> > > - Highlighting a single exception where the same name refers to
> > > different
> > > 
> > >   functions: the case of "compat_binfmt_elf.c," which directly includes
> > >   "binfmt_elf.c" producing identical function copies in two separate
> > >   modules.
> > > 
> > > https://lore.kernel.org/all/20230714150326.1152359-1-alessandro.carminat
> > > i@gm ail.com/
> > > 
> > > Changes from v3:
> > > - kas_alias was rewritten in Python to create a more concise and
> > > 
> > >   maintainable codebase.
> > > 
> > > - The previous automation process used by kas_alias to locate the
> > > vmlinux
> > > 
> > >   and the addr2line has been replaced with an explicit command-line
> > >   switch
> > >   for specifying these requirements.
> > > 
> > > - addr2line has been added into the main Makefile.
> > > - A new command-line switch has been introduced, enabling users to
> > > extend
> > > 
> > >   the alias to global data names.
> > > 
> > > https://lore.kernel.org/all/20230828080423.3539686-1-alessandro.carminat
> > > i@gm ail.com/
> > > 
> > > NOTE:
> > > About the symbols name duplication that happens as consequence of the
> > > inclusion compat_binfmt_elf.c does, it is evident that this corner is
> > > inherently challenging the addr2line approach.
> > > Attempting to conceal this limitation would be counterproductive.
> > > 
> > > compat_binfmt_elf.c includes directly binfmt_elf.c, addr2line can't help
> > > but report all functions and data declared by that file, coming from
> > > binfmt_elf.c.
> > > 
> > > My position is that, rather than producing a more complicated pipeline
> > > to handle this corner case, it is better to fix the compat_binfmt_elf.c
> > > anomaly.
> > > 
> > > This patch does not deal with the two potentially problematic symbols
> > > defined by compat_binfmt_elf.c
> > 
> > First, thank you for the v4, you will find in the remaining of the
> > messages
> > some comments but for now, I did not test it (this is planned).
> > On a general way, using python really helps here as the code is more
> > straightforward, thank you for this change.
> > 
> > Regarding the problem with compat_binfmt_elf.c, do you have any idea on
> > how to address it?
> > I can maybe take a look at it but I would like to avoid breaking
> > everything.
> compat_binfmt_elf.c is a clever hack that enables sharing source code
> between two different modules while allowing for command differences through
> config macros [1] [2].
> The key lies in the fact they have only few differences.
> 
> In my view, a good approach would be to refactor both compat_binfmt_elf.c
> and binfmt_elf.c, extracting common code and accessing it through wrappers.
> This way, anyone looking to explore the functionality provided by either
> module would have distinct symbols to work with.
> Consolidating the two functions into one also seems beneficial, including in
> contexts like livepatch scenarios.
> 
> The trade-off here is that the modifications currently made using macros
> would need to be done at runtime.
> Fortunately, from what I see in the code, these changes appear to be
> relatively modest, and the functions don't seem to be critical loops.
> Therefore, sacrificing a few cycles to evaluate a flag doesn't appear to be
> a game-changer.

Thank you for all this information, I will take a deeper look at it but cannot 
guarantee I will come back with something.

> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/
> binfmt_elf.c#n754 [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/
> binfmt_elf.c#n1317
> > > Signed-off-by: Alessandro Carminati (Red Hat)
> > > <alessandro.carminati@xxxxxxxxx> ---
> > > 
> > >  Makefile                |   4 +-
> > >  init/Kconfig            |  22 +++++++
> > >  scripts/kas_alias.py    | 132 ++++++++++++++++++++++++++++++++++++++++
> > >  scripts/link-vmlinux.sh |  20 +++++-
> > >  4 files changed, 175 insertions(+), 3 deletions(-)
> > >  create mode 100755 scripts/kas_alias.py
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 4f283d915e54..f33c179f4cc3 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -488,6 +488,7 @@ OBJCOPY           = $(LLVM_PREFIX)llvm-objcopy$
> > 
> > (LLVM_SUFFIX)
> > 
> > >  OBJDUMP              = $(LLVM_PREFIX)llvm-objdump$(LLVM_SUFFIX)
> > >  READELF              = $(LLVM_PREFIX)llvm-readelf$(LLVM_SUFFIX)
> > >  STRIP                = $(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX)
> > > 
> > > +ADDR2LINE    = $(LLVM_PREFIX)llvm-addr2line$(LLVM_SUFFIX)
> > > 
> > >  else
> > >  CC           = $(CROSS_COMPILE)gcc
> > >  LD           = $(CROSS_COMPILE)ld
> > > 
> > > @@ -497,6 +498,7 @@ OBJCOPY           = $(CROSS_COMPILE)objcopy
> > > 
> > >  OBJDUMP              = $(CROSS_COMPILE)objdump
> > >  READELF              = $(CROSS_COMPILE)readelf
> > >  STRIP                = $(CROSS_COMPILE)strip
> > > 
> > > +ADDR2LINE    = $(CROSS_COMPILE)addr2line
> > > 
> > >  endif
> > >  RUSTC                = rustc
> > >  RUSTDOC              = rustdoc
> > > 
> > > @@ -611,7 +613,7 @@ export RUSTC_BOOTSTRAP := 1
> > > 
> > >  export ARCH SRCARCH CONFIG_SHELL BASH HOSTCC KBUILD_HOSTCFLAGS
> > > 
> > > CROSS_COMPILE LD CC HOSTPKG_CONFIG export RUSTC RUSTDOC RUSTFMT
> > > RUSTC_OR_CLIPPY_QUIET RUSTC_OR_CLIPPY BINDGEN CARGO export HOSTRUSTC
> > > KBUILD_HOSTRUSTFLAGS
> > > -export CPP AR NM STRIP OBJCOPY OBJDUMP READELF PAHOLE RESOLVE_BTFIDS
> > > LEX
> > > YACC AWK INSTALLKERNEL +export CPP AR NM STRIP OBJCOPY OBJDUMP READELF
> > > ADDR2LINE PAHOLE RESOLVE_BTFIDS LEX YACC AWK INSTALLKERNEL export PERL
> > > PYTHON3 CHECK CHECKFLAGS MAKE UTS_MACHINE HOSTCXX
> > > 
> > >  export KGZIP KBZIP2 KLZOP LZMA LZ4 XZ ZSTD
> > >  export KBUILD_HOSTCXXFLAGS KBUILD_HOSTLDFLAGS KBUILD_HOSTLDLIBS
> > > 
> > > LDFLAGS_MODULE diff --git a/init/Kconfig b/init/Kconfig
> > > index 6d35728b94b2..d45dd423e1ec 100644
> > > --- a/init/Kconfig
> > > +++ b/init/Kconfig
> > > @@ -1738,6 +1738,28 @@ config KALLSYMS_BASE_RELATIVE
> > > 
> > >         time constants, and no relocation pass is required at runtime to
> > >         fix
> > >         up the entries based on the runtime load address of the kernel.
> > > 
> > > +config KALLSYMS_ALIAS_SRCLINE
> > > +     bool "Produces alias for duplicated text symbols" if EXPERT
> > > +     depends on KALLSYMS && DEBUG_INFO && !DEBUG_INFO_SPLIT
> > > +     help
> > > +       It is not uncommon for drivers or modules related to similar
> > > +       peripherals to have symbols with the exact same name.
> > > +       While this is not a problem for the kernel's binary itself, it
> > > +       becomes an issue when attempting to trace or probe specific
> > > +       functions using infrastructure like ftrace or kprobe.
> > > +
> > > +       This option addresses this challenge, producing alias for text
> > > +       symbol names that include the file name and line where the
> > > symbols
> > > +       are defined in the source code.
> > > +
> > > +config KALLSYMS_ALIAS_SRCLINE_DATA
> > > +     bool "Produces alias also for global variables names"
> > > +     depends on KALLSYMS_ALIAS_SRCLINE
> > > +     help
> > > +       Sometimes it can be useful to refer to global vars by name.
> > > Since
> > > +       they suffer the same issue as text symbols, this config option
> > > +       allows having aliases for global variables names too.
> > > +
> > > 
> > >  # end of the "standard kernel features (expert users)" menu
> > >  
> > >  # syscall, maps, verifier
> > > 
> > > diff --git a/scripts/kas_alias.py b/scripts/kas_alias.py
> > > new file mode 100755
> > > index 000000000000..8cc2a2178da6
> > > --- /dev/null
> > > +++ b/scripts/kas_alias.py
> > > @@ -0,0 +1,132 @@
> > > +#!/usr/bin/env python3
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +#
> > > +# Copyright (C) 2023 Red Hat, Inc. Alessandro Carminati
> > > <alessandro.carminati@xxxxxxxxx> +#
> > > +# kas_alias: Adds alias to duplicate symbols in the kallsyms output.
> > > +
> > > +import subprocess
> > > +import sys
> > > +import os
> > > +import argparse
> > > +import re
> > > +from collections import namedtuple
> > > +
> > > +regex_filter = [
> > > +        "^__compound_literal\\.[0-9]+$",
> > > +        "^__[wm]*key\\.[0-9]+$",
> > > +        "^_*TRACE_SYSTEM.*$",
> > > +        "^__already_done\\.[0-9]+$",
> > > +        "^__msg\\.[0-9]+$",
> > > +        "^__func__\\.[0-9]+$",
> > > +        "^CSWTCH\\.[0-9]+$",
> > > +        "^_rs\\.[0-9]+$",
> > > +        "^___tp_str\\.[0-9]+$",
> > > +        "^__flags\\.[0-9]+$",
> > > +        "^___done\\.[0-9]+$",
> > > +        "^__print_once\\.[0-9]+$",
> > > +        "^___once_key\\.[0-9]+$",
> > > +        "^__pfx_.*$",
> > > +        "^__cfi_.*$"
> > > +        ]
> > > +
> > > +class SeparatorType:
> > > +    def __call__(self, separator):
> > > +        if len(separator) != 1:
> > > +            raise argparse.ArgumentTypeError("Separator must be a
> > > single
> > > character") +        return separator
> > > +
> > > +Line = namedtuple('Line', ['address', 'type', 'name'])
> > > +
> > > +def parse_file(filename):
> > > +    symbol_list = []
> > > +    name_occurrences = {}
> > > +
> > > +    with open(filename, 'r') as file:
> > > +        for line in file:
> > > +            fields = line.strip().split()
> > > +
> > > +            if len(fields) >= 3:
> > > +                address, type, name = fields[0], fields[1], '
> > > '.join(fields[2:]) +                symbol_list.append(Line(address,
> > > type,
> > > name))
> > > +                name_occurrences[name] = name_occurrences.get(name, 0)
> > > + 1
> > > +
> > > +    return symbol_list, name_occurrences
> > > +
> > > +def find_duplicate(symbol_list, name_occurrences):
> > > +    name_to_lines = {}
> > > +    duplicate_lines = []
> > > +
> > > +    for line in symbol_list:
> > > +        if line.name in name_to_lines:
> > > +            first_occurrence = name_to_lines[line.name]
> > > +            duplicate_lines.extend([first_occurrence, line])
> > > +        else:
> > > +            name_to_lines[line.name] = line
> > > +
> > > +    return duplicate_lines
> > > +
> > > +def start_addr2line_process(binary_file, addr2line_file):
> > > +    try:
> > > +        addr2line_process = subprocess.Popen([addr2line_file, '-fe',
> > > binary_file], +
> > > stdin=subprocess.PIPE, +
> > > stdout=subprocess.PIPE, +
> > > stderr=subprocess.PIPE, +
> > > text=True)
> > > +        return addr2line_process
> > > +    except Exception as e:
> > > +        print(f"Error starting addr2line process: {str(e)}")
> > > +        return None
> > 
> > Here, you can raise another exception, otherwise this error message will
> > be
> > printed on stdout as you use print().
> > 
> > > +
> > > +def addr2line_fetch_address(addr2line_process, address):
> > > +    try:
> > > +        addr2line_process.stdin.write(address + '\n')
> > > +        addr2line_process.stdin.flush()
> > > +        addr2line_process.stdout.readline().strip()
> > > +        output = addr2line_process.stdout.readline().strip()
> > > +
> > > +        return os.path.normpath(output)
> > > +    except Exception as e:
> > > +        print(f"Error communicating with addr2line: {str(e)}")
> > > +        return None
> > 
> > Same comment than above.
> 
> Hmm, you might be onto something there.
> The issue here is that I probably shouldn't return at all and should just
> go ahead and terminate the program. I mean, if I hit this exception, it
> means I couldn't spawn addr2line or fetch results from it.
> In that case, I can't provide the functionality anyway.
> When I initially wrote the function, my idea was to prevent the kernel
> build pipeline from failing completely by taking the input and pushing it
> to the output (even though the application wouldn't provide the
> functionality).
> But now I started thinking about it from the perspective of a user who
> really needs that functionality.
> Despite having to enable it, it does not present itself.
> That way I'm just complicating the debug.
> 
> I came to the conclusion that it's best to just crash the application and
> halt the pipeline if either of the two fails.
> I will change it accordingly.
> 
> > > +def process_line(line, config):
> > line should be named obj here.
> 
> fair.
> 
> > > +    if config:
> > > +        return not (any(re.match(regex, obj.name) for regex in
> > > regex_filter)) +    else:
> > > +        return obj.type in {"T", "t"}
> > > +if __name__ == "__main__":
> > > +    parser = argparse.ArgumentParser(description='Add alias to multiple
> > > occurring symbols name in kallsyms') +    parser.add_argument('-a',
> > > "--addr2line", dest="addr2line_file", required=True) +
> > > parser.add_argument('-v', "--vmlinux", dest="vmlinux_file",
> > > required=True)
> > > +    parser.add_argument('-o', "--outfile", dest="output_file",
> > > required=True) +    parser.add_argument('-n', "--nmdata",
> > > dest="nm_data_file", required=True) +    parser.add_argument('-s',
> > > "--separator", dest="separator", required=False, default="@",
> > > type=SeparatorType()) +    parser.add_argument('-d', "--data",
> > > dest="include_data", required=False, action='store_true') +    config =
> > > parser.parse_args()
> > > +
> > > +    try:
> > > +        config.linux_base_dir = os.getcwd()+"/"
> > > +        symbol_list, name_occurrences = parse_file(config.nm_data_file)
> > > +        addr2line_process =
> > > start_addr2line_process(config.vmlinux_file,
> > > config.addr2line_file) +
> > > +        with open(config.output_file, 'w') as file:
> > > +            for obj in symbol_list:
> > > +                file.write("{} {} {}\n".format(obj.address, obj.type,
> > > obj.name))
> > 
> > I am not a python expert but is there something which prevents using
> > f-string here?
> 
> Agree, best to have a single style.
> 
> > > +                if (name_occurrences[obj.name] > 1) and
> > > process_line(obj, config.include_data) : +                    output =
> > > addr2line_fetch_address(addr2line_process, obj.address) +
> > > 
> > >  decoration = config.separator + "".join(
> > > 
> > > +                        "_" if not c.isalnum() else c for c in
> > > output.replace(config.linux_base_dir, "") +                    )
> > 
> > Cannot the above be simplified to:
> > decoration = config.separator + config.linux_base_dir + ("_" if not
> > c.isalnum() else c for c in output)
> > 
> > > +                    if decoration != config.separator + "____":
> > Why exactly "____" and not "_+" (+ in the regex meaning of {1, n})?
> 
> The reason for using "____" is because when addr2line emits the special
> string "?:??" its normalized version becomes "____" .
> "?:??" occurs when addr2line can not find the specified address in the
> DWARF section, which is typical of symbols introduced by the compiler.
> In such cases, emitting an alias wouldn't make sense, so I skip it.

OK, this makes sense!
I am wondering nonetheless what do you think about adding a comment which 
would indicate that "____" is the translation of "?:??"? This would be useful 
for people, like me, who does not have a great knowledge about addr2line.

> > > +                        file.write("{} {} {}\n".format(obj.address,
> > > obj.type, obj.name + decoration)) +
> > > +        addr2line_process.stdin.close()
> > > +        addr2line_process.stdout.close()
> > > +        addr2line_process.stderr.close()
> > > +        addr2line_process.wait()
> > > +
> > > +    except Exception as e:
> > > +        print(f"An error occurred: {str(e)}")
> > > +        raise SystemExit("Script terminated due to an error")
> > 
> > Maybe you can fuse the two:
> > raise SystemExit(f"Script terminated due to an error: {str(e)}")
> 
> Got it, thanks
> 
> > > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> > > index a432b171be82..7cc24fd5f6b4 100755
> > > --- a/scripts/link-vmlinux.sh
> > > +++ b/scripts/link-vmlinux.sh
> > > @@ -91,7 +91,12 @@ vmlinux_link()
> > > 
> > >       # The kallsyms linking does not need debug symbols included.
> > >       if [ "$output" != "${output#.tmp_vmlinux.kallsyms}" ] ; then
> > > 
> > > -             ldflags="${ldflags} ${wl}--strip-debug"
> > > +             # The kallsyms linking does not need debug symbols
> > > included,
> > > +             # unless the KALLSYMS_ALIAS_SRCLINE.
> > > +             if ! is_enabled CONFIG_KALLSYMS_ALIAS_SRCLINE && \
> > > +                [ "$output" != "${output#.tmp_vmlinux.kallsyms}" ] ;
> > > then
> > > +                     ldflags="${ldflags} ${wl}--strip-debug"
> > > +             fi
> > > 
> > >       fi
> > >       
> > >       if is_enabled CONFIG_VMLINUX_MAP; then
> > > 
> > > @@ -161,7 +166,18 @@ kallsyms()
> > > 
> > >       fi
> > >       
> > >       info KSYMS ${2}
> > > 
> > > -     scripts/kallsyms ${kallsymopt} ${1} > ${2}
> > > +     ALIAS=""
> > > +     KAS_DATA=""
> > > +     if is_enabled CONFIG_KALLSYMS_ALIAS_SRCLINE_DATA; then
> > > +             KAS_DATA="-d"
> > > +     fi
> > > +     if is_enabled CONFIG_KALLSYMS_ALIAS_SRCLINE; then
> > > +             ALIAS=".alias"
> > > +             scripts/kas_alias.py \
> > > +                     -a ${ADDR2LINE} -v ${kallsyms_vmlinux} -n ${1} \
> > > +                     -o ${1}${ALIAS} -s @ ${KAS_DATA}
> > 
> > The separator can indeed be set for the python script but is hardcoded
> > from
> > the kernel point of view as there are no corresponding CONFIG_.
> > This is totally fine for me, as if someone wants a specific separator
> > he/she can edit this file, but was it your goal?
> 
> Indeed.
> While your earlier point made sense to me, Petr's arguments were quite
> convincing.
> So, the kernel does hardcode the separator, but if someone really wants
> to change it, they can simply edit a character in the
> scripts/link-vmlinux.sh file.

I totally agree with Petr's comment.
I think adding a format or other complicated stuff is just a remix of "the 
highway to hell is paved with good intentions".
So better to let it as it, and expert users can just edit the script.

> > > +     fi
> > > +     scripts/kallsyms ${kallsymopt} ${1}${ALIAS} > ${2}
> > > 
> > >  }
> > >  
> > >  # Perform one step in kallsyms generation, including temporary linking
> > >  of
> > 
> > Best regards.

Best regards.