linux-kernel - Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <AA015194-A0D0-426C-A3B7-11C5B7CA941A@fb.com>
Date: Fri, 9 Aug 2024 16:40:25 +0000
From: Song Liu <songliubraving@...a.com>
To: Alessandro Carminati <alessandro.carminati@...il.com>
CC: Petr Mladek <pmladek@...e.com>, Song Liu <songliubraving@...a.com>,
        Sami
 Tolvanen <samitolvanen@...gle.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>, Song Liu <song@...nel.org>,
        "live-patching@...r.kernel.org" <live-patching@...r.kernel.org>,
        LKML
	<linux-kernel@...r.kernel.org>,
        "linux-trace-kernel@...r.kernel.org"
	<linux-trace-kernel@...r.kernel.org>,
        Josh Poimboeuf <jpoimboe@...nel.org>, Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Joe Lawrence
	<joe.lawrence@...hat.com>,
        Nathan Chancellor <nathan@...nel.org>,
        "morbo@...gle.com" <morbo@...gle.com>,
        Justin Stitt <justinstitt@...gle.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Leizhen <thunder.leizhen@...wei.com>,
        "kees@...nel.org" <kees@...nel.org>,
        Kernel Team <kernel-team@...a.com>,
        Matthew Maurer <mmaurer@...gle.com>
Subject: Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols
 without .XXX suffix

Hi Alessandro,

> On Aug 8, 2024, at 11:20 PM, Alessandro Carminati <alessandro.carminati@...il.com> wrote:
> 
> Hello,
> sorry to join late at the party.
> 
> Il giorno gio 8 ago 2024 alle ore 11:59 Petr Mladek <pmladek@...e.com>
> ha scritto:
>> 
>> On Wed 2024-08-07 20:48:48, Song Liu wrote:
>>> 
>>> 
>>>> On Aug 7, 2024, at 8:33 AM, Sami Tolvanen <samitolvanen@...gle.com> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>>>>> 
>>>>> On Wed, 7 Aug 2024 00:19:20 +0000
>>>>> Song Liu <songliubraving@...a.com> wrote:
>>>>> 
>>>>>> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part
>>>>>> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are
>>>>>> undoing the change by Sami in [1], and thus may break some tracing tools.
>>>>> 
>>>>> What tracing tools may be broke and why?
>>>> 
>>>> This was a few years ago when we were first adding LTO support, but
>>>> the unexpected suffixes in tracing output broke systrace in Android,
>>>> presumably because the tools expected to find specific function names
>>>> without suffixes. I'm not sure if systrace would still be a problem
>>>> today, but other tools might still make assumptions about the function
>>>> name format. At the time, we decided to filter out the suffixes in all
>>>> user space visible output to avoid these issues.
>>>> 
>>>>> For this suffix problem, I would like to add another patch to allow probing on
>>>>> suffixed symbols. (It seems suffixed symbols are not available at this point)
>>>>> 
>>>>> The problem is that the suffixed symbols maybe a "part" of the original function,
>>>>> thus user has to carefully use it.
>>>>> 
>>>>>> 
>>>>>> Sami, could you please share your thoughts on this?
>>>>> 
>>>>> Sami, I would like to know what problem you have on kprobes.
>>>> 
>>>> The reports we received back then were about registering kprobes for
>>>> static functions, which obviously failed if the compiler added a
>>>> suffix to the function name. This was more of a problem with ThinLTO
>>>> and Clang CFI at the time because the compiler used to rename _all_
>>>> static functions, but one can obviously run into the same issue with
>>>> just LTO.
>>> 
>>> I think newer LLVM/clang no longer add suffixes to all static functions
>>> with LTO and CFI. So this may not be a real issue any more?
>>> 
>>> If we still need to allow tracing without suffix, I think the approach
>>> in this patch set is correct (sort syms based on full name,
>> 
>> Yes, we should allow to find the symbols via the full name, definitely.
>> 
>>> remove suffixes in special APIs during lookup).
>> 
>> Just an idea. Alternative solution would be to make make an alias
>> without the suffix when there is only one symbol with the same
>> name.
>> 
>> It would be complementary with the patch adding aliases for symbols
>> with the same name, see
>> https://lore.kernel.org/r/20231204214635.2916691-1-alessandro.carminati@gmail.com
>> 
>> I would allow to find the symbols with and without the suffix using
>> a single API.
> 
> kas_alias isn't handling LTO as effectively as it should.
> This is something I plan to address in the next patch version.
> Introducing aliases is the best approach I found to preserve current
> tools behavior while adding this new feature.
> While I believe it will deliver the promised benefits, there is a trade-off,
> particularly affecting features like live patching, which rely on handling
> duplicate symbols.
> For instance, kallsyms_lookup_names typically returns the last occurrence
> of a symbol when the end argument is not NULL, but introducing aliases
> disrupts this behavior.

Do you think with v3 of this set [1], live patching should be fine? The
idea is to let kallsyms_lookup_names() do full name match, then live
patching can find the right symbol with symbol name + old_sympos. 
Did I miss some cases?

> I'm working on a solution to manage duplicate symbols, ensuring compatibility
> with both LTO and kallsyms_lookup_names.

Thanks,
Song

[1] https://lore.kernel.org/live-patching/20240807220513.3100483-1-song@kernel.org/T/#u