LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Nathan Chancellor <nathan@kernel.org>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kees Cook <keescook@chromium.org>,
	Fangrui Song <maskray@google.com>, "KE . LI" <like1@oppo.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Miguel Ojeda <ojeda@kernel.org>, Joe Perches <joe@perches.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Sami Tolvanen <samitolvanen@google.com>,
	Stephen Boyd <swboyd@chromium.org>,
	linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com
Subject: Re: [PATCH v3] kallsyms: strip LTO suffixes from static functions
Date: Mon, 28 Jun 2021 15:16:16 -0700	[thread overview]
Message-ID: <08a2e179-3f5f-639e-946f-54cd07ae12fa@kernel.org> (raw)
In-Reply-To: <CAKwvOdks4o8A1o7Q49Pa43ZyY5Zh6+s-u67FL-fp=16FP-Y7yw@mail.gmail.com>

On 6/28/2021 3:01 PM, Nick Desaulniers wrote:
> On Mon, Jun 28, 2021 at 2:20 PM Nathan Chancellor <nathan@kernel.org> wrote:
>>
>> On Mon, Jun 28, 2021 at 01:31:06PM -0700, Nick Desaulniers wrote:
>>> Similar to:
>>> commit 8b8e6b5d3b01 ("kallsyms: strip ThinLTO hashes from static
>>> functions")
>>>
>>> It's very common for compilers to modify the symbol name for static
>>> functions as part of optimizing transformations. That makes hooking
>>> static functions (that weren't inlined or DCE'd) with kprobes difficult.
>>>
>>> LLVM has yet another name mangling scheme used by thin LTO.  Strip off
>>> these suffixes so that we can continue to hook such static functions.
>>>
>>> Reported-by: KE.LI(Lieke) <like1@oppo.com>
>>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>>> ---
>>> Changes v2 -> V3:
>>> * Un-nest preprocessor checks, as per Nathan.
>>>
>>> Changes v1 -> v2:
>>> * Both mangling schemes can occur for thinLTO + CFI, this new scheme can
>>>    also occur for thinLTO without CFI. Split cleanup_symbol_name() into
>>>    two function calls.
>>> * Drop KE.LI's tested by tag.
>>> * Do not carry Fangrui's Reviewed by tag.
>>> * Drop the inline keyword; it is meaningless.
>>>   kernel/kallsyms.c | 32 +++++++++++++++++++++++++++++---
>>>   1 file changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
>>> index 4067564ec59f..143c69e7e75d 100644
>>> --- a/kernel/kallsyms.c
>>> +++ b/kernel/kallsyms.c
>>> @@ -171,6 +171,26 @@ static unsigned long kallsyms_sym_address(int idx)
>>>        return kallsyms_relative_base - 1 - kallsyms_offsets[idx];
>>>   }
>>>
>>> +#ifdef CONFIG_LTO_CLANG_THIN
>>> +/*
>>> + * LLVM appends a suffix for local variables that must be promoted to global
>>> + * scope as part of thin LTO. foo() becomes foo.llvm.974640843467629774. This
>>> + * can break hooking of static functions with kprobes.
>>> + */
>>> +static bool cleanup_symbol_name_thinlto(char *s)
>>> +{
>>> +     char *res;
>>> +
>>> +     res = strstr(s, ".llvm.");
>>> +     if (res)
>>> +             *res = '\0';
>>> +
>>> +     return res != NULL;
>>> +}
>>> +#else
>>> +static bool cleanup_symbol_name_thinlto(char *s) { return false; }
>>> +#endif /* CONFIG_LTO_CLANG_THIN */
>>> +
>>>   #if defined(CONFIG_CFI_CLANG) && defined(CONFIG_LTO_CLANG_THIN)
>>>   /*
>>>    * LLVM appends a hash to static function names when ThinLTO and CFI are
>>> @@ -178,7 +198,7 @@ static unsigned long kallsyms_sym_address(int idx)
>>>    * This causes confusion and potentially breaks user space tools, so we
>>>    * strip the suffix from expanded symbol names.
>>>    */
>>> -static inline bool cleanup_symbol_name(char *s)
>>> +static bool cleanup_symbol_name_thinlto_cfi(char *s)
>>>   {
>>>        char *res;
>>>
>>> @@ -189,8 +209,14 @@ static inline bool cleanup_symbol_name(char *s)
>>>        return res != NULL;
>>>   }
>>>   #else
>>> -static inline bool cleanup_symbol_name(char *s) { return false; }
>>> -#endif
>>> +static bool cleanup_symbol_name_thinlto_cfi(char *s) { return false; }
>>> +#endif /* CONFIG_CFI_CLANG && CONFIG_LTO_CLANG_THIN */
>>> +
>>> +static bool cleanup_symbol_name(char *s)
>>> +{
>>> +     return cleanup_symbol_name_thinlto(s) &&
>>> +             cleanup_symbol_name_thinlto_cfi(s);
>>> +}
>>>
>>>   /* Lookup the address for this symbol. Returns 0 if not found. */
>>>   unsigned long kallsyms_lookup_name(const char *name)
>>> --
>>> 2.32.0.93.g670b81a890-goog
>>>
>>
>> Is there any reason that we cannot eliminate the stubs and combine the
>> functions, or am I missing something? Completely untested diff.
>>
>> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
>> index c851ca0ed357..014b59ad68a3 100644
>> --- a/kernel/kallsyms.c
>> +++ b/kernel/kallsyms.c
>> @@ -161,26 +161,36 @@ static unsigned long kallsyms_sym_address(int idx)
>>          return kallsyms_relative_base - 1 - kallsyms_offsets[idx];
>>   }
>>
>> -#if defined(CONFIG_CFI_CLANG) && defined(CONFIG_LTO_CLANG_THIN)
>> -/*
>> - * LLVM appends a hash to static function names when ThinLTO and CFI are
>> - * both enabled, i.e. foo() becomes foo$707af9a22804d33c81801f27dcfe489b.
>> - * This causes confusion and potentially breaks user space tools, so we
>> - * strip the suffix from expanded symbol names.
>> - */
>> -static inline bool cleanup_symbol_name(char *s)
>> +static inline bool cleanup_symbol_name_thinlto(char *s)
> 
> Drop inline while touching this line.

Sure.

>>   {
>>          char *res;
>>
>> -       res = strrchr(s, '$');
>> +       if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN))
>> +               return false;
>> +
>> +       /*
>> +        * LLVM appends a suffix for local variables that must be promoted to global
>> +        * scope as part of ThinLTO. foo() becomes foo.llvm.974640843467629774. This
>> +        * can break hooking of static functions with kprobes.
>> +        */
>> +       res = strstr(s, ".llvm.");
>>          if (res)
>>                  *res = '\0';
> 
> Sure, this is nicer though within the true block we should `return
> true;` early rather than additionally check the $ mangling, I suppose.

I am not sure I follow? Are you talking about moving this into an else 
block in the if statement below this?

This should probably be two separate patches, with the first one 
eliminating the stub, dropping the inline, and renaming the function 
then the second patch do what this one does but I do not have a strong 
opinion.

I do not mind if you take ownership of this diff, with or without 
attribution.

>>
>> +       /*
>> +        * LLVM appends a hash to static function names when ThinLTO and CFI are
>> +        * both enabled, i.e. foo() becomes foo$707af9a22804d33c81801f27dcfe489b.
>> +        * This causes confusion and potentially breaks user space tools, so we
>> +        * strip the suffix from expanded symbol names.
>> +        */
>> +       if (IS_ENABLED(CONFIG_CFI_CLANG)) {
>> +               res = strrchr(s, '$');
>> +               if (res)
>> +                       *res = '\0';
>> +       }
>> +
>>          return res != NULL;
>>   }
>> -#else
>> -static inline bool cleanup_symbol_name(char *s) { return false; }
>> -#endif
>>
>>   /* Lookup the address for this symbol. Returns 0 if not found. */
>>   unsigned long kallsyms_lookup_name(const char *name)
>> @@ -195,7 +205,7 @@ unsigned long kallsyms_lookup_name(const char *name)
>>                  if (strcmp(namebuf, name) == 0)
>>                          return kallsyms_sym_address(i);
>>
>> -               if (cleanup_symbol_name(namebuf) && strcmp(namebuf, name) == 0)
>> +               if (cleanup_symbol_name_thinlto(namebuf) && strcmp(namebuf, name) == 0)
>>                          return kallsyms_sym_address(i);
>>          }
>>          return module_kallsyms_lookup_name(name);
> 
> 
> 

  reply	other threads:[~2021-06-28 22:16 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-22 18:38 [PATCH] " Nick Desaulniers
2021-06-22 20:18 ` Fangrui Song
2021-06-28 17:54   ` Nick Desaulniers
2021-06-28 18:20     ` Nick Desaulniers
2021-06-28 19:05   ` [PATCH v2] " Nick Desaulniers
2021-06-28 19:45     ` Nathan Chancellor
2021-06-28 20:31       ` [PATCH v3] " Nick Desaulniers
2021-06-28 21:19         ` Nathan Chancellor
2021-06-28 22:01           ` Nick Desaulniers
2021-06-28 22:16             ` Nathan Chancellor [this message]
2021-07-07 18:18               ` [PATCH v4] " Nick Desaulniers
2021-07-07 18:34                 ` Nathan Chancellor
2021-07-07 18:59                   ` Fāng-ruì Sòng
2021-08-06 16:20                 ` Sami Tolvanen
2021-10-01 19:58                   ` [PATCH v5] " Nick Desaulniers
2021-10-01 20:05                     ` Sami Tolvanen
2021-10-04 10:46                       ` Padmanabha Srinivasaiah

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=08a2e179-3f5f-639e-946f-54cd07ae12fa@kernel.org \
    --to=nathan@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=clang-built-linux@googlegroups.com \
    --cc=gustavoars@kernel.org \
    --cc=joe@perches.com \
    --cc=keescook@chromium.org \
    --cc=like1@oppo.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maskray@google.com \
    --cc=ndesaulniers@google.com \
    --cc=ojeda@kernel.org \
    --cc=rdunlap@infradead.org \
    --cc=samitolvanen@google.com \
    --cc=sfr@canb.auug.org.au \
    --cc=swboyd@chromium.org \
    --subject='Re: [PATCH v3] kallsyms: strip LTO suffixes from static functions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).