Re: [RFC PATCH] Syscall tracing on PPC64_ELF_ABI_V1 without KALLSYMS_ALL

[Christophe Leroy <christophe.leroy@csgroup.eu>]

The changes are absolutely not specific to powerpc. You should adjust 
the subject accordingly, and copy linux-arch and tracing and probably 
also ia64 and parisc.

Le 12/12/2022 à 22:21, Michael Jeanson a écrit :
> In ad050d2390fccb22aa3e6f65e11757ce7a5a7ca5 we fixed ftrace syscall
> tracing on PPC64_ELF_ABI_V1 by looking for the non-dot prefixed symbol
> of a syscall.

Should be written as:

Commit ad050d2390fc ("powerpc/ftrace: fix syscall tracing on 
PPC64_ELF_ABI_V1") fixed ....


> 
> Ftrace uses kallsyms to locate syscall symbols and those non-dot
> prefixed symbols reside in a separate '.opd' section which is not
> included by kallsyms.
> 
> So we either need to have FTRACE_SYSCALLS select KALLSYMS_ALL on
> PPC64_ELF_ABI_V1 or add the '.opd' section symbols to kallsyms.
> 
> This patch does the minimum to achieve the latter, it's tested on a
> corenet64_smp_defconfig with KALLSYMS_ALL turned off.
> 
> I'm unsure which of the alternatives would be better.
> 
> ---
> In 'kernel/module/kallsyms.c' the 'is_core_symbol' function might also
> require some tweaking to make all opd symbols available to kallsyms but
> that doesn't impact ftrace syscall tracing.
> 
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
> ---
>   include/asm-generic/sections.h | 14 ++++++++++++++
>   include/linux/kallsyms.h       |  3 +++
>   kernel/kallsyms.c              |  2 ++
>   scripts/kallsyms.c             |  1 +
>   4 files changed, 20 insertions(+)
> 
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index db13bb620f52..1410566957e5 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -180,6 +180,20 @@ static inline bool is_kernel_rodata(unsigned long addr)
>   	       addr < (unsigned long)__end_rodata;
>   }
>   
> +/**
> + * is_kernel_opd - checks if the pointer address is located in the
> + *                 .opd section
> + *
> + * @addr: address to check
> + *
> + * Returns: true if the address is located in .opd, false otherwise.
> + */
> +static inline bool is_kernel_opd(unsigned long addr)
> +{

I would add a check of CONFIG_HAVE_FUNCTION_DESCRIPTORS:

	if (!IS_ENABLED(CONFIG_HAVE_FUNCTION_DESCRIPTORS))
		return false;

> +	return addr >= (unsigned long)__start_opd &&
> +	       addr < (unsigned long)__end_opd;
> +}
> +
>   /**
>    * is_kernel_inittext - checks if the pointer address is located in the
>    *                      .init.text section
> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index 649faac31ddb..9bfb4d8d41a5 100644
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> @@ -43,6 +43,9 @@ static inline int is_ksym_addr(unsigned long addr)
>   	if (IS_ENABLED(CONFIG_KALLSYMS_ALL))
>   		return is_kernel(addr);
>   
> +	if (IS_ENABLED(CONFIG_HAVE_FUNCTION_DESCRIPTORS))
> +		return is_kernel_text(addr) || is_kernel_inittext(addr) || is_kernel_opd(addr);
> +

With the check inside is_kernel_opd(), you can make it simpler:

	return is_kernel_text(addr) || is_kernel_inittext(addr) || 
is_kernel_opd(addr);

>   	return is_kernel_text(addr) || is_kernel_inittext(addr);
>   }
>   
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 60c20f301a6b..009b1ca21618 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -281,6 +281,8 @@ static unsigned long get_symbol_pos(unsigned long addr,
>   			symbol_end = (unsigned long)_einittext;
>   		else if (IS_ENABLED(CONFIG_KALLSYMS_ALL))
>   			symbol_end = (unsigned long)_end;
> +		else if (IS_ENABLED(CONFIG_HAVE_FUNCTION_DESCRIPTORS) && is_kernel_opd(addr))
> +			symbol_end = (unsigned long)__end_opd;

Same, with the check included inside is_kernel_opd() you don't need the 
IS_ENABLED(CONFIG_HAVE_FUNCTION_DESCRIPTORS) here.

>   		else
>   			symbol_end = (unsigned long)_etext;
>   	}
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 03fa07ad45d9..decf31c497f5 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -64,6 +64,7 @@ static unsigned long long relative_base;
>   static struct addr_range text_ranges[] = {
>   	{ "_stext",     "_etext"     },
>   	{ "_sinittext", "_einittext" },
> +	{ "__start_opd", "__end_opd" },
>   };
>   #define text_range_text     (&text_ranges[0])
>   #define text_range_inittext (&text_ranges[1])