On Tue, 23 Nov 2021, Peter Zijlstra wrote:
> On Tue, Nov 23, 2021 at 11:03:20AM -0500, Steven Rostedt wrote:
> > On Tue, 23 Nov 2021 12:39:15 +0100 (CET)
> > Miroslav Benes <mbenes@xxxxxxx> wrote:
> >
> > > +++ b/kernel/livepatch/patch.c
> > > @@ -127,15 +127,18 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> > > /*
> > > * Convert a function address into the appropriate ftrace location.
> > > *
> > > - * Usually this is just the address of the function, but on some architectures
> > > - * it's more complicated so allow them to provide a custom behaviour.
> > > + * Usually this is just the address of the function, but there are some
> > > + * exceptions.
> > > + *
> > > + * * PPC - live patch works only with -mprofile-kernel. In this case,
> > > + * the ftrace location is always within the first 16 bytes.
> > > + * * x86_64 with CET/IBT enabled - there is ENDBR instruction at +0 offset.
> > > + * __fentry__ follows it.
> > > */
> > > -#ifndef klp_get_ftrace_location
> > > -static unsigned long klp_get_ftrace_location(unsigned long faddr)
> > > +static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
> >
> > Why make this the default function? It should only do this for powerpc and
> > x86 *if* CET/IBT is enabled.
I thought that given it was a slow path anyway, it would be just nice to
get rid of the special casing. We support only x86, powerpc and s390x
after all.
And as Peter later pointed out, it would have the benefit to have a check
even for default version.
> Well, only this variant of IBT. Once Joao gets his clang patches
> together we'll probably have it back at +0.
>
> Something like the below would be more robust, it also gets us something
> grep-able for when the IBT code-gen changes yet again.
>
> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index 7c5cc6660e4b..4e683a1aa411 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -17,4 +17,13 @@ static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
> ftrace_instruction_pointer_set(fregs, ip);
> }
>
> +#define klp_get_ftrace_location klp_get_ftrace_location
> +static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
> +{
> + unsigned long addr = faddr_location(faddr);
> + if (!addr && IS_ENABLED(CONFIG_X86_IBT))
> + addr = faddr_location(faddr + 4);
> + return addr;
> +}
> +
> #endif /* _ASM_X86_LIVEPATCH_H */
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index fe316c021d73..fd295bbbcbc7 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -133,7 +133,7 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> #ifndef klp_get_ftrace_location
> static unsigned long klp_get_ftrace_location(unsigned long faddr)
> {
> - return faddr;
> + return ftrace_location(faddr);
> }
> #endif
Yes, this looks nice.
Thanks
Miroslav
