Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210720221541.16164-1-padmanabha.srinivasaiah@harman.com>
In-Reply-To: <20210720221541.16164-1-padmanabha.srinivasaiah@harman.com>
From:   Sami Tolvanen <samitolvanen@google.com>
Date:   Mon, 26 Jul 2021 08:17:45 -0700
Message-ID: <CABCJKuecCch5v32GJ8Bt9wEJOWtp2aYanRT7qdrD6BHx5qDRNw@mail.gmail.com>
Subject: Re: [PATCH] tracing/syscalls: Clang cfi enable syscall events
To:     treasure4paddy@gmail.com
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        Padmanabha Srinivasaiah <padmanabha.srinivasaiah@harman.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        clang-built-linux <clang-built-linux@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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