Re: [PATCH v2 2/3] kallsyms: Add APIs to match symbol without .XXXX suffix.

[Song Liu <songliubraving@meta.com>]

> On Aug 8, 2024, at 3:20 AM, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Fri 2024-08-02 14:08:34, Song Liu wrote:
>> With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names
>> to avoid duplication. This causes confusion with users of kallsyms.
>> On one hand, users like livepatch are required to match the symbols
>> exactly. On the other hand, users like kprobe would like to match to
>> original function names.
>> 
>> Solve this by splitting kallsyms APIs. Specifically, existing APIs now
>> should match the symbols exactly. Add two APIs that match only the part
>> without .XXXX suffix. Specifically, the following two APIs are added.
>> 
>> 1. kallsyms_lookup_name_without_suffix()
>> 2. kallsyms_on_each_match_symbol_without_suffix()
>> 
>> These APIs will be used by kprobe.
>> 
>> Also cleanup some code and update kallsyms_selftests accordingly.
>> 
>> --- a/kernel/kallsyms.c
>> +++ b/kernel/kallsyms.c
>> @@ -164,30 +164,27 @@ static void cleanup_symbol_name(char *s)
>> {
>> char *res;
>> 
>> - if (!IS_ENABLED(CONFIG_LTO_CLANG))
>> - return;
>> -
>> /*
>> * LLVM appends various suffixes for local functions and variables that
>> * must be promoted to global scope as part of LTO.  This can break
>> * hooking of static functions with kprobes. '.' is not a valid
>> - * character in an identifier in C. Suffixes only in LLVM LTO observed:
>> - * - foo.llvm.[0-9a-f]+
>> + * character in an identifier in C, so we can just remove the
>> + * suffix.
>> */
>> - res = strstr(s, ".llvm.");
>> + res = strstr(s, ".");
> 
> IMHO, we should not remove the suffixes like .constprop*, .part*,
> .isra*. They implement a special optimized variant of the function.
> It is not longer the original full-featured one.
> 
>> if (res)
>> *res = '\0';
>> 
>> return;
>> }
>> 
>> -static int compare_symbol_name(const char *name, char *namebuf)
>> +static int compare_symbol_name(const char *name, char *namebuf, bool exact_match)
>> {
>> - /* The kallsyms_seqs_of_names is sorted based on names after
>> - * cleanup_symbol_name() (see scripts/kallsyms.c) if clang lto is enabled.
>> - * To ensure correct bisection in kallsyms_lookup_names(), do
>> - * cleanup_symbol_name(namebuf) before comparing name and namebuf.
>> - */
>> + int ret = strcmp(name, namebuf);
>> +
>> + if (exact_match || !ret)
>> + return ret;
>> +
>> cleanup_symbol_name(namebuf);
>> return strcmp(name, namebuf);
>> }
>> @@ -204,13 +201,17 @@ static unsigned int get_symbol_seq(int index)
>> 
>> static int kallsyms_lookup_names(const char *name,
>> unsigned int *start,
>> - unsigned int *end)
>> + unsigned int *end,
>> + bool exact_match)
>> {
>> int ret;
>> int low, mid, high;
>> unsigned int seq, off;
>> char namebuf[KSYM_NAME_LEN];
>> 
>> + if (!IS_ENABLED(CONFIG_LTO_CLANG))
>> + exact_match = true;
> 
> IMHO, this is very a bad design. It causes that
> 
>     kallsyms_on_each_match_symbol_without_suffix(,,, false);
> 
> does not longer work as expected. It creates a hard to maintain
> code. The code does not do what it looks like.

Indeed. It actually caused issue with GCC-built kernel in my 
tests after submitting v2. 

Thanks,
Song