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

[Kris Van Hees]

On Tue, Oct 10, 2023 at 01:33:09PM +0200, Petr Mladek wrote:
> On Mon 2023-10-09 15:14:28, Alessandro Carminati wrote:
> > Hello Kris,
> > 
> > Thank you for your contribution and for having your thought shared with me.
> > 
> > Allow me to begin this conversation by explaining what came to mind when
> > I decided to propose a patch that creates aliases.
> > 
> > The objective was to address a specific problem I was facing while
> > minimizing any potential impact on other aspects.
> > My initial consideration was the existence of numerous tools, both in the
> > kernel and in userspace, that rely on the current kallsyms implementation.
> > Both Nick and I shared the concern that making changes to elements upon
> > which these tools depend on could have significant consequences.
> > 
> > To the best of my knowledge, Nick's strategy has been to duplicate kallsyms
> > with something new - a new, improved kallsyms file.
> > 
> > However, even if Nick's patch were to be accepted, it wouldn't fully meet
> > my personal requirements.
> > This is because my goal was to utilize kprobe on a symbol that shares its
> > name with others. Nick's work wouldn't allow me to do this, and that's why,
> > I proposed an alternative.
> > 
> > As a result, my strategy was more modest and focused solely on creating
> > aliases for duplicate symbols.
> > By adding these aliases, existing tools would remain unaffected, and the
> > current system state and ecosystem would be preserved.
> > For instance, mechanisms like live patching could continue to use the
> > symbol hit count.
> > 
> > On the flip side, introducing these new symbols would enable tracers to
> > directly employ the new names without any modifications, and humans could
> > easily identify the symbol they are dealing with just by examining the
> > name.
> > These are the fundamental principles behind my patch - introducing aliases.
> > 
> > Il giorno gio 5 ott 2023 alle ore 18:25 Kris Van Hees
> > <kris.van.hees@xxxxxxxxxx> ha scritto:
> > >
> > > On Wed, Sep 27, 2023 at 05:35:16PM +0000, Alessandro Carminati (Red Hat) wrote:
> > > > 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
> > >
> > > One problem that remains as far as I can see is that this approach does not
> > > take into consideration that there can be duplicate symbols in the core
> > > kernel, but also between core kernel and loadable modules, and even between
> > > loadable modules.  So, I think more is needed to also ensure that this
> > > approach of adding alias symbols is also done for loadable modules.
> > 
> > To identify which symbols are duplicated, including those contained in
> > modules, it requires exploring all the objects. If I were to propose a
> > complementary tool to kas_alias that operates on modules, it would need to
> > run on all objects to ascertain the state of the names.
> > Only after this assessment could it produce its output.
> > This would entail postponing the second kallsyms pass until after all
> > modules have been processed.
> > Additionally, modules would need to be processed twice: once to assess the
> > names and a second time to generate aliases for duplicated symbols.
> > I am uncertain if the community would be willing to accept such a delay in
> > the build process to introduce this feature.
> 
> >From the livepatching POV:
> 
>   + It needs a way to distinguish duplicate symbols within a module.
> 
>   + It does _not_ need to distinguish symbols which have the same name
>     in two modules or in a module and vmlinux.
> 
> Background: The livepatch contains a structure where the livepatched
> symbols are already split per-livepatched objects: vmlinux or modules.
> I has to know whether a later loaded or removed module is livepatched
> or not and what functions need some tweaking.

Thank you for sharing the POV for livepatching.  That is cery helpful.

My follow-up email to my original response shows an example that demonstrates
this remaining problem...  a loadable module that contains a duplicate symbol,
so this issue does pop up (and is why I raise it here):

# grep ' metadata_show' /proc/kallsyms
ffffffffc05659c0 t metadata_show        [md_mod]
ffffffffc05739f0 t metadata_show        [md_mod]

This is certainly a harder problem to deal with because of how kallsyms data
for a module is handled, but unfortunately, there is a need because this
situation does occur.

> > > I'd be happy to work on something like this as a contribution to your work.
> > > I would envision the alias entry not needing to have the typical [module] added
> > > to it because that will already be annotated on the actual symbol entry.  So,
> > > the alias could be extended to be something like:
> > >
> > > ffffffffc0533720 t floppy_open  [floppy]
> > > ffffffffc0533720 t floppy_open@floppy:drivers_block_floppy_c_3988
> > >
> > > (absence of a name: prefix to the path would indicate the symbol is not
> > >  associated with any module)
> > >
> > > Doing this is more realistic now as a result of the clean-up patches that
> > > Nick introduced, e.g.
> > >
> > > https://lore.kernel.org/lkml/20230302211759.30135-1-nick.alcock@xxxxxxxxxx/
> > >
> > 
> > Personally, I don't perceive any specific benefit in including the module name
> > as part of the decoration. Please don't get me wrong; I do recognize that it
> > enhances clarity in Nick's work.
> > In that context, a human can easily discern that a duplicated name originates
> > from a module, aiding in understanding which symbol they require as it is
> > already for duplicates coming from modules.
> > 
> > However, when it comes to my current implementation, I don't see a compelling
> > reason to include module name into the decoration I append to names aliases.
> > Please accept my apologies if I may not be taking into account any obvious use
> > cases, but as it stands, I don't find a strong rationale for incorporating
> > module names into the symbol decoration.
> >
> > In any case, as you've pointed out, duplicates can arise from names in code
> > that is not intended to be a module.
> > Therefore, relying solely on the module name would not fully address the
> > problem you initially aimed to solve.
> 
> >From my POV:
> 
> The source path and the line number is enough to distinguish duplicate
> symbols even in modules.
> 
> The added module name would just add extra complexity into the kernel
> and tools parsing and using the alias. The tracing tools would need to
> handle the source path and line number anyway for symbols duplicated
> within same module/vmlinux.
> 
> Adding module name for builtin modules might be misleading. It won't
> be clear which symbols are in vmlinux binary and which are in
> real modules.

The merit of having module names even for symbols that are in a module that is
built into the kernel image has been discussed before and there certainly is
a benefit for tracers.  Also, it is easy to avoid misleading information bu
making it easy to distinguish whether a module name is for a builtin module vs
a loaded module (e.g. Nick proposed {modname} vs [modname]). 

But I will defer that topic to a different mail thread, picking up from the
earlier discussions on this feature, and proposing a minimal patch to make
this data available in a way that is completely independent from this work
(and won't impact kallsyms data presentation in any way).

Thanks,
Kris