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

[Michael Jeanson <mjeanson@xxxxxxxxxxxx>]

On 2022-12-13 02:29, Christophe Leroy wrote:
> 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.

I was hoping to get feedback from the PPC maintainers before posting something 
more widely. I'll send an updated patch which addresses your comments.

Thanks.

> 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@xxxxxxxxxxxxxx>
> > Cc: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> > Signed-off-by: Michael Jeanson <mjeanson@xxxxxxxxxxxx>
> > ---
> >    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])