Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Balbir Singh]

On 24/02/16 20:23, Torsten Duwe wrote:
> On Wed, Feb 24, 2016 at 05:55:35PM +1100, Balbir Singh wrote:
>> 
>>
>> We need to remove the SQUASH_TOC_SAVE_INSNS bits as well, now that the 
>> ppc64_profile_stub_insns does not save r2
> Sure -- this was meant to _replace_ the changes from patch 2/8, not on top.
> And yes, it exposes duplicate definitions, but does not cause them AFAICS.
> The two unasked questions about it were: Is Michael's solution on a similar
> basis? Is this worth any further effort e.g. put into v9?
>
>
My bad you did mention _replace_, but I think 2/8 and 6/8 of tightly bound
together, so the replacement is not straight forward. Yes, it is heading in
a similar direction, but it focuses mostly on ftrace. I think v9 makes sense,
but I'll let Michael comment on this as well]

Personally, I think your v8 or v9 + Michael's changes - RECORD_C_MCOUNT +
some changes (yet to code them based on v8/v9/ftrace stability) should get
the full live patching working.

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

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Torsten Duwe]

On Wed, Feb 24, 2016 at 05:55:35PM +1100, Balbir Singh wrote:
> 
> 
> We need to remove the SQUASH_TOC_SAVE_INSNS bits as well, now that the 
> ppc64_profile_stub_insns does not save r2

Sure -- this was meant to _replace_ the changes from patch 2/8, not on top.
And yes, it exposes duplicate definitions, but does not cause them AFAICS.
The two unasked questions about it were: Is Michael's solution on a similar
basis? Is this worth any further effort e.g. put into v9?

Torsten

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

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Kamalesh Babulal]

* Torsten Duwe  [2016-02-23 18:00:17]:

> On Wed, Feb 17, 2016 at 02:08:41PM +1100, Michael Ellerman wrote:
> > 
> > That stub uses r2 to find the location of itself, but it only works if r2 
> > holds
> > the TOC for scsi_mod.ko. In this case r2 still contains ibmvscsi.ko's TOC.
> 
> Here's my solution, a bit rough still. This replaces the module_64.c change
> from patch 2/8:
> 
> I shuffle the trampoline instructions so the R2-save-to-stack comes first.
> This allows me to construct a profiling trampoline code that
> looks very similar. The first instruction, harmful to -mprofile-kernel
> can now be replaced with a load of the *kernel* TOC value via paca.
> Arithmetic is done in r11, to keep it bitwise identical where possible.
> Likewise the result is "moved" to r12 via an addi.
> 
> What do you think?
> 

Hi Torsten,

 I hit build failure, after replacing this patch with patch 2/8 module_64.c
hunk.

  CC  arch/powerpc/kernel/module.o
  CC  arch/powerpc/kernel/module_64.o
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
 from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:14:0: error: "NMI_MASK" redefined [-Werror]
 #define NMI_MASK 1048576 /* NMI_MASK  # */
 ^
In file included from include/linux/spinlock.h:50:0,
 from include/linux/seqlock.h:35,
 from include/linux/time.h:5,
 from include/linux/stat.h:18,
 from include/linux/module.h:10,
 from arch/powerpc/kernel/module_64.c:21:
include/linux/preempt.h:46:0: note: this is the location of the previous 
definition
 #define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
 ^
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
 from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:148:0: error: "CLONE_VM" redefined [-Werror]
 #define CLONE_VM 256 /* CLONE_VM  # */
 ^
In file included from include/linux/sched.h:4:0,
 from ./arch/powerpc/include/asm/elf.h:12,
 from include/linux/elf.h:4,
 from include/linux/module.h:15,
 from arch/powerpc/kernel/module_64.c:21:
include/uapi/linux/sched.h:8:0: note: this is the location of the previous 
definition
 #define CLONE_VM 0x0100 /* set if VM shared between processes */
 ^
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
 from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:149:0: error: "CLONE_UNTRACED" redefined 
[-Werror]
 #define CLONE_UNTRACED 8388608 /* CLONE_UNTRACED  # */
 ^
In file included from include/linux/sched.h:4:0,
 from ./arch/powerpc/include/asm/elf.h:12,
 from include/linux/elf.h:4,
 from include/linux/module.h:15,
 from arch/powerpc/kernel/module_64.c:21:
include/uapi/linux/sched.h:22:0: note: this is the location of the previous 
definition
 #define CLONE_UNTRACED  0x0080 /* set if the tracing process can't force 
CLONE_PTRACE on this clone */
 ^
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
 from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:185:0: error: "NSEC_PER_SEC" redefined [-Werror]
 #define NSEC_PER_SEC 10 /* NSEC_PER_SEC  # */
 ^
In file included from include/linux/time.h:7:0,
 from include/linux/stat.h:18,
 from include/linux/module.h:10,
 from arch/powerpc/kernel/module_64.c:21:
include/linux/time64.h:35:0: note: this is the location of the previous 
definition
 #define NSEC_PER_SEC 10L
 ^
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
 from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:188:0: error: "PGD_TABLE_SIZE" redefined 
[-Werror]
 #define PGD_TABLE_SIZE 32768 /* PGD_TABLE_SIZE  # */
 ^
In file included from ./arch/powerpc/include/asm/book3s/64/hash.h:58:0,
 from ./arch/powerpc/include/asm/book3s/64/pgtable.h:8,
 from ./arch/powerpc/include/asm/mmu-hash64.h:24,
 from ./arch/powerpc/include/asm/mmu.h:185,
 from ./arch/powerpc/include/asm/lppaca.h:36,
 from ./arch/powerpc/include/asm/paca.h:21,
 from ./arch/powerpc/include/asm/hw_irq.h:42,
 from ./arch/powerpc/include/asm/irqflags.h:11,
 from include/linux/irqflags.h:15,
 from include/linux/spinlock.h:53,
 from include/linux/seqlock.h:35,
 from include/linux/time.h:5,
 from include/linux/stat.h:18,
 from include/linux/module.h:10,
 from arch/powerpc/kernel/module_64.c:21:
./arch/powerpc/include/asm/book3s/64/hash-64k.h:133:0: note: this is the 
location of the previous definition
 #define PGD_TABLE_SIZE (sizeof(pgd_t) << PGD_INDEX_SIZE)
 

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Balbir Singh]

We need to remove the SQUASH_TOC_SAVE_INSNS bits as well, now that the 
ppc64_profile_stub_insns does not save r2
> Looks like we are getting closer to the final solution Thanks, Balbir 

With the SQUASH_TOC_SAVE_INSNS removed, ftrace function seems to work, but 
function_graph is broken. I've not yet debugged this.

[   77.182430] b'Oops: Kernel access of bad area, sig: 11 [#1]'
[   77.182464] b'SMP NR_CPUS=32 NUMA pSeries'
[   77.182513] b'Modules linked in: sr_mod cdrom virtio_blk virtio_net ibmvscsi 
scsi_transport_srp scsi_mod virtio_pci virtio_ring virtio'
[   77.182661] b'CPU: 1 PID: 2287 Comm: sshd Not tainted 
4.5.0-rc4-7-g1968536-dirty #143'
[   77.182709] b'task: c00037b6bc00 ti: c0003e8c4000 task.ti: 
c0003e8c4000'
[   77.182757] b'NIP: c0194ebc LR: c0049d4c CTR: 
d04f1434'
[   77.182804] b'REGS: c0003e8c72a0 TRAP: 0300   Not tainted  
(4.5.0-rc4-7-g1968536-dirty)'
[   77.182858] b'MSR: 80009033   CR: 28282828  
XER: 2000'
[   77.183008] b'CFAR: c017f400 DAR: d0653c70 DSISR: 4000 
SOFTE: 1 '
[   77.183008] b'GPR00: c0009f3c c0003e8c7520 d04fde40 
c077da34 '
[   77.183008] b'GPR04: d04f1430 c0003e008100 c0003719e520 
c0003719e008 '
[   77.183008] b'GPR08: c113ea50 d064de40 d04f57b8 
c0009d1c '
[   77.183008] b'GPR12: c077da34 cfff8400 05a8 
4000 '
[   77.183008] b'GPR16: 2200 000346db c0003e8c7720 
c113fbe0 '
[   77.183008] b'GPR20:  c113fbd0 c00037a9dc00 
 '
[   77.183008] b'GPR24:  c113fbe0 c00037077000 
c00037077090 '
[   77.183008] b'GPR28: c000379982d8 d04f1430 c077da34 
c00037077068 '
[   77.183788] b'NIP [c0194ebc] ftrace_graph_is_dead+0xc/0x20'
[   77.183850] b'LR [c0049d4c] prepare_ftrace_return+0x2c/0x110'
---Type  to continue, or q  to quit---
[   77.183890] b'Call Trace:'
[   77.183911] b'[c0003e8c7520] [c00037a9dc00] 0xc00037a9dc00 
(unreliable)'
[   77.183987] b'[c0003e8c7570] [c0009f3c] 
ftrace_graph_caller+0x34/0x74'
[   77.184080] b'[c0003e8c75e0] [c077da34] 
dev_hard_start_xmit+0x374/0x4e0'
[   77.184139] b'[c0003e8c76c0] [c0009f7c] 
return_to_handler+0x0/0x58 (bad_page_fault+0x130/0x150)'
[   77.184210] b'[c0003e8c7760] [c0009f7c] 
return_to_handler+0x0/0x58 (handle_page_fault+0x2c/0x30)'
[   77.184281] b'[c0003e8c7800] [c0009f7c] 
return_to_handler+0x0/0x58 (sch_direct_xmit+0xe0/0x2d0)'
[   77.184369] b'[c0003e8c7860] [c0009f7c] 
return_to_handler+0x0/0x58 (__dev_queue_xmit+0x2d4/0x6a0)'
[   77.184473] b'[c0003e8c78f0] [c0009f7c] 
return_to_handler+0x0/0x58 (return_to_handler+0x0/0x58)'
[   77.184544] b'[c0003e8c7930] [c0009f7c] 
return_to_handler+0x0/0x58 (ip_finish_output2+0x348/0x420)'
[   77.184614] b'[c0003e8c79a0] [c0009f7c] 
return_to_handler+0x0/0x58 (return_to_handler+0x0/0x58)'
[   77.184684] b'[c0003e8c7a70] [c0009f7c] 
return_to_handler+0x0/0x58 (ip_output+0xd0/0x160)'
[   77.184754] b'[c0003e8c7ae0] [c0009f7c] 
return_to_handler+0x0/0x58 (ip_local_out+0x6c/0x90)'
[   77.184823] b'[c0003e8c7b30] [c0009f7c] 
return_to_handler+0x0/0x58 (return_to_handler+0x0/0x58)'
[   77.184893] b'[c0003e8c7c00] [c0009f7c] 
return_to_handler+0x0/0x58 (tcp_transmit_skb+0x980/0xa50)'
[   77.184969] b'[c0003e8c7c40] [c0009f7c] 
return_to_handler+0x0/0x58 (tcp_write_xmit+0xd9c/0x1120)'
[   77.185039] b'[c0003e8c7c60] [c0009f7c] 
return_to_handler+0x0/0x58 (__tcp_push_pending_frames+0x50/0x130)'
[   77.185117] b'[c0003e8c7d00] [c0009f7c] 
return_to_handler+0x0/0x58 (tcp_push+0x194/0x1e0)'
[   77.185192] b'[c0003e8c7d90] [c0009f7c] 
return_to_handler+0x0/0x58 (tcp_sendmsg+0xa54/0xce0)'
[   77.185262] b'[c0003e8c7de0] [c0009f7c] 
return_to_handler+0x0/0x58 (inet_sendmsg+0xd8/0x100)'
[   77.185342] b'[c0003e8c7e30] [c0009f7c] 
return_to_handler+0x0/0x58 (sock_sendmsg+0x38/0x60)'
[   77.185416] b'Instruction dump:'
[   77.185469] b'6000 4bfe3b89 6000 e8610020 38210030 e8010010 7c0803a6 
4b40 '
[   77.185566] b'6042 3c4c00fa 3842e750 3d220015 <88695e30> 4e800020 
6000 6000 '
[   77.185668] b'---[ end trace 78e882547ec0a563 ]---'
[   79.191159] b'Kernel panic - not syncing: Fatal exception in interrupt'

Warm Regards,
Balbir Singh.


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

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Balbir Singh]

On 24/02/16 04:00, Torsten Duwe wrote:
> On Wed, Feb 17, 2016 at 02:08:41PM +1100, Michael Ellerman wrote:
>> That stub uses r2 to find the location of itself, but it only works if r2 
>> holds
>> the TOC for scsi_mod.ko. In this case r2 still contains ibmvscsi.ko's TOC.
> Here's my solution, a bit rough still. This replaces the module_64.c change
> from patch 2/8:
>
> I shuffle the trampoline instructions so the R2-save-to-stack comes first.
> This allows me to construct a profiling trampoline code that
> looks very similar. The first instruction, harmful to -mprofile-kernel
> can now be replaced with a load of the *kernel* TOC value via paca.
> Arithmetic is done in r11, to keep it bitwise identical where possible.
> Likewise the result is "moved" to r12 via an addi.
Michael has a similar change that he intends to post. I gave this a run but
the system crashes on boot.

>
> What do you think?
>
>   Torsten
>
>
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
Creates an include conflict for me. NMI_MASK, PGD_TABLE_SIZE, etc
are already defined elsewhere.
>  #include 
>  #include 
>  #include 
> @@ -123,10 +124,10 @@ struct ppc64_stub_entry
>   */
>  
>  static u32 ppc64_stub_insns[] = {
> - 0x3d62, /* addis   r11,r2,  */
> - 0x396b, /* addir11,r11,  */
>   /* Save current r2 value in magic place on the stack. */
>   0xf841|R2_STACK_OFFSET, /* std r2,R2_STACK_OFFSET(r1) */
> + 0x3d62, /* addis   r11,r2,  */
> + 0x396b, /* addir11,r11,  */
>   0xe98b0020, /* ld  r12,32(r11) */
>  #if !defined(_CALL_ELF) || _CALL_ELF != 2
>   /* Set up new r2 from function descriptor */
> @@ -136,13 +137,30 @@ static u32 ppc64_stub_insns[] = {
>   0x4e800420  /* bctr */
>  };
>  
> +/* In case of _mcount calls or dynamic ftracing with -mprofile-kernel,
> + * the stack frame already holds the TOC value of the original
> + * caller. And even worse, for a leaf function without global data
> + * references, R2 holds the TOC of the caller's caller, e.g. is
> + * completely undefined. So: do not dare to write r2 anywhere, and use
> + * the kernel's TOC to find _mcount / ftrace_caller.  Mcount and
> + * ftrace_caller will then take care of the r2 value themselves.
> + */
> +static u32 ppc64_profile_stub_insns[] = {
> + 0xe98d|PACATOC, /* ld  r12,PACATOC(r13) */
> + 0x3d6c, /* addis   r11,r12,  */
> + 0x396b, /* addir11,r11,  */
> + 0x398b, /* addir12,r11,0 */
> + 0x7d8903a6, /* mtctr   r12 */
> + 0x4e800420  /* bctr */
> +};
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  
>  static u32 ppc64_stub_mask[] = {
> + 0xee33,
> + 0xfff1,
>   0x,
> - 0x,
> - 0x,
> - 0x,
> + 0x2fdf,
>  #if !defined(_CALL_ELF) || _CALL_ELF != 2
>   0x,
>  #endif
> @@ -168,10 +186,15 @@ bool is_module_trampoline(u32 *p)
>   if ((insna & mask) != (insnb & mask))
>   return false;
>   }
> + if (insns[0] != ppc64_stub_insns[0] &&
> + insns[0] != ppc64_profile_stub_insns[0])
> + return false;
>  
Michael was mentioning a better way of doing this, we can simplify the
checking bits

>   return true;
>  }
>  
> +extern unsigned long __toc_start;
> +
>  int module_trampoline_target(struct module *mod, u32 *trampoline,
>unsigned long *target)
>  {
> @@ -180,7 +203,7 @@ int module_trampoline_target(struct modu
>   long offset;
>   void *toc_entry;
>  
> - if (probe_kernel_read(buf, trampoline, sizeof(buf)))
> + if (probe_kernel_read(buf, trampoline+1, sizeof(buf)))
>   return -EFAULT;
>  
>   upper = buf[0] & 0x;
> @@ -189,6 +212,13 @@ int module_trampoline_target(struct modu
>   /* perform the addis/addi, both signed */
>   offset = ((short)upper << 16) + (short)lower;
>  
> + /* profiling trampolines work differently */
> + if ((buf[0] & 0x) == 0x3D6C)
> +   {
> + *target = offset + (unsigned long)(&__toc_start) + 0x8000UL;
> + return 0;
> +   }
> +
>   /*
>* Now get the address this trampoline jumps to. This
>* is always 32 bytes into our trampoline stub.
> @@ -427,14 +457,24 @@ static inline unsigned long my_r2(Elf64_
>  static inline int create_stub(Elf64_Shdr *sechdrs,
> struct ppc64_stub_entry *entry,
> unsigned long addr,
> -   struct module *me)
> +   struct module *me,
> +   bool prof)
>  {
>   

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Torsten Duwe]

On Wed, Feb 17, 2016 at 02:08:41PM +1100, Michael Ellerman wrote:
> 
> That stub uses r2 to find the location of itself, but it only works if r2 
> holds
> the TOC for scsi_mod.ko. In this case r2 still contains ibmvscsi.ko's TOC.

Here's my solution, a bit rough still. This replaces the module_64.c change
from patch 2/8:

I shuffle the trampoline instructions so the R2-save-to-stack comes first.
This allows me to construct a profiling trampoline code that
looks very similar. The first instruction, harmful to -mprofile-kernel
can now be replaced with a load of the *kernel* TOC value via paca.
Arithmetic is done in r11, to keep it bitwise identical where possible.
Likewise the result is "moved" to r12 via an addi.

What do you think?

Torsten


--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -123,10 +124,10 @@ struct ppc64_stub_entry
  */
 
 static u32 ppc64_stub_insns[] = {
-   0x3d62, /* addis   r11,r2,  */
-   0x396b, /* addir11,r11,  */
/* Save current r2 value in magic place on the stack. */
0xf841|R2_STACK_OFFSET, /* std r2,R2_STACK_OFFSET(r1) */
+   0x3d62, /* addis   r11,r2,  */
+   0x396b, /* addir11,r11,  */
0xe98b0020, /* ld  r12,32(r11) */
 #if !defined(_CALL_ELF) || _CALL_ELF != 2
/* Set up new r2 from function descriptor */
@@ -136,13 +137,30 @@ static u32 ppc64_stub_insns[] = {
0x4e800420  /* bctr */
 };
 
+/* In case of _mcount calls or dynamic ftracing with -mprofile-kernel,
+ * the stack frame already holds the TOC value of the original
+ * caller. And even worse, for a leaf function without global data
+ * references, R2 holds the TOC of the caller's caller, e.g. is
+ * completely undefined. So: do not dare to write r2 anywhere, and use
+ * the kernel's TOC to find _mcount / ftrace_caller.  Mcount and
+ * ftrace_caller will then take care of the r2 value themselves.
+ */
+static u32 ppc64_profile_stub_insns[] = {
+   0xe98d|PACATOC, /* ld  r12,PACATOC(r13) */
+   0x3d6c, /* addis   r11,r12,  */
+   0x396b, /* addir11,r11,  */
+   0x398b, /* addir12,r11,0 */
+   0x7d8903a6, /* mtctr   r12 */
+   0x4e800420  /* bctr */
+};
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 static u32 ppc64_stub_mask[] = {
+   0xee33,
+   0xfff1,
0x,
-   0x,
-   0x,
-   0x,
+   0x2fdf,
 #if !defined(_CALL_ELF) || _CALL_ELF != 2
0x,
 #endif
@@ -168,10 +186,15 @@ bool is_module_trampoline(u32 *p)
if ((insna & mask) != (insnb & mask))
return false;
}
+   if (insns[0] != ppc64_stub_insns[0] &&
+   insns[0] != ppc64_profile_stub_insns[0])
+   return false;
 
return true;
 }
 
+extern unsigned long __toc_start;
+
 int module_trampoline_target(struct module *mod, u32 *trampoline,
 unsigned long *target)
 {
@@ -180,7 +203,7 @@ int module_trampoline_target(struct modu
long offset;
void *toc_entry;
 
-   if (probe_kernel_read(buf, trampoline, sizeof(buf)))
+   if (probe_kernel_read(buf, trampoline+1, sizeof(buf)))
return -EFAULT;
 
upper = buf[0] & 0x;
@@ -189,6 +212,13 @@ int module_trampoline_target(struct modu
/* perform the addis/addi, both signed */
offset = ((short)upper << 16) + (short)lower;
 
+   /* profiling trampolines work differently */
+   if ((buf[0] & 0x) == 0x3D6C)
+ {
+   *target = offset + (unsigned long)(&__toc_start) + 0x8000UL;
+   return 0;
+ }
+
/*
 * Now get the address this trampoline jumps to. This
 * is always 32 bytes into our trampoline stub.
@@ -427,14 +457,24 @@ static inline unsigned long my_r2(Elf64_
 static inline int create_stub(Elf64_Shdr *sechdrs,
  struct ppc64_stub_entry *entry,
  unsigned long addr,
- struct module *me)
+ struct module *me,
+ bool prof)
 {
long reladdr;
 
-   memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
+   if (prof)
+   {
+   memcpy(entry->jump, ppc64_profile_stub_insns,
+  sizeof(ppc64_stub_insns));
 
-   /* Stub uses address relative to r2. */
-   reladdr = (unsigned long)entry - my_r2(sechdrs, me);
+   /* Stub uses address relative to kernel TOC. */
+   reladdr = addr - ((unsigned 

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Michael Ellerman]

On Tue, 2016-02-16 at 14:57 +0100, Petr Mladek wrote:
> 
> Some dugging has shown an Oops in the fucntion int_to_scsilun()
> called from ibmvscsi_queuecommand(). So, I rebooted and
> did the following test:
> 
> $> echo ibmvscsi_queuecommand >/sys/kernel/debug/tracing/set_ftrace_filter
> $> echo function > /sys/kernel/debug/tracing/current_tracer 
> $> echo 1 > /sys/kernel/debug/tracing/tracing_on
> $> cat /sys/kernel/debug/tracing/trace
> # tracer: function
> #
> # entries-in-buffer/entries-written: 7/7   #P:4
> #
> #  _-=> irqs-off
> # / _=> need-resched
> #| / _---=> hardirq/softirq
> #|| / _--=> preempt-depth
> #||| / delay
> #   TASK-PID   CPU#  TIMESTAMP  FUNCTION
> #  | |   |      | |
> bash-3488  [000]    100.278622: ibmvscsi_queuecommand 
> <-scsi_dispatch_cmd
>  kworker/1:2-223   [001]    101.048569: ibmvscsi_queuecommand 
> <-scsi_dispatch_cmd
>  kworker/1:2-223   [001]    103.048575: ibmvscsi_queuecommand 
> <-scsi_dispatch_cmd
>  jbd2/sda3-8-1021  [003]    104.008645: ibmvscsi_queuecommand 
> <-scsi_dispatch_cmd
>  jbd2/sda3-8-1021  [003]    104.008883: ibmvscsi_queuecommand 
> <-scsi_dispatch_cmd
>   -0 [000] ..s.   104.017672: ibmvscsi_queuecommand 
> <-scsi_dispatch_cmd
>   -0 [003] ..s.   104.017771: ibmvscsi_queuecommand 
> <-scsi_dispatch_cmd
> 
> It means that ibmvscsi_queuecommand can be traced. Then I did
> 
> c79:/sys/kernel/debug/tracing # echo int_to_scsilun >set_ftrace_filter
> 
> BANG!
> 
> Unable to handle kernel paging request for data at address 0xd108b148
> Faulting instruction address: 0xd0bde35c
> Oops: Kernel access of bad area, sig: 11 [#1]
> SMP NR_CPUS=2048 NUMA pSeries
> Modules linked in: af_packet(E) dm_mod(E) e1000(E) rtc_generic(E) ext4(E) 
> crc16(E) mbcache(E) jbd2(E) sr_mod(E) cdrom(E) sd_mod(E) ibmvscsi(E) 
> scsi_transport_srp(E) sg(E) scsi_mod(E) autofs4(E)
> CPU: 1 PID: 223 Comm: kworker/1:2 Tainted: GE   
> 4.5.0-rc2-11-default+ #90
> Workqueue: events_freezable_power_ disk_events_workfn
> task: c000f7d99aa0 ti: c000f7cb8000 task.ti: c000f7cb8000
> NIP: d0bde35c LR: d0bcffec CTR: d0bcffe0
> REGS: c000f7cbb3b0 TRAP: 0300   Tainted: GE
> (4.5.0-rc2-11-default+)
> MSR: 80019033   CR: 24c82220  XER: 
> 
> CFAR: d0bdd144 DAR: d108b148 DSISR: 4000 SOFTE: 0 
> GPR00: d10a21cc c000f7cbb630 d10aeda0 8200 
> GPR04: c000fa0b33fc 014a c000fa0b3408 0010 
> GPR08: 0008 c370a4e0  d108b128 
> GPR12: d0bcffe0 c7e80300 c00d8b18 c000fe04ba80 
> GPR16:  c000f7b6f208  0001 
> GPR20: c000f7b6f144 c000f7b6f140 d0bcbcf0  
> GPR24: 0200  0001 c000f7b6f810 
> GPR28: c000f7b6f000 c000f7b6f000 c000fa0b33a0 c000fe837e00 
> NIP [d0bde35c] scsi_inq_str+0x21b0/0x41ac [scsi_mod]
> LR [d0bcffec] int_to_scsilun+0xc/0x60 [scsi_mod]
> Call Trace:
> [c000f7cbb630] [d10a21cc] ibmvscsi_queuecommand+0x10c/0x4e0 
> [ibmvscsi] (unreliable)
> [c000f7cbb6e0] [d0bcbea8] scsi_dispatch_cmd+0xe8/0x2c0 [scsi_mod]
> [c000f7cbb760] [d0bceb0c] scsi_request_fn+0x50c/0x8b0 [scsi_mod]
> [c000f7cbb850] [c0407280] __blk_run_queue+0x60/0x90
> [c000f7cbb880] [c0413640] blk_execute_rq_nowait+0x100/0x1a0
> [c000f7cbb8d0] [c0413768] blk_execute_rq+0x88/0x170
> [c000f7cbb9b0] [d0bca048] scsi_execute+0x108/0x1d0 [scsi_mod]
> [c000f7cbba20] [d0bca2e8] scsi_execute_req_flags+0xc8/0x150 
> [scsi_mod]
> [c000f7cbbae0] [d12209e4] sr_check_events+0xb4/0x340 [sr_mod]
> [c000f7cbbb90] [d11c00b4] cdrom_check_events+0x44/0x80 [cdrom]
> [c000f7cbbbc0] [d1220fa4] sr_block_check_events+0x44/0x60 [sr_mod]
> [c000f7cbbbe0] [c04222f8] disk_check_events+0x78/0x1b0
> [c000f7cbbc50] [c00d0610] process_one_work+0x1a0/0x480
> [c000f7cbbce0] [c00d0998] worker_thread+0xa8/0x5c0
> [c000f7cbbd80] [c00d8c24] kthread+0x114/0x140
> [c000f7cbbe30] [c0009538] ret_from_kernel_thread+0x5c/0xa4
> Instruction dump:
> 396bc360 f8410018 e98b0020 7d8903a6 4e800420   0044fb30 
> c000 3d62fffe 396bc388 6000  7d8903a6 4e800420  
> ---[ end trace 3b830c669dd7adb5 ]---
> 
> 
> Note that ibmvscsi_queuecommand() handle TOC and int_to_scsilun()
> does not handle TOC
> 
> $> objdump -hdr  drivers/scsi/ibmvscsi/ibmvscsi.ko
> 20c0 :
>

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Petr Mladek]

On Tue 2016-02-16 11:39:07, Torsten Duwe wrote:
> On Tue, Feb 16, 2016 at 04:00:30PM +0530, Kamalesh Babulal wrote:
> > * Torsten Duwe  [2016-02-16 09:23:02]:
> > > 
> > > N.b.: if you try to livepatch/trace such a leaf function without
> > > global dependencies, it will crash if that function got called with
> > > a different TOC value. Hence this whole testing.
> > > 
> > 
> > I am running out of ideas on how to generate this crash, any pointers
> > will be helpful.
> 
> Petr discovered some modular SCSI helper function does it.

The kernel crashes here when I build it _with_ modules and
enable a tracer.

I see this:

$> lsmod
Module  Size  Used by
af_packet  46767  0 
dm_mod148307  0 
e1000 184154  0 
rtc_generic 2617  0 
ext4  716333  1 
crc16   2307  1 ext4
mbcache14237  1 ext4
jbd2  143496  1 ext4
sd_mod 49002  3 
sr_mod 22532  0 
cdrom  65024  1 sr_mod
ibmvscsi   34139  2 
scsi_transport_srp 18979  1 ibmvscsi
sg 44568  0 
scsi_mod  289641  5 sg,scsi_transport_srp,ibmvscsi,sd_mod,sr_mod
autofs448256  0 

echo function >/sys/kernel/debug/tracing/current_tracer

BANG!


Some dugging has shown an Oops in the fucntion int_to_scsilun()
called from ibmvscsi_queuecommand(). So, I rebooted and
did the following test:

$> echo ibmvscsi_queuecommand >/sys/kernel/debug/tracing/set_ftrace_filter
$> echo function > /sys/kernel/debug/tracing/current_tracer 
$> echo 1 > /sys/kernel/debug/tracing/tracing_on
$> cat /sys/kernel/debug/tracing/trace
# tracer: function
#
# entries-in-buffer/entries-written: 7/7   #P:4
#
#  _-=> irqs-off
# / _=> need-resched
#| / _---=> hardirq/softirq
#|| / _--=> preempt-depth
#||| / delay
#   TASK-PID   CPU#  TIMESTAMP  FUNCTION
#  | |   |      | |
bash-3488  [000]    100.278622: ibmvscsi_queuecommand 
<-scsi_dispatch_cmd
 kworker/1:2-223   [001]    101.048569: ibmvscsi_queuecommand 
<-scsi_dispatch_cmd
 kworker/1:2-223   [001]    103.048575: ibmvscsi_queuecommand 
<-scsi_dispatch_cmd
 jbd2/sda3-8-1021  [003]    104.008645: ibmvscsi_queuecommand 
<-scsi_dispatch_cmd
 jbd2/sda3-8-1021  [003]    104.008883: ibmvscsi_queuecommand 
<-scsi_dispatch_cmd
  -0 [000] ..s.   104.017672: ibmvscsi_queuecommand 
<-scsi_dispatch_cmd
  -0 [003] ..s.   104.017771: ibmvscsi_queuecommand 
<-scsi_dispatch_cmd

It means that ibmvscsi_queuecommand can be traced. Then I did

c79:/sys/kernel/debug/tracing # echo int_to_scsilun >set_ftrace_filter

BANG!

Unable to handle kernel paging request for data at address 0xd108b148
Faulting instruction address: 0xd0bde35c
Oops: Kernel access of bad area, sig: 11 [#1]
SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: af_packet(E) dm_mod(E) e1000(E) rtc_generic(E) ext4(E) 
crc16(E) mbcache(E) jbd2(E) sr_mod(E) cdrom(E) sd_mod(E) ibmvscsi(E) 
scsi_transport_srp(E) sg(E) scsi_mod(E) autofs4(E)
CPU: 1 PID: 223 Comm: kworker/1:2 Tainted: GE   
4.5.0-rc2-11-default+ #90
Workqueue: events_freezable_power_ disk_events_workfn
task: c000f7d99aa0 ti: c000f7cb8000 task.ti: c000f7cb8000
NIP: d0bde35c LR: d0bcffec CTR: d0bcffe0
REGS: c000f7cbb3b0 TRAP: 0300   Tainted: GE
(4.5.0-rc2-11-default+)
MSR: 80019033   CR: 24c82220  XER: 
CFAR: d0bdd144 DAR: d108b148 DSISR: 4000 SOFTE: 0 
GPR00: d10a21cc c000f7cbb630 d10aeda0 8200 
GPR04: c000fa0b33fc 014a c000fa0b3408 0010 
GPR08: 0008 c370a4e0  d108b128 
GPR12: d0bcffe0 c7e80300 c00d8b18 c000fe04ba80 
GPR16:  c000f7b6f208  0001 
GPR20: c000f7b6f144 c000f7b6f140 d0bcbcf0  
GPR24: 0200  0001 c000f7b6f810 
GPR28: c000f7b6f000 c000f7b6f000 c000fa0b33a0 c000fe837e00 
NIP [d0bde35c] scsi_inq_str+0x21b0/0x41ac [scsi_mod]
LR [d0bcffec] int_to_scsilun+0xc/0x60 [scsi_mod]
Call Trace:
[c000f7cbb630] [d10a21cc] ibmvscsi_queuecommand+0x10c/0x4e0 
[ibmvscsi] (unreliable)
[c000f7cbb6e0] [d0bcbea8] scsi_dispatch_cmd+0xe8/0x2c0 [scsi_mod]
[c000f7cbb760] [d0bceb0c] scsi_request_fn+0x50c/0x8b0 [scsi_mod]
[c000f7cbb850] [c0407280] __blk_run_queue+0x60/0x90
[c000f7cbb880] [c0413640] blk_execute_rq_nowait+0x100/0x1a0
[c000f7cbb8d0] 

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Torsten Duwe]

On Tue, Feb 16, 2016 at 04:00:30PM +0530, Kamalesh Babulal wrote:
> * Torsten Duwe  [2016-02-16 09:23:02]:
> > 
> > N.b.: if you try to livepatch/trace such a leaf function without
> > global dependencies, it will crash if that function got called with
> > a different TOC value. Hence this whole testing.
> > 
> 
> I am running out of ideas on how to generate this crash, any pointers
> will be helpful.

Petr discovered some modular SCSI helper function does it.

HTH,
Torsten

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

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Kamalesh Babulal]

* Torsten Duwe  [2016-02-16 09:23:02]:

> On Tue, Feb 16, 2016 at 11:17:02AM +0530, Kamalesh Babulal wrote:
> > * Petr Mladek  [2016-02-12 17:45:17]:
> > >  int test(int a)
> > >  {
> > > + printk("%d\n", a);
> > >   return ++a;
> > >  }
> > 
> > Thanks. This workaround, helped to load sample livepatch module.
> 
> N.b.: if you try to livepatch/trace such a leaf function without
> global dependencies, it will crash if that function got called with
> a different TOC value. Hence this whole testing.
> 

I am running out of ideas on how to generate this crash, any pointers
will be helpful.

> You may alternatively try my gcc patch ;-)

Thank you. I will give the patch a try.

Regards,
Kamalesh

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

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Torsten Duwe]

On Tue, Feb 16, 2016 at 11:17:02AM +0530, Kamalesh Babulal wrote:
> * Petr Mladek  [2016-02-12 17:45:17]:
> >  int test(int a)
> >  {
> > +   printk("%d\n", a);
> > return ++a;
> >  }
> 
> Thanks. This workaround, helped to load sample livepatch module.

N.b.: if you try to livepatch/trace such a leaf function without
global dependencies, it will crash if that function got called with
a different TOC value. Hence this whole testing.

You may alternatively try my gcc patch ;-)

Another caveat is functions with stack arguments (>8 args, varargs).
My code needs special precautions then because of the return helper.

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

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Kamalesh Babulal]

* Petr Mladek  [2016-02-12 17:45:17]:

[...]
> I guess that you used a broken gcc and cheated the check
> to pass the compilation. Did you, please?
> 
> The test used to detect the offset is using a minimalistic
> function is is afftected by the gcc bug.
> 
> The patch below might be used to cheat the offset check as well.
> 
> Torsten, please mention this somewhere if you, just by chance,
> send a new version of the patchset.
> 
> From f6a438a3f2f60cc1acc859b41d0cc9259c9a331e Mon Sep 17 00:00:00 2001
> From: root 
> Date: Tue, 2 Feb 2016 15:35:06 +0100
> Subject: [PATCH 2/2] livepatch: Make sure the TOC is handled when detecting
>  ftrace location
> 
> There seems to be a bug in gcc on PPC. It does not handle TOC
> if the function does not access global variables or functions
> by default. But it should when profiling is enabled.
> 
> This patch works around this problem by adding a call
> to a global function.
> 
> This patch is for testing only!
> ---
>  kernel/livepatch/ftrace-test.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/livepatch/ftrace-test.c b/kernel/livepatch/ftrace-test.c
> index 22f0c54bf7b3..a3b7aabb67e5 100644
> --- a/kernel/livepatch/ftrace-test.c
> +++ b/kernel/livepatch/ftrace-test.c
> @@ -1,6 +1,9 @@
>  /* Sample code to figure out mcount location offset */
> +#include 
> +
>  
>  int test(int a)
>  {
> + printk("%d\n", a);
>   return ++a;
>  }

Thanks. This workaround, helped to load sample livepatch module.

Thanks,
Kamalesh.

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

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Petr Mladek]

On Sat 2016-02-13 03:13:29, Balbir Singh wrote:
> On Thu, 2016-01-28 at 16:32 +0100, Torsten Duwe wrote:
> > From: Petr Mladek 
> > 
> > Livepatch works on x86_64 and s390 only when the ftrace call
> > is at the very beginning of the function. But PPC is different.
> > We need to handle TOC and save LR there before calling the
> > global ftrace handler.
> > 
> > Now, the problem is that the extra operations have different
> > length on PPC depending on the used gcc version. It is
> > 4 instructions (16 bytes) before gcc-6 and only 3 instructions
> > (12 bytes) with gcc-6.
> > 
> > This patch tries to detect the offset a generic way during
> > build. It assumes that the offset of the ftrace location
> > is the same for all functions. It modifies the existing
> > recordmcount tool that is able to find read mcount locations
> > directly from the object files. It adds an option -p
> > to print the first found offset.
> > 
> > The recordmcount tool is then used in the kernel/livepatch
> > subdirectory to generate a header file. It defines
> > a constant that is used to compute the ftrace location
> > from the function address.
> > 
> > Finally, we have to enable the C implementation of the
> > recordmcount tool to be used on PPC and S390. It seems
> > to work fine there. It should be more reliable because
> > it reads the standardized elf structures. The old perl
> > implementation uses rather complex regular expressions
> > to parse objdump output and is therefore much more tricky.
> 
> I'm still missing something, I'm getting offset as 8
>
> When I run, I get
> 
> scripts/recordmcount -p kernel/livepatch/core.o 
> #define KLP_FTRACE_LOCATION 8
> 
> scripts/recordmcount -p kernel/livepatch/ftrace-test.o 
> #define KLP_FTRACE_LOCATION 8
> 
> My sample fails as well, since the expected offset is 16.
> I guess the script is being run against a not so good
> test.

I guess that you used a broken gcc and cheated the check
to pass the compilation. Did you, please?

The test used to detect the offset is using a minimalistic
function is is afftected by the gcc bug.

The patch below might be used to cheat the offset check as well.

Torsten, please mention this somewhere if you, just by chance,
send a new version of the patchset.

From f6a438a3f2f60cc1acc859b41d0cc9259c9a331e Mon Sep 17 00:00:00 2001
From: root 
Date: Tue, 2 Feb 2016 15:35:06 +0100
Subject: [PATCH 2/2] livepatch: Make sure the TOC is handled when detecting
 ftrace location

There seems to be a bug in gcc on PPC. It does not handle TOC
if the function does not access global variables or functions
by default. But it should when profiling is enabled.

This patch works around this problem by adding a call
to a global function.

This patch is for testing only!
---
 kernel/livepatch/ftrace-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/livepatch/ftrace-test.c b/kernel/livepatch/ftrace-test.c
index 22f0c54bf7b3..a3b7aabb67e5 100644
--- a/kernel/livepatch/ftrace-test.c
+++ b/kernel/livepatch/ftrace-test.c
@@ -1,6 +1,9 @@
 /* Sample code to figure out mcount location offset */
+#include 
+
 
 int test(int a)
 {
+   printk("%d\n", a);
return ++a;
 }
-- 
1.8.5.6

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

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Balbir Singh]

On Thu, 2016-01-28 at 16:32 +0100, Torsten Duwe wrote:
> From: Petr Mladek 
> 
> Livepatch works on x86_64 and s390 only when the ftrace call
> is at the very beginning of the function. But PPC is different.
> We need to handle TOC and save LR there before calling the
> global ftrace handler.
> 
> Now, the problem is that the extra operations have different
> length on PPC depending on the used gcc version. It is
> 4 instructions (16 bytes) before gcc-6 and only 3 instructions
> (12 bytes) with gcc-6.
> 
> This patch tries to detect the offset a generic way during
> build. It assumes that the offset of the ftrace location
> is the same for all functions. It modifies the existing
> recordmcount tool that is able to find read mcount locations
> directly from the object files. It adds an option -p
> to print the first found offset.
> 
> The recordmcount tool is then used in the kernel/livepatch
> subdirectory to generate a header file. It defines
> a constant that is used to compute the ftrace location
> from the function address.
> 
> Finally, we have to enable the C implementation of the
> recordmcount tool to be used on PPC and S390. It seems
> to work fine there. It should be more reliable because
> it reads the standardized elf structures. The old perl
> implementation uses rather complex regular expressions
> to parse objdump output and is therefore much more tricky.

I'm still missing something, I'm getting offset as 8

When I run, I get

scripts/recordmcount -p kernel/livepatch/core.o 
#define KLP_FTRACE_LOCATION 8

scripts/recordmcount -p kernel/livepatch/ftrace-test.o 
#define KLP_FTRACE_LOCATION 8

My sample fails as well, since the expected offset is 16.
I guess the script is being run against a not so good
test.

A quick hack (no signoff below, its just an experiment),
seems to do the trick for the provided sample-livepatch.
It is hacky because it uses the sample object and due to
lack of a better description of srctree, it uses 
srctree/../..

I suspect the usage of recordmcount needs to be revisited

diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index 65a44b68..10b5f38 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -2,7 +2,7 @@ obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
 livepatch-objs := core.o
 
-always := $(hostprogs-y) ftrace-test.o
+always := $(hostprogs-y) 
$(srctree)/../../samples/livepatch/livepatch-sample.o
 
 # dependencies on generated files need to be listed explicitly
 $(obj)/core.o: $(obj)/livepatch-ftrace.h
@@ -10,7 +10,7 @@ $(obj)/core.o: $(obj)/livepatch-ftrace.h
 quiet_cmd_livepatch-rmcount = RMCOUNT $@
   cmd_livepatch-rmcount = $(objtree)/scripts/recordmcount -p $< > $@
 
-$(obj)/livepatch-ftrace.h: $(obj)/ftrace-test.o $(objtree)/scripts/recordmcount
+$(obj)/livepatch-ftrace.h: $(obj)/../../samples/livepatch/livepatch-sample.o 
$(objtree)/scripts/recordmcount
    $(call if_changed,livepatch-rmcount)
 
 targets += livepatch-ftrace.h

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

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Balbir Singh]

On Fri, 2016-02-12 at 17:45 +0100, Petr Mladek wrote:
> On Sat 2016-02-13 03:13:29, Balbir Singh wrote:
> > On Thu, 2016-01-28 at 16:32 +0100, Torsten Duwe wrote:
> > > From: Petr Mladek 
> > > 
> > > Livepatch works on x86_64 and s390 only when the ftrace call
> > > is at the very beginning of the function. But PPC is different.
> > > We need to handle TOC and save LR there before calling the
> > > global ftrace handler.
> > > 
> > > Now, the problem is that the extra operations have different
> > > length on PPC depending on the used gcc version. It is
> > > 4 instructions (16 bytes) before gcc-6 and only 3 instructions
> > > (12 bytes) with gcc-6.
> > > 
> > > This patch tries to detect the offset a generic way during
> > > build. It assumes that the offset of the ftrace location
> > > is the same for all functions. It modifies the existing
> > > recordmcount tool that is able to find read mcount locations
> > > directly from the object files. It adds an option -p
> > > to print the first found offset.
> > > 
> > > The recordmcount tool is then used in the kernel/livepatch
> > > subdirectory to generate a header file. It defines
> > > a constant that is used to compute the ftrace location
> > > from the function address.
> > > 
> > > Finally, we have to enable the C implementation of the
> > > recordmcount tool to be used on PPC and S390. It seems
> > > to work fine there. It should be more reliable because
> > > it reads the standardized elf structures. The old perl
> > > implementation uses rather complex regular expressions
> > > to parse objdump output and is therefore much more tricky.
> > 
> > I'm still missing something, I'm getting offset as 8
> > 
> > When I run, I get
> > 
> > scripts/recordmcount -p kernel/livepatch/core.o 
> > #define KLP_FTRACE_LOCATION 8
> > 
> > scripts/recordmcount -p kernel/livepatch/ftrace-test.o 
> > #define KLP_FTRACE_LOCATION 8
> > 
> > My sample fails as well, since the expected offset is 16.
> > I guess the script is being run against a not so good
> > test.
> 
> I guess that you used a broken gcc and cheated the check
> to pass the compilation. Did you, please?
> 
> The test used to detect the offset is using a minimalistic
> function is is afftected by the gcc bug.
> 
> The patch below might be used to cheat the offset check as well.
> 
> Torsten, please mention this somewhere if you, just by chance,
> send a new version of the patchset.
> 
> From f6a438a3f2f60cc1acc859b41d0cc9259c9a331e Mon Sep 17 00:00:00 2001
> From: root 
> Date: Tue, 2 Feb 2016 15:35:06 +0100
> Subject: [PATCH 2/2] livepatch: Make sure the TOC is handled when detecting
>  ftrace location
> 
> There seems to be a bug in gcc on PPC. It does not handle TOC
> if the function does not access global variables or functions
> by default. But it should when profiling is enabled.
> 

Yep.. Please see see http://marc.info/?l=linux-kernel=145518015816435=2
and my question at http://marc.info/?l=linuxppc-embedded=145518330317496=2

> This patch works around this problem by adding a call
> to a global function.
> 
> This patch is for testing only!
> ---
>  kernel/livepatch/ftrace-test.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/livepatch/ftrace-test.c b/kernel/livepatch/ftrace-test.c
> index 22f0c54bf7b3..a3b7aabb67e5 100644
> --- a/kernel/livepatch/ftrace-test.c
> +++ b/kernel/livepatch/ftrace-test.c
> @@ -1,6 +1,9 @@
>  /* Sample code to figure out mcount location offset */
> +#include 
> +
>  
>  int test(int a)
>  {
> + printk("%d\n", a);
>   return ++a;
>  }

This is much better, I see the offset of 16.

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

[PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Torsten Duwe]

From: Petr Mladek 

Livepatch works on x86_64 and s390 only when the ftrace call
is at the very beginning of the function. But PPC is different.
We need to handle TOC and save LR there before calling the
global ftrace handler.

Now, the problem is that the extra operations have different
length on PPC depending on the used gcc version. It is
4 instructions (16 bytes) before gcc-6 and only 3 instructions
(12 bytes) with gcc-6.

This patch tries to detect the offset a generic way during
build. It assumes that the offset of the ftrace location
is the same for all functions. It modifies the existing
recordmcount tool that is able to find read mcount locations
directly from the object files. It adds an option -p
to print the first found offset.

The recordmcount tool is then used in the kernel/livepatch
subdirectory to generate a header file. It defines
a constant that is used to compute the ftrace location
from the function address.

Finally, we have to enable the C implementation of the
recordmcount tool to be used on PPC and S390. It seems
to work fine there. It should be more reliable because
it reads the standardized elf structures. The old perl
implementation uses rather complex regular expressions
to parse objdump output and is therefore much more tricky.

Signed-off-by: Petr Mladek 
Signed-off-by: Torsten Duwe 
---
 arch/powerpc/Kconfig   |  1 +
 arch/s390/Kconfig  |  1 +
 kernel/livepatch/Makefile  | 13 +
 kernel/livepatch/core.c| 12 +---
 kernel/livepatch/ftrace-test.c |  6 ++
 scripts/recordmcount.c |  6 +-
 scripts/recordmcount.h | 17 +++--
 7 files changed, 50 insertions(+), 6 deletions(-)
 create mode 100644 kernel/livepatch/ftrace-test.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 8c7a327..a546829 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -93,6 +93,7 @@ config PPC
select OF_EARLY_FLATTREE
select OF_RESERVED_MEM
select HAVE_FTRACE_MCOUNT_RECORD
+   select HAVE_C_RECORDMCOUNT
select HAVE_DYNAMIC_FTRACE
select HAVE_DYNAMIC_FTRACE_WITH_REGS if PPC64 && CPU_LITTLE_ENDIAN
select HAVE_FUNCTION_TRACER
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 3be9c83..c574bc4 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -121,6 +121,7 @@ config S390
select HAVE_ARCH_TRACEHOOK
select HAVE_ARCH_TRANSPARENT_HUGEPAGE
select HAVE_BPF_JIT if PACK_STACK && HAVE_MARCH_Z196_FEATURES
+   select HAVE_C_RECORDMCOUNT
select HAVE_CMPXCHG_DOUBLE
select HAVE_CMPXCHG_LOCAL
select HAVE_DEBUG_KMEMLEAK
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index e8780c0..65a44b6 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,16 @@
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
 livepatch-objs := core.o
+
+always := $(hostprogs-y) ftrace-test.o
+
+# dependencies on generated files need to be listed explicitly
+$(obj)/core.o: $(obj)/livepatch-ftrace.h
+
+quiet_cmd_livepatch-rmcount = RMCOUNT $@
+  cmd_livepatch-rmcount = $(objtree)/scripts/recordmcount -p $< > $@
+
+$(obj)/livepatch-ftrace.h: $(obj)/ftrace-test.o $(objtree)/scripts/recordmcount
+   $(call if_changed,livepatch-rmcount)
+
+targets += livepatch-ftrace.h
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bc2c85c..864d589 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -30,6 +30,8 @@
 #include 
 #include 
 
+#include "livepatch-ftrace.h"
+
 /**
  * struct klp_ops - structure for tracking registered ftrace ops structs
  *
@@ -312,8 +314,10 @@ static void klp_disable_func(struct klp_func *func)
return;
 
if (list_is_singular(>func_stack)) {
+   unsigned long ftrace_loc = func->old_addr + KLP_FTRACE_LOCATION;
+
WARN_ON(unregister_ftrace_function(>fops));
-   WARN_ON(ftrace_set_filter_ip(>fops, func->old_addr, 1, 0));
+   WARN_ON(ftrace_set_filter_ip(>fops, ftrace_loc, 1, 0));
 
list_del_rcu(>stack_node);
list_del(>node);
@@ -338,6 +342,8 @@ static int klp_enable_func(struct klp_func *func)
 
ops = klp_find_ops(func->old_addr);
if (!ops) {
+   unsigned long ftrace_loc = func->old_addr + KLP_FTRACE_LOCATION;
+
ops = kzalloc(sizeof(*ops), GFP_KERNEL);
if (!ops)
return -ENOMEM;
@@ -352,7 +358,7 @@ static int klp_enable_func(struct klp_func *func)
INIT_LIST_HEAD(>func_stack);
list_add_rcu(>stack_node, >func_stack);
 
-   ret = ftrace_set_filter_ip(>fops, func->old_addr, 0, 0);
+   ret = ftrace_set_filter_ip(>fops, ftrace_loc, 0, 0);
if (ret) {
pr_err("failed to set ftrace filter for function '%s'