Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Tue, 10 Oct 2023 13:33:09 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Alessandro Carminati <alessandro.carminati@gmail.com>
Cc:     Kris Van Hees <kris.van.hees@oracle.com>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Daniel Bristot de Oliveira <bristot@kernel.org>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Masahiro Yamada <masahiroy@kernel.org>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Nicolas Schier <nicolas@fjasle.eu>,
        Alexander Lobakin <aleksander.lobakin@intel.com>,
        Nick Alcock <nick.alcock@oracle.com>,
        Eugene Loh <eugene.loh@oracle.com>,
        Francis Laniel <flaniel@linux.microsoft.com>,
        Viktor Malik <vmalik@redhat.com>, linux-kbuild@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH v5] scripts/link-vmlinux.sh: Add alias to duplicate
 symbols for kallsyms
Message-ID: <ZSU2dTTfLqIydSQU@alley>
References: <20230927173516.1456594-1-alessandro.carminati@gmail.com>
 <ZR7jIxh/VNT6tMVr@oracle.com>
 <CAPp5cGThN5bKsGAqiVwOV2w4tH1LgjS3eoAgucaRwty4d8JVSQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPp5cGThN5bKsGAqiVwOV2w4tH1LgjS3eoAgucaRwty4d8JVSQ@mail.gmail.com>
Precedence: bulk

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@oracle.com> 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.

> > 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@oracle.com/
> >
> 
> 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.

Best Regards,
Petr