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 <[email protected]>
---
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
Hi,
On Tue, Jul 27, 2021 at 7:07 AM Padmanabha Srinivasaiah
<[email protected]> 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 <[email protected]>
> ---
> 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/[email protected]/
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
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
> <[email protected]> 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 <[email protected]>
> > ---
> > 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/[email protected]/
>
> 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
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 <[email protected]>
---
Change in v3:
- Modified commit message to indicate fix is for Clang CFI postfix
- Rebased on recent patch from [email protected].
- 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
Hi,
On Thu, Jul 29, 2021 at 1:54 PM Padmanabha Srinivasaiah
<[email protected]> 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 <[email protected]>
> ---
>
> Change in v3:
> - Modified commit message to indicate fix is for Clang CFI postfix
> - Rebased on recent patch from [email protected].
> - 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
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
> <[email protected]> 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 <[email protected]>
> > ---
> >
> > Change in v3:
> > - Modified commit message to indicate fix is for Clang CFI postfix
> > - Rebased on recent patch from [email protected].
> > - 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
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 <[email protected]>
---
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 [email protected].
https://lore.kernel.org/lkml/
[email protected]/#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
On Thu, Aug 5, 2021 at 4:28 PM Padmanabha Srinivasaiah
<[email protected]> 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 <[email protected]>
> ---
> 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 [email protected].
> https://lore.kernel.org/lkml/
> [email protected]/#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
On Thu, Aug 12, 2021 at 04:05:25PM -0700, Sami Tolvanen wrote:
> On Thu, Aug 5, 2021 at 4:28 PM Padmanabha Srinivasaiah
> <[email protected]> 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 <[email protected]>
> > ---
> > 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 [email protected].
> > https://lore.kernel.org/lkml/
> > [email protected]/#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
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 <[email protected]>
---
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 [email protected].
https://lore.kernel.org/lkml/
[email protected]/#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
On Sat, Aug 14, 2021 at 5:43 AM Padmanabha Srinivasaiah
<[email protected]> 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/[email protected]/;
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 <[email protected]>
> ---
> 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 [email protected].
> https://lore.kernel.org/lkml/
> [email protected]/#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