* [PATCH v2] kallsyms: strip ThinLTO postfix ".cfi_jt" [not found] <20210727131853.GA18032@pswork> @ 2021-07-27 14:06 ` Padmanabha Srinivasaiah 2021-07-28 20:57 ` Sami Tolvanen 0 siblings, 1 reply; 11+ messages in thread From: Padmanabha Srinivasaiah @ 2021-07-27 14:06 UTC (permalink / raw) To: jeyu, keescook Cc: samitolvanen, treasure4paddy, Nathan Chancellor, Nick Desaulniers, Miroslav Benes, Petr Mladek, Miguel Ojeda, Joe Perches, Stephen Boyd, Gustavo A. R. Silva, linux-kernel, clang-built-linux Clang ThinLTO adds a postfix ".cfi_jt" to a symbols of extern functions. For example this breaks syscall tracer that doesn't expect such postfix, so strip out the postfix from the output. Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> --- Change in v2: - Use existing routine in kallsyms to strip postfix ".cfi_jt" from extern function name. - Modified the commit message accordingly kernel/kallsyms.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 0ba87982d017..e9148626ae6c 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -166,16 +166,20 @@ static unsigned long kallsyms_sym_address(int 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. + * LLVM appends a hash to static function names and just ".cfi_jt" postfix + * for non-static functions when both ThinLTO and CFI are enabled, + * i.e. for example foo() becomes foo$707af9a22804d33c81801f27dcfe489b. + * This causes confusion and potentially breaks user space tools and + * built-in components, so we strip the suffix from expanded symbol names. */ static inline bool cleanup_symbol_name(char *s) { char *res; res = strrchr(s, '$'); + if (!res) + res = strstr(s, ".cfi_jt"); + if (res) *res = '\0'; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] kallsyms: strip ThinLTO postfix ".cfi_jt" 2021-07-27 14:06 ` [PATCH v2] kallsyms: strip ThinLTO postfix ".cfi_jt" Padmanabha Srinivasaiah @ 2021-07-28 20:57 ` Sami Tolvanen 2021-07-29 15:29 ` Padmanabha Srinivasaiah 2021-07-29 20:53 ` [PATCH v3] kallsyms: strip CLANG CFI " Padmanabha Srinivasaiah 0 siblings, 2 replies; 11+ messages in thread From: Sami Tolvanen @ 2021-07-28 20:57 UTC (permalink / raw) To: Padmanabha Srinivasaiah Cc: Jessica Yu, Kees Cook, Nathan Chancellor, Nick Desaulniers, Miroslav Benes, Petr Mladek, Miguel Ojeda, Joe Perches, Stephen Boyd, Gustavo A. R. Silva, LKML, clang-built-linux Hi, On Tue, Jul 27, 2021 at 7:07 AM Padmanabha Srinivasaiah <treasure4paddy@gmail.com> wrote: > > Clang ThinLTO adds a postfix ".cfi_jt" to a symbols of extern functions. These symbols are added with CONFIG_CFI_CLANG no matter which LTO mode is selected, so talking about ThinLTO here isn't quite correct. > For example this breaks syscall tracer that doesn't expect such postfix, > so strip out the postfix from the output. > > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> > --- > Change in v2: > - Use existing routine in kallsyms to strip postfix ".cfi_jt" from > extern function name. > - Modified the commit message accordingly > > kernel/kallsyms.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index 0ba87982d017..e9148626ae6c 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -166,16 +166,20 @@ static unsigned long kallsyms_sym_address(int 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. > + * LLVM appends a hash to static function names and just ".cfi_jt" postfix > + * for non-static functions when both ThinLTO and CFI are enabled, Functions aren't technically speaking renamed to add a .cfi_jt postfix. Instead, these are separate symbols that point to the CFI jump table. Perhaps the comment should just say that we want to strip .cfi_jt from CFI jump table symbols? > + * i.e. for example foo() becomes foo$707af9a22804d33c81801f27dcfe489b. > + * This causes confusion and potentially breaks user space tools and > + * built-in components, so we strip the suffix from expanded symbol names. > */ > static inline bool cleanup_symbol_name(char *s) > { > char *res; > > res = strrchr(s, '$'); > + if (!res) > + res = strstr(s, ".cfi_jt"); > + > if (res) > *res = '\0'; This looks otherwise fine to me, but it's going to conflict with Nick's earlier patch: https://lore.kernel.org/lkml/20210707181814.365496-1-ndesaulniers@google.com/ Could you please rebase this on top of that, and take into account that we should do this when CONFIG_LTO_CLANG is enabled, not only with LTO_CLANG_THIN? Sami ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] kallsyms: strip ThinLTO postfix ".cfi_jt" 2021-07-28 20:57 ` Sami Tolvanen @ 2021-07-29 15:29 ` Padmanabha Srinivasaiah 2021-07-29 20:53 ` [PATCH v3] kallsyms: strip CLANG CFI " Padmanabha Srinivasaiah 1 sibling, 0 replies; 11+ messages in thread From: Padmanabha Srinivasaiah @ 2021-07-29 15:29 UTC (permalink / raw) To: Sami Tolvanen Cc: Jessica Yu, Kees Cook, Nathan Chancellor, Nick Desaulniers, Miroslav Benes, Petr Mladek, Miguel Ojeda, Joe Perches, Stephen Boyd, Gustavo A. R. Silva, LKML, clang-built-linux On Wed, Jul 28, 2021 at 01:57:21PM -0700, Sami Tolvanen wrote: > Hi, > > On Tue, Jul 27, 2021 at 7:07 AM Padmanabha Srinivasaiah > <treasure4paddy@gmail.com> wrote: > > > > Clang ThinLTO adds a postfix ".cfi_jt" to a symbols of extern functions. > > These symbols are added with CONFIG_CFI_CLANG no matter which LTO mode > is selected, so talking about ThinLTO here isn't quite correct. > Yes, checked irrespective of the LTO mode choosen ".cfi_jt" postfix is added with CONFIG_CFI_CLANG flag. Thanks for correcting out, will make neccessary changes. > > For example this breaks syscall tracer that doesn't expect such postfix, > > so strip out the postfix from the output. > > > > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> > > --- > > Change in v2: > > - Use existing routine in kallsyms to strip postfix ".cfi_jt" from > > extern function name. > > - Modified the commit message accordingly > > > > kernel/kallsyms.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > > index 0ba87982d017..e9148626ae6c 100644 > > --- a/kernel/kallsyms.c > > +++ b/kernel/kallsyms.c > > @@ -166,16 +166,20 @@ static unsigned long kallsyms_sym_address(int 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. > > + * LLVM appends a hash to static function names and just ".cfi_jt" postfix > > + * for non-static functions when both ThinLTO and CFI are enabled, > > Functions aren't technically speaking renamed to add a .cfi_jt > postfix. Instead, these are separate symbols that point to the CFI > jump table. Perhaps the comment should just say that we want to strip > .cfi_jt from CFI jump table symbols? > Agree, in jest modified existing comment. Will address same. > > + * i.e. for example foo() becomes foo$707af9a22804d33c81801f27dcfe489b. > > + * This causes confusion and potentially breaks user space tools and > > + * built-in components, so we strip the suffix from expanded symbol names. > > */ > > static inline bool cleanup_symbol_name(char *s) > > { > > char *res; > > > > res = strrchr(s, '$'); > > + if (!res) > > + res = strstr(s, ".cfi_jt"); > > + > > if (res) > > *res = '\0'; > > This looks otherwise fine to me, but it's going to conflict with > Nick's earlier patch: > > https://lore.kernel.org/lkml/20210707181814.365496-1-ndesaulniers@google.com/ > > Could you please rebase this on top of that, and take into account > that we should do this when CONFIG_LTO_CLANG is enabled, not only with > LTO_CLANG_THIN? > Thanks Sami for pointing out the link, will rebase and refactor the change. > Sami ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] kallsyms: strip CLANG CFI postfix ".cfi_jt" 2021-07-28 20:57 ` Sami Tolvanen 2021-07-29 15:29 ` Padmanabha Srinivasaiah @ 2021-07-29 20:53 ` Padmanabha Srinivasaiah 2021-08-03 16:28 ` Sami Tolvanen 1 sibling, 1 reply; 11+ messages in thread From: Padmanabha Srinivasaiah @ 2021-07-29 20:53 UTC (permalink / raw) To: jeyu, keescook, nathan, ndesaulniers Cc: samitolvanen, treasure4paddy, Miroslav Benes, Stephen Boyd, Gustavo A. R. Silva, Joe Perches, linux-kernel, clang-built-linux Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions. For example this breaks syscall tracer that doesn't expect such postfix, so strip out the postfix from the expanded symbol. Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> --- Change in v3: - Modified commit message to indicate fix is for Clang CFI postfix - Rebased on recent patch from ndesaulniers@google.com. - Fix is enabled even for CONFIG_LTO_CLANG Change in v2: - Use existing routine in kallsyms to strip postfix ".cfi_jt" from extern function name. - Modified the commit message accordingly kernel/kallsyms.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 5cabe4dd3ff4..67d015854cbd 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -174,7 +174,8 @@ static bool cleanup_symbol_name(char *s) * foo.llvm.974640843467629774. This can break hooking of static * functions with kprobes. */ - if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN)) + if (!(IS_ENABLED(CONFIG_LTO_CLANG) || + IS_ENABLED(CONFIG_LTO_CLANG_THIN))) return false; res = strstr(s, ".llvm."); @@ -184,16 +185,24 @@ static bool cleanup_symbol_name(char *s) } /* - * 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. + * LLVM appends a hash to static function names when both + * (Thin/FULL) LTO and CFI are enabled, i.e. foo() becomes + * foo$707af9a22804d33c81801f27dcfe489b. + * + * In case of non static function symbol <funcsym>, + * the local jump table will have entry as <funcsym>.cfi_jt. + * + * This causes confusion and potentially breaks + * user space tools and some built-in components. + * So we strip the suffix from expanded symbol names. */ if (!IS_ENABLED(CONFIG_CFI_CLANG)) return false; res = strrchr(s, '$'); + if (!res) + res = strstr(s, ".cfi_jt"); + if (res) { *res = '\0'; return true; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] kallsyms: strip CLANG CFI postfix ".cfi_jt" 2021-07-29 20:53 ` [PATCH v3] kallsyms: strip CLANG CFI " Padmanabha Srinivasaiah @ 2021-08-03 16:28 ` Sami Tolvanen 2021-08-04 17:17 ` Padmanabha Srinivasaiah 2021-08-05 23:27 ` [PATCH v4] " Padmanabha Srinivasaiah 0 siblings, 2 replies; 11+ messages in thread From: Sami Tolvanen @ 2021-08-03 16:28 UTC (permalink / raw) To: Padmanabha Srinivasaiah Cc: Jessica Yu, Kees Cook, Nathan Chancellor, Nick Desaulniers, Miroslav Benes, Stephen Boyd, Gustavo A. R. Silva, Joe Perches, LKML, clang-built-linux Hi, On Thu, Jul 29, 2021 at 1:54 PM Padmanabha Srinivasaiah <treasure4paddy@gmail.com> wrote: > > Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions. > For example this breaks syscall tracer that doesn't expect such postfix, > so strip out the postfix from the expanded symbol. > > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> > --- > > Change in v3: > - Modified commit message to indicate fix is for Clang CFI postfix > - Rebased on recent patch from ndesaulniers@google.com. > - Fix is enabled even for CONFIG_LTO_CLANG > > Change in v2: > - Use existing routine in kallsyms to strip postfix ".cfi_jt" from > extern function name. > - Modified the commit message accordingly > > kernel/kallsyms.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index 5cabe4dd3ff4..67d015854cbd 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -174,7 +174,8 @@ static bool cleanup_symbol_name(char *s) > * foo.llvm.974640843467629774. This can break hooking of static > * functions with kprobes. > */ > - if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN)) > + if (!(IS_ENABLED(CONFIG_LTO_CLANG) || > + IS_ENABLED(CONFIG_LTO_CLANG_THIN))) This is redundant. LTO_CLANG is selected for both LTO modes, so there's no need to also check for LTO_CLANG_THIN here. > return false; > > res = strstr(s, ".llvm."); However, we should probably check for ".llvm." only with LTO_CLANG_THIN. > @@ -184,16 +185,24 @@ static bool cleanup_symbol_name(char *s) > } > > /* > - * 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. > + * LLVM appends a hash to static function names when both > + * (Thin/FULL) LTO and CFI are enabled, i.e. foo() becomes > + * foo$707af9a22804d33c81801f27dcfe489b. That's not quite right, the hash is only appended with ThinLTO. I would leave this comment untouched. > + * > + * In case of non static function symbol <funcsym>, > + * the local jump table will have entry as <funcsym>.cfi_jt. > + * > + * This causes confusion and potentially breaks > + * user space tools and some built-in components. > + * So we strip the suffix from expanded symbol names. > */ > if (!IS_ENABLED(CONFIG_CFI_CLANG)) > return false; > > res = strrchr(s, '$'); > + if (!res) > + res = strstr(s, ".cfi_jt"); And add a comment about stripping .cfi_jt from jump table symbols before this part. > + > if (res) { > *res = '\0'; > return true; > -- > 2.17.1 > Sami ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] kallsyms: strip CLANG CFI postfix ".cfi_jt" 2021-08-03 16:28 ` Sami Tolvanen @ 2021-08-04 17:17 ` Padmanabha Srinivasaiah 2021-08-05 23:27 ` [PATCH v4] " Padmanabha Srinivasaiah 1 sibling, 0 replies; 11+ messages in thread From: Padmanabha Srinivasaiah @ 2021-08-04 17:17 UTC (permalink / raw) To: Sami Tolvanen Cc: Jessica Yu, Kees Cook, Nathan Chancellor, Nick Desaulniers, Miroslav Benes, Stephen Boyd, Gustavo A. R. Silva, Joe Perches, LKML, clang-built-linux On Tue, Aug 03, 2021 at 09:28:23AM -0700, Sami Tolvanen wrote: > Hi, > > On Thu, Jul 29, 2021 at 1:54 PM Padmanabha Srinivasaiah > <treasure4paddy@gmail.com> wrote: > > > > Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions. > > For example this breaks syscall tracer that doesn't expect such postfix, > > so strip out the postfix from the expanded symbol. > > > > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> > > --- > > > > Change in v3: > > - Modified commit message to indicate fix is for Clang CFI postfix > > - Rebased on recent patch from ndesaulniers@google.com. > > - Fix is enabled even for CONFIG_LTO_CLANG > > > > Change in v2: > > - Use existing routine in kallsyms to strip postfix ".cfi_jt" from > > extern function name. > > - Modified the commit message accordingly > > > > kernel/kallsyms.c | 21 +++++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > > index 5cabe4dd3ff4..67d015854cbd 100644 > > --- a/kernel/kallsyms.c > > +++ b/kernel/kallsyms.c > > @@ -174,7 +174,8 @@ static bool cleanup_symbol_name(char *s) > > * foo.llvm.974640843467629774. This can break hooking of static > > * functions with kprobes. > > */ > > - if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN)) > > + if (!(IS_ENABLED(CONFIG_LTO_CLANG) || > > + IS_ENABLED(CONFIG_LTO_CLANG_THIN))) > > This is redundant. LTO_CLANG is selected for both LTO modes, so > there's no need to also check for LTO_CLANG_THIN here. > As my setup is little endian, couldn't verify for below condition and was the reason to add such check. Sure will remove it. " select ARCH_SUPPORTS_LTO_CLANG if CPU_LITTLE_ENDIAN select ARCH_SUPPORTS_LTO_CLANG_THIN" > > return false; > > > > res = strstr(s, ".llvm."); > > However, we should probably check for ".llvm." only with LTO_CLANG_THIN. > Thank you for the input, will regenrate the patch with suggested check > > @@ -184,16 +185,24 @@ static bool cleanup_symbol_name(char *s) > > } > > > > /* > > - * 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. > > + * LLVM appends a hash to static function names when both > > + * (Thin/FULL) LTO and CFI are enabled, i.e. foo() becomes > > + * foo$707af9a22804d33c81801f27dcfe489b. > > That's not quite right, the hash is only appended with ThinLTO. I > would leave this comment untouched. > sure, will revert it. > > + * > > + * In case of non static function symbol <funcsym>, > > + * the local jump table will have entry as <funcsym>.cfi_jt. > > + * > > + * This causes confusion and potentially breaks > > + * user space tools and some built-in components. > > + * So we strip the suffix from expanded symbol names. > > */ > > if (!IS_ENABLED(CONFIG_CFI_CLANG)) > > return false; > > > > res = strrchr(s, '$'); > > + if (!res) > > + res = strstr(s, ".cfi_jt"); > > And add a comment about stripping .cfi_jt from jump table symbols > before this part. > sure, will add it > > + > > if (res) { > > *res = '\0'; > > return true; > > -- > > 2.17.1 > > > > Sami ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4] kallsyms: strip CLANG CFI postfix ".cfi_jt" 2021-08-03 16:28 ` Sami Tolvanen 2021-08-04 17:17 ` Padmanabha Srinivasaiah @ 2021-08-05 23:27 ` Padmanabha Srinivasaiah 2021-08-12 23:05 ` Sami Tolvanen 1 sibling, 1 reply; 11+ messages in thread From: Padmanabha Srinivasaiah @ 2021-08-05 23:27 UTC (permalink / raw) To: Jessica Yu, Kees Cook, nathan, ndesaulniers, samitolvanen Cc: treasure4paddy, Miroslav Benes, Stephen Boyd, Gustavo A. R. Silva, Joe Perches, linux-kernel, clang-built-linux Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions. For e.g. this breaks syscall tracer that doesn't expect such postfix, so strip out the postfix from the expanded symbol. Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> --- Change in v4: - Remove redundant check; irrespective of LTO type (THIN/FULL), LTO_CLANG will be always enabled. Hence will be used as entry flag to check various postfix patterns. - And prior to stripping postfix ".cfi_jt", added a comment to justify why we are doing so. Change in v3: - Modified commit message to indicate fix is for Clang CFI postfix - Rebased on recent patch from ndesaulniers@google.com. https://lore.kernel.org/lkml/ 20210707181814.365496-1-ndesaulniers@google.com/#t - Fix is enabled even for CONFIG_LTO_CLANG Change in v2: - Use existing routine in kallsyms to strip postfix ".cfi_jt" from extern function name. - Modified the commit message accordingly kernel/kallsyms.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 5cabe4dd3ff4..1b40bcf20fe6 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -174,13 +174,15 @@ static bool cleanup_symbol_name(char *s) * foo.llvm.974640843467629774. This can break hooking of static * functions with kprobes. */ - if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN)) + if (!IS_ENABLED(CONFIG_LTO_CLANG)) return false; - res = strstr(s, ".llvm."); - if (res) { - *res = '\0'; - return true; + if (IS_ENABLED(CONFIG_LTO_CLANG_THIN)) { + res = strstr(s, ".llvm."); + if (res) { + *res = '\0'; + return true; + } } /* @@ -194,6 +196,17 @@ static bool cleanup_symbol_name(char *s) return false; res = strrchr(s, '$'); + if (!res) { + /* + * In case of non static function symbol <funcsym>, + * the local jump table will have entry as <funcsym>.cfi_jt. + * + * Such expansion breaks some built-in components, + * e.g. syscall tracer. Hence remove postfix ".cfi_jt". + */ + res = strstr(s, ".cfi_jt"); + } + if (res) { *res = '\0'; return true; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4] kallsyms: strip CLANG CFI postfix ".cfi_jt" 2021-08-05 23:27 ` [PATCH v4] " Padmanabha Srinivasaiah @ 2021-08-12 23:05 ` Sami Tolvanen 2021-08-14 12:15 ` Padmanabha Srinivasaiah 2021-08-14 12:42 ` [PATCH v5] " Padmanabha Srinivasaiah 0 siblings, 2 replies; 11+ messages in thread From: Sami Tolvanen @ 2021-08-12 23:05 UTC (permalink / raw) To: Padmanabha Srinivasaiah, Nick Desaulniers Cc: Jessica Yu, Kees Cook, Nathan Chancellor, Miroslav Benes, Stephen Boyd, Gustavo A. R. Silva, Joe Perches, LKML, clang-built-linux On Thu, Aug 5, 2021 at 4:28 PM Padmanabha Srinivasaiah <treasure4paddy@gmail.com> wrote: > > Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions. > For e.g. this breaks syscall tracer that doesn't expect such postfix, > so strip out the postfix from the expanded symbol. > > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> > --- > Change in v4: > - Remove redundant check; irrespective of LTO type (THIN/FULL), > LTO_CLANG will be always enabled. Hence will be used as entry flag > to check various postfix patterns. > - And prior to stripping postfix ".cfi_jt", added a comment to > justify why we are doing so. > > Change in v3: > - Modified commit message to indicate fix is for Clang CFI postfix > - Rebased on recent patch from ndesaulniers@google.com. > https://lore.kernel.org/lkml/ > 20210707181814.365496-1-ndesaulniers@google.com/#t > - Fix is enabled even for CONFIG_LTO_CLANG > > Change in v2: > - Use existing routine in kallsyms to strip postfix ".cfi_jt" from > extern function name. > - Modified the commit message accordingly > > kernel/kallsyms.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index 5cabe4dd3ff4..1b40bcf20fe6 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -174,13 +174,15 @@ static bool cleanup_symbol_name(char *s) > * foo.llvm.974640843467629774. This can break hooking of static > * functions with kprobes. > */ > - if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN)) > + if (!IS_ENABLED(CONFIG_LTO_CLANG)) > return false; > > - res = strstr(s, ".llvm."); > - if (res) { > - *res = '\0'; > - return true; > + if (IS_ENABLED(CONFIG_LTO_CLANG_THIN)) { > + res = strstr(s, ".llvm."); > + if (res) { > + *res = '\0'; > + return true; > + } > } I confirmed that LLVM renames these also with full LTO, so the config check can be dropped here. > > /* > @@ -194,6 +196,17 @@ static bool cleanup_symbol_name(char *s) > return false; > > res = strrchr(s, '$'); > + if (!res) { > + /* > + * In case of non static function symbol <funcsym>, > + * the local jump table will have entry as <funcsym>.cfi_jt. > + * > + * Such expansion breaks some built-in components, > + * e.g. syscall tracer. Hence remove postfix ".cfi_jt". > + */ > + res = strstr(s, ".cfi_jt"); > + } > + > if (res) { > *res = '\0'; > return true; Otherwise, the logic looks pretty good to me. Nick, are you planning to resend your earlier patch? Should this be just folded into the next version? Sami ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] kallsyms: strip CLANG CFI postfix ".cfi_jt" 2021-08-12 23:05 ` Sami Tolvanen @ 2021-08-14 12:15 ` Padmanabha Srinivasaiah 2021-08-14 12:42 ` [PATCH v5] " Padmanabha Srinivasaiah 1 sibling, 0 replies; 11+ messages in thread From: Padmanabha Srinivasaiah @ 2021-08-14 12:15 UTC (permalink / raw) To: Sami Tolvanen Cc: Nick Desaulniers, Jessica Yu, Kees Cook, Nathan Chancellor, Miroslav Benes, Stephen Boyd, Gustavo A. R. Silva, Joe Perches, LKML, clang-built-linux On Thu, Aug 12, 2021 at 04:05:25PM -0700, Sami Tolvanen wrote: > On Thu, Aug 5, 2021 at 4:28 PM Padmanabha Srinivasaiah > <treasure4paddy@gmail.com> wrote: > > > > Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions. > > For e.g. this breaks syscall tracer that doesn't expect such postfix, > > so strip out the postfix from the expanded symbol. > > > > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> > > --- > > Change in v4: > > - Remove redundant check; irrespective of LTO type (THIN/FULL), > > LTO_CLANG will be always enabled. Hence will be used as entry flag > > to check various postfix patterns. > > - And prior to stripping postfix ".cfi_jt", added a comment to > > justify why we are doing so. > > > > Change in v3: > > - Modified commit message to indicate fix is for Clang CFI postfix > > - Rebased on recent patch from ndesaulniers@google.com. > > https://lore.kernel.org/lkml/ > > 20210707181814.365496-1-ndesaulniers@google.com/#t > > - Fix is enabled even for CONFIG_LTO_CLANG > > > > Change in v2: > > - Use existing routine in kallsyms to strip postfix ".cfi_jt" from > > extern function name. > > - Modified the commit message accordingly > > > > kernel/kallsyms.c | 23 ++++++++++++++++++----- > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > > index 5cabe4dd3ff4..1b40bcf20fe6 100644 > > --- a/kernel/kallsyms.c > > +++ b/kernel/kallsyms.c > > @@ -174,13 +174,15 @@ static bool cleanup_symbol_name(char *s) > > * foo.llvm.974640843467629774. This can break hooking of static > > * functions with kprobes. > > */ > > - if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN)) > > + if (!IS_ENABLED(CONFIG_LTO_CLANG)) > > return false; > > > > - res = strstr(s, ".llvm."); > > - if (res) { > > - *res = '\0'; > > - return true; > > + if (IS_ENABLED(CONFIG_LTO_CLANG_THIN)) { > > + res = strstr(s, ".llvm."); > > + if (res) { > > + *res = '\0'; > > + return true; > > + } > > } > > I confirmed that LLVM renames these also with full LTO, so the config > check can be dropped here. > Thank you sami for the input I missread your earlier review commit, will re-genrate the patch > > > > /* > > @@ -194,6 +196,17 @@ static bool cleanup_symbol_name(char *s) > > return false; > > > > res = strrchr(s, '$'); > > + if (!res) { > > + /* > > + * In case of non static function symbol <funcsym>, > > + * the local jump table will have entry as <funcsym>.cfi_jt. > > + * > > + * Such expansion breaks some built-in components, > > + * e.g. syscall tracer. Hence remove postfix ".cfi_jt". > > + */ > > + res = strstr(s, ".cfi_jt"); > > + } > > + > > if (res) { > > *res = '\0'; > > return true; > > Otherwise, the logic looks pretty good to me. Nick, are you planning > to resend your earlier patch? Should this be just folded into the next > version? > > Sami ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5] kallsyms: strip CLANG CFI postfix ".cfi_jt" 2021-08-12 23:05 ` Sami Tolvanen 2021-08-14 12:15 ` Padmanabha Srinivasaiah @ 2021-08-14 12:42 ` Padmanabha Srinivasaiah 2021-10-01 18:29 ` Nick Desaulniers 1 sibling, 1 reply; 11+ messages in thread From: Padmanabha Srinivasaiah @ 2021-08-14 12:42 UTC (permalink / raw) To: jeyu, keescook, nathan, ndesaulniers, samitolvanen Cc: treasure4paddy, Miroslav Benes, Stephen Boyd, Joe Perches, Gustavo A. R. Silva, linux-kernel, clang-built-linux Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions. For e.g. this breaks syscall tracer that doesn't expect such postfix, so strip out the postfix from the expanded symbol. Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> --- Change in v5: - Also remove explcit config check for postfix ".llvm." as LLVM renames even in full LTO case Change in v4: - Remove redundant check; irrespective of LTO type (THIN/FULL), LTO_CLANG will be always enabled. Hence will be used as entry flag to check various postfix patterns. - And prior to stripping postfix ".cfi_jt", added a comment to justify why we are doing so. Change in v3: - Modified commit message to indicate fix is for Clang CFI postfix - Rebased on recent patch from ndesaulniers@google.com. https://lore.kernel.org/lkml/ 20210707181814.365496-1-ndesaulniers@google.com/#t - Fix is enabled even for CONFIG_LTO_CLANG Change in v2: - Use existing routine in kallsyms to strip postfix ".cfi_jt" from extern function name. - Modified the commit message accordingly kernel/kallsyms.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 5cabe4dd3ff4..c8ef618e2a71 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -174,7 +174,7 @@ static bool cleanup_symbol_name(char *s) * foo.llvm.974640843467629774. This can break hooking of static * functions with kprobes. */ - if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN)) + if (!IS_ENABLED(CONFIG_LTO_CLANG)) return false; res = strstr(s, ".llvm."); @@ -194,6 +194,17 @@ static bool cleanup_symbol_name(char *s) return false; res = strrchr(s, '$'); + if (!res) { + /* + * In case of non static function symbol <funcsym>, + * the local jump table will have entry as <funcsym>.cfi_jt. + * + * Such expansion breaks some built-in components, + * e.g. syscall tracer. Hence remove postfix ".cfi_jt". + */ + res = strstr(s, ".cfi_jt"); + } + if (res) { *res = '\0'; return true; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5] kallsyms: strip CLANG CFI postfix ".cfi_jt" 2021-08-14 12:42 ` [PATCH v5] " Padmanabha Srinivasaiah @ 2021-10-01 18:29 ` Nick Desaulniers 0 siblings, 0 replies; 11+ messages in thread From: Nick Desaulniers @ 2021-10-01 18:29 UTC (permalink / raw) To: Padmanabha Srinivasaiah Cc: jeyu, keescook, nathan, samitolvanen, Miroslav Benes, Stephen Boyd, Joe Perches, Gustavo A. R. Silva, linux-kernel, clang-built-linux, Fangrui Song On Sat, Aug 14, 2021 at 5:43 AM Padmanabha Srinivasaiah <treasure4paddy@gmail.com> wrote: > > Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions. > For e.g. this breaks syscall tracer that doesn't expect such postfix, > so strip out the postfix from the expanded symbol. $ llvm-nm vmlinux | grep -e 'T ' -e 't ' ... ffff800010cce56c t xhci_map_urb_for_dma ffff800010cce56c t xhci_map_urb_for_dma.86d975cb70058c10e8ae4c2960627264 ffff800011227f28 t xhci_map_urb_for_dma.86d975cb70058c10e8ae4c2960627264.cfi_jt ... so I think it's not just the `.cfi_jt` that we want to truncate. Sami asked me about sending a v5 for https://lore.kernel.org/lkml/20210707181814.365496-1-ndesaulniers@google.com/; I was looking to rebase your v5 on my patch, but Sami also noted that here https://lore.kernel.org/lkml/CABCJKue5Ay6_+8sibzh5wRh3gPzV1g72gJ9m2ot4E1ezj8bpHA@mail.gmail.com/ that the separator was changed from $ to . for other CFI symbols in clang-13. So I think I'm going to "combine" our patches to truncate after the first `.` as long as CONFIG_LTO_CLANG is enabled, but still check for `$` for clang-12 for CONFIG_CFI_CLANG. I will credit you with the Suggested-by tag; stay tuned. > > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com> > --- > Change in v5: > - Also remove explcit config check for postfix ".llvm." as LLVM > renames even in full LTO case > > Change in v4: > - Remove redundant check; irrespective of LTO type (THIN/FULL), > LTO_CLANG will be always enabled. Hence will be used as entry flag > to check various postfix patterns. > - And prior to stripping postfix ".cfi_jt", added a comment to > justify why we are doing so. > > Change in v3: > - Modified commit message to indicate fix is for Clang CFI postfix > - Rebased on recent patch from ndesaulniers@google.com. > https://lore.kernel.org/lkml/ > 20210707181814.365496-1-ndesaulniers@google.com/#t > - Fix is enabled even for CONFIG_LTO_CLANG > > Change in v2: > - Use existing routine in kallsyms to strip postfix ".cfi_jt" from > extern function name. > - Modified the commit message accordingly > > kernel/kallsyms.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index 5cabe4dd3ff4..c8ef618e2a71 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -174,7 +174,7 @@ static bool cleanup_symbol_name(char *s) > * foo.llvm.974640843467629774. This can break hooking of static > * functions with kprobes. > */ > - if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN)) > + if (!IS_ENABLED(CONFIG_LTO_CLANG)) > return false; > > res = strstr(s, ".llvm."); > @@ -194,6 +194,17 @@ static bool cleanup_symbol_name(char *s) > return false; > > res = strrchr(s, '$'); > + if (!res) { > + /* > + * In case of non static function symbol <funcsym>, > + * the local jump table will have entry as <funcsym>.cfi_jt. > + * > + * Such expansion breaks some built-in components, > + * e.g. syscall tracer. Hence remove postfix ".cfi_jt". > + */ > + res = strstr(s, ".cfi_jt"); > + } > + > if (res) { > *res = '\0'; > return true; > -- > 2.17.1 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-10-01 18:30 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20210727131853.GA18032@pswork> 2021-07-27 14:06 ` [PATCH v2] kallsyms: strip ThinLTO postfix ".cfi_jt" Padmanabha Srinivasaiah 2021-07-28 20:57 ` Sami Tolvanen 2021-07-29 15:29 ` Padmanabha Srinivasaiah 2021-07-29 20:53 ` [PATCH v3] kallsyms: strip CLANG CFI " Padmanabha Srinivasaiah 2021-08-03 16:28 ` Sami Tolvanen 2021-08-04 17:17 ` Padmanabha Srinivasaiah 2021-08-05 23:27 ` [PATCH v4] " Padmanabha Srinivasaiah 2021-08-12 23:05 ` Sami Tolvanen 2021-08-14 12:15 ` Padmanabha Srinivasaiah 2021-08-14 12:42 ` [PATCH v5] " Padmanabha Srinivasaiah 2021-10-01 18:29 ` Nick Desaulniers
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).