Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Torsten Duwe]

On Tue, Feb 02, 2016 at 04:46:27PM +0300, Denis Kirjanov wrote:
> > +
> > +#ifdef CONFIG_LIVEPATCH
> > +static inline int klp_check_compiler_support(void)
> > +{
> > +#if !defined(_CALL_ELF) || _CALL_ELF != 2 ||
> > !defined(CC_USING_MPROFILE_KERNEL)
> > +   return 1;
> > +#endif
> > +   return 0;
> > +}
> This function can be boolean.

Yes, like on the other archs, too.

Torsten

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Jiri Kosina]

On Tue, 2 Feb 2016, Petr Mladek wrote:

> Note that TOC is not set only when the problematic functions are 
> compiled with --mprofile-kernel. I still see the TOC stuff when 
> compiling only with -pg.

I don't see how this wouldn't be a gcc bug.

No matter whether it's plain profiling call (-pg) or kernel profiling call 
(-mprofile-kernel), gcc must always assume that global function (that will 
typically have just one instance for the whole address space) will be 
called.

-- 
Jiri Kosina
SUSE Labs

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Petr Mladek]

On Tue 2016-02-02 16:45:23, Torsten Duwe wrote:
> On Tue, Feb 02, 2016 at 01:12:24PM +0100, Petr Mladek wrote:
> > 
> > Hmm, the size of the offset is not a constant. In particular, leaf
> > functions do not set TOC before the mcount location.
> 
> To be slightly more precise, a leaf function that additionally uses
> no global data. No global function calls, no global data access =>
> no need to load the TOC.

Thanks for explanation.
 
> > The result is that kernel crashes when trying to trace leaf function
> 
> The trampoline *requires* a proper TOC pointer to find the remote function
> entry point. If you jump onto the trampoline with the TOC from the caller's
> caller you'll grab some address from somewhere and jump into nirvana.

The dmesg messages suggested someting like this.


> > By other words, it seems that the code generated with -mprofile-kernel
> > option has been buggy in all gcc versions.
> 
> Either that or we need bigger trampolines for everybody.
> 
> Michael, should we grow every module trampoline to always load R2,
> or fix GCC to recognise the generated bl _mcount as a global function call?
> Anton, what do you think?

BTW: Is the trampoline used also for classic probes? If not, we might need
a trampoline for them as well.

Note that TOC is not set only when the problematic functions are
compiled with --mprofile-kernel. I still see the TOC stuff when
compiling only with -pg.


Best Regards,
Petr
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Petr Mladek]

On Tue 2016-01-26 13:48:53, Petr Mladek wrote:
> On Tue 2016-01-26 11:50:25, Miroslav Benes wrote:
> > 
> > [ added Petr to CC list ]
> > 
> > On Mon, 25 Jan 2016, Torsten Duwe wrote:
> > 
> > >   * create the appropriate files+functions
> > > arch/powerpc/include/asm/livepatch.h
> > > klp_check_compiler_support,
> > > klp_arch_set_pc
> > > arch/powerpc/kernel/livepatch.c with a stub for
> > > klp_write_module_reloc
> > > This is architecture-independent work in progress.
> > >   * introduce a fixup in arch/powerpc/kernel/entry_64.S
> > > for local calls that are becoming global due to live patching.
> > > And of course do the main KLP thing: return to a maybe different
> > > address, possibly altered by the live patching ftrace op.
> > > 
> > > Signed-off-by: Torsten Duwe 
> > 
> > Hi,
> > 
> > I have a few questions...
> > 
> > We still need Petr's patch from [1] to make livepatch work, right? Could 
> > you, please, add it to this patch set to make it self-sufficient?
> > 
> > Second, what is the situation with mcount prologue between gcc < 6 and 
> > gcc-6? Are there only 12 bytes in gcc-6 prologue? If yes, we need to 
> > change Petr's patch to make it more general and to be able to cope with 
> > different prologues. This is unfortunate. Either way, please mention it 
> > somewhere in a changelog.
> 
> I am going to update the extra patch. There is an idea to detect the
> offset during build by scrips/recordmcount. This tool looks for the
> ftrace locations. The offset should always be a constant that depends
> on the used architecture, compiler, and compiler flags.
> 
> The tool is called post build. We might need to pass the constant
> as a symbol added to the binary. The tool already adds some symbols.

Hmm, the size of the offset is not a constant. In particular, leaf
functions do not set TOC before the mcount location.

For example, the code generated for int_to_scsilun() looks like:


02d0 :
 2d0:   a6 02 08 7c mflrr0
 2d4:   10 00 01 f8 std r0,16(r1)
 2d8:   01 00 00 48 bl  2d8 
2d8: R_PPC64_REL24  _mcount
 2dc:   a6 02 08 7c mflrr0
 2e0:   10 00 01 f8 std r0,16(r1)
 2e4:   e1 ff 21 f8 stdur1,-32(r1)
 2e8:   00 00 20 39 li  r9,0
 2ec:   00 00 24 f9 std r9,0(r4)
 2f0:   04 00 20 39 li  r9,4
 2f4:   a6 03 29 7d mtctr   r9
 2f8:   00 00 40 39 li  r10,0
 2fc:   02 c2 68 78 rldicl  r8,r3,56,8
 300:   78 23 89 7c mr  r9,r4
 304:   ee 51 09 7d stbux   r8,r9,r10
 308:   02 00 4a 39 addir10,r10,2
 30c:   01 00 69 98 stb r3,1(r9)
 310:   02 84 63 78 rldicl  r3,r3,48,16
 314:   e8 ff 00 42 bdnz2fc 
 318:   20 00 21 38 addir1,r1,32
 31c:   10 00 01 e8 ld  r0,16(r1)
 320:   a6 03 08 7c mtlrr0
 324:   20 00 80 4e blr
 328:   00 00 00 60 nop
 32c:   00 00 42 60 ori r2,r2,0


Note that non-leaf functions starts with

0330 :
 330:   00 00 4c 3c addis   r2,r12,0
330: R_PPC64_REL16_HA   .TOC.
 334:   00 00 42 38 addir2,r2,0
334: R_PPC64_REL16_LO   .TOC.+0x4
 338:   a6 02 08 7c mflrr0
 33c:   10 00 01 f8 std r0,16(r1)
 340:   01 00 00 48 bl  340 
340: R_PPC64_REL24  _mcount


The above code is generated from kernel-4.5-rc1 sources using

$> gcc --version
gcc (SUSE Linux) 4.8.5
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


But I get similar code also with

$> gcc-6 --version
gcc-6 (SUSE Linux) 6.0.0 20160121 (experimental) [trunk revision 232670]
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.



The result is that kernel crashes when trying to trace leaf function
from modules. The mcount location is replaced with a call (branch)
that does not work without the TOC stuff.

By other words, it seems that the code generated with -mprofile-kernel
option has been buggy in all gcc versions.

I am curious that nobody found this earlier. Do I something wrong,
please?


Best Regards,
Petr
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Denis Kirjanov]

On 1/25/16, Torsten Duwe  wrote:
>   * create the appropriate files+functions
> arch/powerpc/include/asm/livepatch.h
> klp_check_compiler_support,
> klp_arch_set_pc
> arch/powerpc/kernel/livepatch.c with a stub for
> klp_write_module_reloc
> This is architecture-independent work in progress.
>   * introduce a fixup in arch/powerpc/kernel/entry_64.S
> for local calls that are becoming global due to live patching.
> And of course do the main KLP thing: return to a maybe different
> address, possibly altered by the live patching ftrace op.
>
> Signed-off-by: Torsten Duwe 
> ---
>  arch/powerpc/include/asm/livepatch.h | 45 +++
>  arch/powerpc/kernel/entry_64.S   | 51
> +---
>  arch/powerpc/kernel/livepatch.c  | 38 +++
>  3 files changed, 130 insertions(+), 4 deletions(-)
>  create mode 100644 arch/powerpc/include/asm/livepatch.h
>  create mode 100644 arch/powerpc/kernel/livepatch.c
>
> diff --git a/arch/powerpc/include/asm/livepatch.h
> b/arch/powerpc/include/asm/livepatch.h
> new file mode 100644
> index 000..44e8a2d
> --- /dev/null
> +++ b/arch/powerpc/include/asm/livepatch.h
> @@ -0,0 +1,45 @@
> +/*
> + * livepatch.h - powerpc-specific Kernel Live Patching Core
> + *
> + * Copyright (C) 2015 SUSE
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */
> +#ifndef _ASM_POWERPC64_LIVEPATCH_H
> +#define _ASM_POWERPC64_LIVEPATCH_H
> +
> +#include 
> +#include 
> +
> +#ifdef CONFIG_LIVEPATCH
> +static inline int klp_check_compiler_support(void)
> +{
> +#if !defined(_CALL_ELF) || _CALL_ELF != 2 ||
> !defined(CC_USING_MPROFILE_KERNEL)
> + return 1;
> +#endif
> + return 0;
> +}
This function can be boolean.
> +
> +extern int klp_write_module_reloc(struct module *mod, unsigned long type,
> +unsigned long loc, unsigned long value);
> +
> +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> +{
> + regs->nip = ip;
> +}
> +#else
> +#error Live patching support is disabled; check CONFIG_LIVEPATCH
> +#endif
> +
> +#endif /* _ASM_POWERPC64_LIVEPATCH_H */
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 9e98aa1..f6e3ee7 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1267,6 +1267,9 @@ _GLOBAL(ftrace_caller)
>   mflrr3
>   std r3, _NIP(r1)
>   std r3, 16(r1)
> +#ifdef CONFIG_LIVEPATCH
> + mr  r14,r3  /* remember old NIP */
> +#endif
>   subir3, r3, MCOUNT_INSN_SIZE
>   mfmsr   r4
>   std r4, _MSR(r1)
> @@ -1283,7 +1286,10 @@ ftrace_call:
>   nop
>
>   ld  r3, _NIP(r1)
> - mtlrr3
> + mtctr   r3  /* prepare to jump there */
> +#ifdef CONFIG_LIVEPATCH
> + cmpdr14,r3  /* has NIP been altered? */
> +#endif
>
>   REST_8GPRS(0,r1)
>   REST_8GPRS(8,r1)
> @@ -1296,6 +1302,27 @@ ftrace_call:
>   mtlrr12
>   mr  r2,r0   /* restore callee's TOC */
>
> +#ifdef CONFIG_LIVEPATCH
> + beq+4f  /* likely(old_NIP == new_NIP) */
> +
> + /* For a local call, restore this TOC after calling the patch function.
> +  * For a global call, it does not matter what we restore here,
> +  * since the global caller does its own restore right afterwards,
> +  * anyway. Just insert a KLP_return_helper frame in any case,
> +  * so a patch function can always count on the changed stack offsets.
> +  */
> + stdur1,-32(r1)  /* open new mini stack frame */
> + std r0,24(r1)   /* save TOC now, unconditionally. */
> + bl  5f
> +5:   mflrr12
> + addir12,r12,(KLP_return_helper+4-.)@l
> + std r12,LRSAVE(r1)
> + mtlrr12
> + mfctr   r12 /* allow for TOC calculation in newfunc */
> + bctr
> +4:
> +#endif
> +
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>   stdur1, -112(r1)
>  .globl ftrace_graph_call
> @@ -1305,15 +1332,31 @@ _GLOBAL(ftrace_graph_stub)
>   addir1, r1, 112
>  #endif
>
> - mflrr0  /* move this LR to CTR */
> - mtctr   r0
> -
>   ld  r0,LRSAVE(r1)   /* restore callee's lr at _mcount site */
>   mtlrr0
>

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Torsten Duwe]

On Tue, Feb 02, 2016 at 01:12:24PM +0100, Petr Mladek wrote:
> 
> Hmm, the size of the offset is not a constant. In particular, leaf
> functions do not set TOC before the mcount location.

To be slightly more precise, a leaf function that additionally uses
no global data. No global function calls, no global data access =>
no need to load the TOC.

> For example, the code generated for int_to_scsilun() looks like:
> 
> 
> 02d0 :
>  2d0:   a6 02 08 7c mflrr0
>  2d4:   10 00 01 f8 std r0,16(r1)
>  2d8:   01 00 00 48 bl  2d8 
> 2d8: R_PPC64_REL24  _mcount
[...]
> The above code is generated from kernel-4.5-rc1 sources using
> 
> $> gcc --version
> gcc (SUSE Linux) 4.8.5
> 
> But I get similar code also with
> 
> $> gcc-6 --version
> gcc-6 (SUSE Linux) 6.0.0 20160121 (experimental) [trunk revision 232670]
> 
> 
> The result is that kernel crashes when trying to trace leaf function

The trampoline *requires* a proper TOC pointer to find the remote function
entry point. If you jump onto the trampoline with the TOC from the caller's
caller you'll grab some address from somewhere and jump into nirvana.

> By other words, it seems that the code generated with -mprofile-kernel
> option has been buggy in all gcc versions.

Either that or we need bigger trampolines for everybody.

Michael, should we grow every module trampoline to always load R2,
or fix GCC to recognise the generated bl _mcount as a global function call?
Anton, what do you think?

Torsten

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Miroslav Benes]

[ added Petr to CC list ]

On Mon, 25 Jan 2016, Torsten Duwe wrote:

>   * create the appropriate files+functions
> arch/powerpc/include/asm/livepatch.h
> klp_check_compiler_support,
> klp_arch_set_pc
> arch/powerpc/kernel/livepatch.c with a stub for
> klp_write_module_reloc
> This is architecture-independent work in progress.
>   * introduce a fixup in arch/powerpc/kernel/entry_64.S
> for local calls that are becoming global due to live patching.
> And of course do the main KLP thing: return to a maybe different
> address, possibly altered by the live patching ftrace op.
> 
> Signed-off-by: Torsten Duwe 

Hi,

I have a few questions...

We still need Petr's patch from [1] to make livepatch work, right? Could 
you, please, add it to this patch set to make it self-sufficient?

Second, what is the situation with mcount prologue between gcc < 6 and 
gcc-6? Are there only 12 bytes in gcc-6 prologue? If yes, we need to 
change Petr's patch to make it more general and to be able to cope with 
different prologues. This is unfortunate. Either way, please mention it 
somewhere in a changelog.

I haven't reviewed the patch properly yet, but there is a comment below.

[1] http://lkml.kernel.org/g/20151203160004.ge8...@pathway.suse.cz

> +/**
> + * klp_write_module_reloc() - write a relocation in a module
> + * @mod:   module in which the section to be modified is found
> + * @type:  ELF relocation type (see asm/elf.h)
> + * @loc:   address that the relocation should be written to
> + * @value: relocation value (sym address + addend)
> + *
> + * This function writes a relocation to the specified location for
> + * a particular module.
> + */
> +int klp_write_module_reloc(struct module *mod, unsigned long type,
> + unsigned long loc, unsigned long value)
> +{
> + /* This requires infrastructure changes; we need the loadinfos. */
> + pr_err("lpc_write_module_reloc not yet supported\n");

This is a nit, but there is no lpc_write_module_reloc. It should be 
klp_write_module_reloc.

Thanks,
Miroslav
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Torsten Duwe]

On Tue, Jan 26, 2016 at 01:48:53PM +0100, Petr Mladek wrote:
> On Tue 2016-01-26 11:50:25, Miroslav Benes wrote:
> > 
> > We still need Petr's patch from [1] to make livepatch work, right? Could 
> > you, please, add it to this patch set to make it self-sufficient?

It's Petr's patch, I don't want to decide how to best tackle this, see below.
I think Michael is already aware that it is needed, too.

> > Second, what is the situation with mcount prologue between gcc < 6 and 
> > gcc-6? Are there only 12 bytes in gcc-6 prologue? If yes, we need to 

Precisely, it's commit e95d0248daced44 (in http://repo.or.cz/official-gcc.git)
or svn trunk change 222352 "No need for -mprofile-kernel to save LR to stack."
It's efficient, I like it.

> I am going to update the extra patch. There is an idea to detect the
> offset during build by scrips/recordmcount. This tool looks for the
> ftrace locations. The offset should always be a constant that depends
> on the used architecture, compiler, and compiler flags.

My first idea was to check for compiler version defines, but some vendors
are rumoured to patch their compilers ;-)

> The tool is called post build. We might need to pass the constant
> as a symbol added to the binary. The tool already adds some symbols.

That's even better.
Thanks!
Torsten

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Torsten Duwe]

On Tue, Jan 26, 2016 at 11:50:25AM +0100, Miroslav Benes wrote:
> > + */
> > +int klp_write_module_reloc(struct module *mod, unsigned long type,
> > +   unsigned long loc, unsigned long value)
> > +{
> > +   /* This requires infrastructure changes; we need the loadinfos. */
> > +   pr_err("lpc_write_module_reloc not yet supported\n");
> 
> This is a nit, but there is no lpc_write_module_reloc. It should be 
> klp_write_module_reloc.

Indeed. Michael, feel free to fix this on the fly or not. It needs to
disappear anyway and be replaced with functionality.

Torsten

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Miroslav Benes]

[ Jessica added to CC list so she is aware that there are plans to 
implement livepatch on ppc64le ]

On Tue, 26 Jan 2016, Torsten Duwe wrote:

> On Tue, Jan 26, 2016 at 11:50:25AM +0100, Miroslav Benes wrote:
> > > + */
> > > +int klp_write_module_reloc(struct module *mod, unsigned long type,
> > > + unsigned long loc, unsigned long value)
> > > +{
> > > + /* This requires infrastructure changes; we need the loadinfos. */
> > > + pr_err("lpc_write_module_reloc not yet supported\n");
> > 
> > This is a nit, but there is no lpc_write_module_reloc. It should be 
> > klp_write_module_reloc.
> 
> Indeed. Michael, feel free to fix this on the fly or not. It needs to
> disappear anyway and be replaced with functionality.

Or not at all thanks to Jessica's effort [1].

Miroslav

[1] http://lkml.kernel.org/g/1452281304-28618-1-git-send-email-j...@redhat.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2)

[Petr Mladek]

On Tue 2016-01-26 11:50:25, Miroslav Benes wrote:
> 
> [ added Petr to CC list ]
> 
> On Mon, 25 Jan 2016, Torsten Duwe wrote:
> 
> >   * create the appropriate files+functions
> > arch/powerpc/include/asm/livepatch.h
> > klp_check_compiler_support,
> > klp_arch_set_pc
> > arch/powerpc/kernel/livepatch.c with a stub for
> > klp_write_module_reloc
> > This is architecture-independent work in progress.
> >   * introduce a fixup in arch/powerpc/kernel/entry_64.S
> > for local calls that are becoming global due to live patching.
> > And of course do the main KLP thing: return to a maybe different
> > address, possibly altered by the live patching ftrace op.
> > 
> > Signed-off-by: Torsten Duwe 
> 
> Hi,
> 
> I have a few questions...
> 
> We still need Petr's patch from [1] to make livepatch work, right? Could 
> you, please, add it to this patch set to make it self-sufficient?
> 
> Second, what is the situation with mcount prologue between gcc < 6 and 
> gcc-6? Are there only 12 bytes in gcc-6 prologue? If yes, we need to 
> change Petr's patch to make it more general and to be able to cope with 
> different prologues. This is unfortunate. Either way, please mention it 
> somewhere in a changelog.

I am going to update the extra patch. There is an idea to detect the
offset during build by scrips/recordmcount. This tool looks for the
ftrace locations. The offset should always be a constant that depends
on the used architecture, compiler, and compiler flags.

The tool is called post build. We might need to pass the constant
as a symbol added to the binary. The tool already adds some symbols.


Best Regards,
Petr
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev