[PATCH] tracing/syscalls: Clang cfi enable syscall events

[PATCH] tracing/syscalls: Clang cfi enable syscall events

[treasure4paddy]

From: Padmanabha Srinivasaiah <padmanabha.srinivasaiah@harman.com>

With clang cfi syscall symbols are appended with <syscall>.cfi_jt,
hence syscall tracer can not find corresponding syscall name.
And results in no syscall ftrace events with CFI.

To fix this issue, this introduces custom cleanup_syscall_symbol_name()
to strip postfix ".cfi_jt" before comparing syscall and symbol name.

Signed-off-by: Padmanabha Srinivasaiah <padmanabha.srinivasaiah@harman.com>
---
 kernel/trace/trace_syscalls.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 8bfcd3b09422..dbc253fea34d 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -79,6 +79,27 @@ trace_get_syscall_nr(struct task_struct *task, struct pt_regs *regs)
 }
 #endif /* ARCH_TRACE_IGNORE_COMPAT_SYSCALLS */
 
+#if defined(CONFIG_CFI_CLANG) && defined(CONFIG_LTO_CLANG_THIN)
+/*
+ * For a syscall symbol, clang generated non-canonical local jump tables
+ * will have entry as <syscall>.cfi_jt and address of this entry
+ * will be used to replace references to the syscall symbol.
+ * so we will strip the postfix from appended symbol name.
+ */
+static inline bool cleanup_syscall_symbol_name(char *s)
+{
+	char *res;
+
+	res = strrchr(s, '.');
+	if (res)
+		*res = '\0';
+
+	return res != NULL;
+}
+#else
+static inline bool cleanup_syscall_symbol_name(char *s) { return false; }
+#endif
+
 static __init struct syscall_metadata *
 find_syscall_meta(unsigned long syscall)
 {
@@ -90,6 +111,7 @@ find_syscall_meta(unsigned long syscall)
 	start = __start_syscalls_metadata;
 	stop = __stop_syscalls_metadata;
 	kallsyms_lookup(syscall, NULL, NULL, NULL, str);
+	cleanup_syscall_symbol_name(str);
 
 	if (arch_syscall_match_sym_name(str, "sys_ni_syscall"))
 		return NULL;
-- 
2.17.1

Re: [PATCH] tracing/syscalls: Clang cfi enable syscall events

[Sami Tolvanen]

Hi,

On Tue, Jul 20, 2021 at 3:16 PM <treasure4paddy@gmail.com> wrote:
>
> From: Padmanabha Srinivasaiah <padmanabha.srinivasaiah@harman.com>
>
> With clang cfi syscall symbols are appended with <syscall>.cfi_jt,
> hence syscall tracer can not find corresponding syscall name.
> And results in no syscall ftrace events with CFI.
>
> To fix this issue, this introduces custom cleanup_syscall_symbol_name()
> to strip postfix ".cfi_jt" before comparing syscall and symbol name.
>
> Signed-off-by: Padmanabha Srinivasaiah <padmanabha.srinivasaiah@harman.com>
> ---
>  kernel/trace/trace_syscalls.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 8bfcd3b09422..dbc253fea34d 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -79,6 +79,27 @@ trace_get_syscall_nr(struct task_struct *task, struct pt_regs *regs)
>  }
>  #endif /* ARCH_TRACE_IGNORE_COMPAT_SYSCALLS */
>
> +#if defined(CONFIG_CFI_CLANG) && defined(CONFIG_LTO_CLANG_THIN)
> +/*
> + * For a syscall symbol, clang generated non-canonical local jump tables
> + * will have entry as <syscall>.cfi_jt and address of this entry
> + * will be used to replace references to the syscall symbol.
> + * so we will strip the postfix from appended symbol name.
> + */
> +static inline bool cleanup_syscall_symbol_name(char *s)
> +{
> +       char *res;
> +
> +       res = strrchr(s, '.');
> +       if (res)
> +               *res = '\0';
> +
> +       return res != NULL;
> +}
> +#else
> +static inline bool cleanup_syscall_symbol_name(char *s) { return false; }
> +#endif
> +
>  static __init struct syscall_metadata *
>  find_syscall_meta(unsigned long syscall)
>  {
> @@ -90,6 +111,7 @@ find_syscall_meta(unsigned long syscall)
>         start = __start_syscalls_metadata;
>         stop = __stop_syscalls_metadata;
>         kallsyms_lookup(syscall, NULL, NULL, NULL, str);
> +       cleanup_syscall_symbol_name(str);

Shouldn't this happen in the kallsyms code instead? We already have
code there to clean up function names, and it seems like we should
just strip the postfix there before returning the name.

Sami

Re: [PATCH] tracing/syscalls: Clang cfi enable syscall events

[Padmanabha Srinivasaiah]

On Mon, Jul 26, 2021 at 08:17:45AM -0700, Sami Tolvanen wrote:
> Hi,
> 
> On Tue, Jul 20, 2021 at 3:16 PM <treasure4paddy@gmail.com> wrote:
> >
> > From: Padmanabha Srinivasaiah <padmanabha.srinivasaiah@harman.com>
> >
> > With clang cfi syscall symbols are appended with <syscall>.cfi_jt,
> > hence syscall tracer can not find corresponding syscall name.
> > And results in no syscall ftrace events with CFI.
> >
> > To fix this issue, this introduces custom cleanup_syscall_symbol_name()
> > to strip postfix ".cfi_jt" before comparing syscall and symbol name.
> >
> > Signed-off-by: Padmanabha Srinivasaiah <padmanabha.srinivasaiah@harman.com>
> > ---
> >  kernel/trace/trace_syscalls.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> > index 8bfcd3b09422..dbc253fea34d 100644
> > --- a/kernel/trace/trace_syscalls.c
> > +++ b/kernel/trace/trace_syscalls.c
> > @@ -79,6 +79,27 @@ trace_get_syscall_nr(struct task_struct *task, struct pt_regs *regs)
> >  }
> >  #endif /* ARCH_TRACE_IGNORE_COMPAT_SYSCALLS */
> >
> > +#if defined(CONFIG_CFI_CLANG) && defined(CONFIG_LTO_CLANG_THIN)
> > +/*
> > + * For a syscall symbol, clang generated non-canonical local jump tables
> > + * will have entry as <syscall>.cfi_jt and address of this entry
> > + * will be used to replace references to the syscall symbol.
> > + * so we will strip the postfix from appended symbol name.
> > + */
> > +static inline bool cleanup_syscall_symbol_name(char *s)
> > +{
> > +       char *res;
> > +
> > +       res = strrchr(s, '.');
> > +       if (res)
> > +               *res = '\0';
> > +
> > +       return res != NULL;
> > +}
> > +#else
> > +static inline bool cleanup_syscall_symbol_name(char *s) { return false; }
> > +#endif
> > +
> >  static __init struct syscall_metadata *
> >  find_syscall_meta(unsigned long syscall)
> >  {
> > @@ -90,6 +111,7 @@ find_syscall_meta(unsigned long syscall)
> >         start = __start_syscalls_metadata;
> >         stop = __stop_syscalls_metadata;
> >         kallsyms_lookup(syscall, NULL, NULL, NULL, str);
> > +       cleanup_syscall_symbol_name(str);
> 
> Shouldn't this happen in the kallsyms code instead? We already have
> code there to clean up function names, and it seems like we should
> just strip the postfix there before returning the name.
> 
> Sami

Thanks Sami for the input, yes have regenrated patch:
https://lore.kernel.org/lkml/20210727140618.19130-1-treasure4paddy@gmail.com/T/#u