Re: [PATCH v1 0/5] Implement livepatch on PPC32

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 13/12/2021 à 18:15, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 14:39:15 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>>> Note, you can implement this first, (I looked over the patches and they
>>> seem fine) and then update both ppc64 and ppc32 to implement
>>> DYNAMIC_FTRACE_WITH_ARGS.
>>>    
>>
>> I tried to activate DYNAMIC_FTRACE_WITH_ARGS on PPC32.
>>
>> I copied into powerpc the changes from 5740a7c71ab6 ("s390/ftrace: add
>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>
>> Ftrace selftests tell "Testing tracer function_graph: FAILED!".
>>
>> Is there anything else to do ?
> 
> Yes. Because BPF is now hooking into the function callbacks, it causes
> issues with function graph tracer. So what we did was to have function
> graph tracing to now use the function tracer callback as well (this allows
> both the BPF direct trampolines to work with function graph tracer).
> 
> As it requires DYNAMIC_FTRACE_WITH_ARGS, and x86 was the only one to
> support that for now, I decided to make all the archs change function graph
> tracing when they implement DYNAMIC_FTRACE_WITH_ARGS too. (It is becoming a
> pain to have too many variants of function tracing between the archs).
> 
> The change that did this for x86 was:
> 
> 0c0593b45c9b4 ("x86/ftrace: Make function graph use ftrace directly")
> 
> This actually simplifies the function graph tracer, as you no longer need
> it's own entry trampoline (still need the trampoline for the return of the
> function).
> 
> What you need to do is:
> 
> In your arch/*/include/asm/ftrace.h add:
> 
> struct ftrace_ops;
> 
> #define ftrace_graph_func ftrace_graph_func
> void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>                        struct ftrace_ops *op, struct ftrace_regs *fregs);
> 
> 
> Where ftrace_graph_func() is now what is called for the function graph
> tracer, directly from the ftrace callbacks (no longer a secondary
> trampoline).
> 
> Define the ftrace_graph_func() to be something like:
> 
> void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>                        struct ftrace_ops *op, struct ftrace_regs *fregs)
> {
>         struct pt_regs *regs = &fregs->regs;
>         unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);
> 
>         prepare_ftrace_return(ip, (unsigned long *)stack, 0);
> }
> 
> This is called by the function tracer code. But because with
> DYNAMIC_FTRACE_WITH_ARGS, we have access to the argument register, we should
> also have access to the link register and the stack. Then you can use that
> to modify the stack and or link register to jump to the the return
> trampoline.
> 
> This should all work with powerpc (both 64 and 32) but if it does not, let
> me know. I'm happy to help out.
> 

Thanks, I will try that.

I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't 
have a working function tracer anymore ?

I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use 
ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace: 
add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.

Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Mon, 13 Dec 2021 17:30:48 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> Thanks, I will try that.
> 
> I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't 
> have a working function tracer anymore ?
> 
> I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use 
> ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace: 
> add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.

Hmm, maybe not. I can't test it.

This needs to be fixed if that's the case.

Thanks for bringing it up!

-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 17:30:48 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> Thanks, I will try that.
>>
>> I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
>> have a working function tracer anymore ?
>>
>> I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
>> ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
>> add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
> 
> Hmm, maybe not. I can't test it.
> 
> This needs to be fixed if that's the case.
> 
> Thanks for bringing it up!
> 

On PPC32, I did your suggested changes, I get an Oops:

[    8.038441] Testing tracer function_graph:
[    8.064147] Kernel attempted to read user page (4) - exploit attempt? 
(uid: 0)
[    8.075296] Kernel attempted to read user page (4) - exploit attempt? 
(uid: 0)
[    8.082424] BUG: Kernel NULL pointer dereference on read at 0x00000004
[    8.088864] Faulting instruction address: 0xc001468c
[    8.093778] Oops: Kernel access of bad area, sig: 11 [#1]
[    8.099105] BE PAGE_SIZE=16K PREEMPT CMPC885
[    8.103329] Modules linked in:
[    8.106340] CPU: 0 PID: 1 Comm: swapper Not tainted 
5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty #732
[    8.115461] NIP:  c001468c LR: c00c8414 CTR: c0014674
[    8.120448] REGS: c902ba00 TRAP: 0300   Not tainted 
(5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty)
[    8.129398] MSR:  00001032 <ME,IR,DR,RI>  CR: 88022252  XER: 20000000
[    8.135853] DAR: 00000004 DSISR: c0000000
[    8.135853] GPR00: c00c8414 c902bac0 c2140000 c0015260 c0003ac4 
c122db78 00000000 00000300
[    8.135853] GPR08: c2140000 c0014674 c0015260 00000000 2802b252 
00000000 c0004f38 00000000
[    8.135853] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000010 c1037d1c c12d0000
[    8.135853] GPR24: c121c440 c12b5380 c12b0000 c0003ac4 c0015260 
00000000 00000200 c0015260
[    8.174493] NIP [c001468c] ftrace_graph_func+0x18/0x74
[    8.179572] LR [c00c8414] arch_ftrace_ops_list_func+0x118/0x230
[    8.185430] Call Trace:
[    8.187837] [c902bac0] [c12b5380] ftrace_list_end+0x0/0x50 (unreliable)
[    8.194379] [c902bad0] [c00c8414] arch_ftrace_ops_list_func+0x118/0x230
[    8.200920] [c902bb20] [c001475c] ftrace_call+0x4/0x44
[    8.205997] [c902bb50] [c0003ac4] DataTLBError_virt+0x114/0x118
[    8.211848] --- interrupt: 300 at ftrace_graph_func+0x18/0x74
[    8.217527] NIP:  c001468c LR: c00c8414 CTR: c0014674
[    8.222516] REGS: c902bb60 TRAP: 0300   Not tainted 
(5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty)
[    8.231466] MSR:  00001032 <ME,IR,DR,RI>  CR: 82002842  XER: 20000000
[    8.237920] DAR: 00000004 DSISR: c0000000
[    8.237920] GPR00: c00c8414 c902bc20 c2140000 c001573c c001624c 
c122db78 00000000 00000100
[    8.237920] GPR08: c2140000 c0014674 c001573c 00000000 22004842 
00000000 c0004f38 00000000
[    8.237920] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000010 c1037d1c c12d0000
[    8.237920] GPR24: c121c440 c12b5380 c12b0000 c001624c c001573c 
00000000 00000100 c001573c
[    8.276561] NIP [c001468c] ftrace_graph_func+0x18/0x74
[    8.281639] LR [c00c8414] arch_ftrace_ops_list_func+0x118/0x230
[    8.287491] --- interrupt: 300
[    8.290508] [c902bc20] [00000001] 0x1 (unreliable)
[    8.295242] [c902bc30] [c00c8414] arch_ftrace_ops_list_func+0x118/0x230
[    8.301782] [c902bc80] [c001475c] ftrace_call+0x4/0x44
[    8.306860] [c902bcb0] [c001624c] map_kernel_page+0xc8/0x12c
[    8.312454] [c902bd00] [c0019cb0] patch_instruction+0xbc/0x278
[    8.318221] [c902bd30] [c0013964] ftrace_modify_code+0x38/0xc4
[    8.323986] [c902bd70] [c00c2c0c] ftrace_replace_code+0x78/0xec
[    8.329838] [c902bd90] [c00c2e30] ftrace_modify_all_code+0xd0/0x148
[    8.336035] [c902bdb0] [c00c2f38] ftrace_run_update_code+0x28/0x88
[    8.342145] [c902bdc0] [c00c75dc] ftrace_startup+0x118/0x1e0
[    8.347739] [c902bde0] [c00e8310] register_ftrace_graph+0x334/0x3c0
[    8.353935] [c902be20] [c100ccf4] 
trace_selftest_startup_function_graph+0x64/0x164
[    8.361422] [c902be50] [c00debc0] run_tracer_selftest+0x120/0x1b4
[    8.367447] [c902be70] [c100c74c] register_tracer+0x14c/0x218
[    8.373126] [c902be90] [c0004a30] do_one_initcall+0x44/0x1e8
[    8.378720] [c902bef0] [c10011f4] kernel_init_freeable+0x1a8/0x250
[    8.384831] [c902bf20] [c0004f68] kernel_init+0x30/0x150
[    8.390081] [c902bf30] [c001322c] ret_from_kernel_thread+0x5c/0x64
[    8.396193] Instruction dump:
[    8.399115] 83c10018 83e1001c 3863489c 7c0803a6 38210020 4e800020 
9421fff0 7c0802a6
[    8.407031] 93e1000c 90010014 93c10008 7c7f1b78 <83c60004> 480d343d 
2c030000 40820038
[    8.415154] ---[ end trace 717d695f81a0970d ]---

I also tried with the additional change below, but still the same:

int ftrace_enable_ftrace_graph_caller(void)
{
	return 0;
}

int ftrace_disable_ftrace_graph_caller(void)
{
	return 0;
}

Anything else to do ?

Full change below:

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
  	select HAVE_DEBUG_KMEMLEAK
  	select HAVE_DEBUG_STACKOVERFLOW
  	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if MPROFILE_KERNEL || PPC32
  	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL || PPC32
  	select HAVE_EBPF_JIT
  	select HAVE_EFFICIENT_UNALIGNED_ACCESS	if !(CPU_LITTLE_ENDIAN && 
POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h 
b/arch/powerpc/include/asm/ftrace.h
index debe8c4f7062..68f503294342 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -59,9 +59,24 @@ static inline unsigned long 
ftrace_call_adjust(unsigned long addr)
  struct dyn_arch_ftrace {
  	struct module *mod;
  };
+
+struct ftrace_regs {
+	struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
ftrace_regs *fregs)
+{
+	return &fregs->regs;
+}
+
+struct ftrace_ops;
+
+#define ftrace_graph_func ftrace_graph_func
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs);
  #endif /* __ASSEMBLY__ */

-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#if defined(CONFIG_DYNAMIC_FTRACE_WITH_REGS) || 
defined(CONFIG_DYNAMIC_FTRACE_WITH_ARGS)
  #define ARCH_SUPPORTS_FTRACE_OPS 1
  #endif
  #endif /* CONFIG_FUNCTION_TRACER */
diff --git a/arch/powerpc/kernel/trace/ftrace.c 
b/arch/powerpc/kernel/trace/ftrace.c
index 80b6285769f2..7662c88c4c0c 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -912,28 +912,12 @@ extern void ftrace_graph_stub(void);

  int ftrace_enable_ftrace_graph_caller(void)
  {
-	unsigned long ip = (unsigned long)(&ftrace_graph_call);
-	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
-	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
-	ppc_inst_t old, new;
-
-	old = ftrace_call_replace(ip, stub, 0);
-	new = ftrace_call_replace(ip, addr, 0);
-
-	return ftrace_modify_code(ip, old, new);
+	return 0;
  }

  int ftrace_disable_ftrace_graph_caller(void)
  {
-	unsigned long ip = (unsigned long)(&ftrace_graph_call);
-	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
-	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
-	ppc_inst_t old, new;
-
-	old = ftrace_call_replace(ip, addr, 0);
-	new = ftrace_call_replace(ip, stub, 0);
-
-	return ftrace_modify_code(ip, old, new);
+	return 0;
  }

  /*
@@ -958,6 +942,12 @@ unsigned long prepare_ftrace_return(unsigned long 
parent, unsigned long ip,
  out:
  	return parent;
  }
+
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+	prepare_ftrace_return(ip, kernel_stack_pointer(&fregs->regs), 0);
+}
  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */

  #ifdef PPC64_ELF_ABI_v1


Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Mon, 13 Dec 2021 17:50:52 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> @@ -958,6 +942,12 @@ unsigned long prepare_ftrace_return(unsigned long 
> parent, unsigned long ip,
>   out:
>   	return parent;
>   }
> +
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> +		       struct ftrace_ops *op, struct ftrace_regs *fregs)
> +{
> +	prepare_ftrace_return(ip, kernel_stack_pointer(&fregs->regs), 0);
> +}

I have for powerpc prepare_ftrace_return as:


unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
                                                unsigned long sp)
{
        unsigned long return_hooker;

        if (unlikely(ftrace_graph_is_dead()))
                goto out;

        if (unlikely(atomic_read(&current->tracing_graph_pause)))
                goto out;

        return_hooker = ppc_function_entry(return_to_handler);

        if (!function_graph_enter(parent, ip, 0, (unsigned long *)sp))
                parent = return_hooker;
out:
        return parent;
}

Which means you'll need different parameters to it than what x86 has, which
has the prototype of:

void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
			   unsigned long frame_pointer)

and it does not use the frame_pointer for this case, which is why it is
zero.

For powerpc though, it uses the stack pointer, so you parameters are
incorrect. Looks like it should be:

	prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));

And that will likely not be enough. I'll need to update the ctr register,
as that is where the return address is saved. So you'll probably need it to be:

void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
		       struct ftrace_ops *op, struct ftrace_regs *fregs)
{
	unsigned long parent;

	parent = prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
	fregs->regs.ctr = parent;
}



-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 13/12/2021 à 19:54, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 17:50:52 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> @@ -958,6 +942,12 @@ unsigned long prepare_ftrace_return(unsigned long
>> parent, unsigned long ip,
>>    out:
>>    	return parent;
>>    }
>> +
>> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>> +		       struct ftrace_ops *op, struct ftrace_regs *fregs)
>> +{
>> +	prepare_ftrace_return(ip, kernel_stack_pointer(&fregs->regs), 0);
>> +}
> 
> I have for powerpc prepare_ftrace_return as:
> 
> 
> unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
>                                                  unsigned long sp)
> {
>          unsigned long return_hooker;
> 
>          if (unlikely(ftrace_graph_is_dead()))
>                  goto out;
> 
>          if (unlikely(atomic_read(&current->tracing_graph_pause)))
>                  goto out;
> 
>          return_hooker = ppc_function_entry(return_to_handler);
> 
>          if (!function_graph_enter(parent, ip, 0, (unsigned long *)sp))
>                  parent = return_hooker;
> out:
>          return parent;
> }
> 
> Which means you'll need different parameters to it than what x86 has, which
> has the prototype of:
> 
> void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
> 			   unsigned long frame_pointer)
> 
> and it does not use the frame_pointer for this case, which is why it is
> zero.
> 
> For powerpc though, it uses the stack pointer, so you parameters are
> incorrect. Looks like it should be:
> 
> 	prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
> 
> And that will likely not be enough. I'll need to update the ctr register,
> as that is where the return address is saved. So you'll probably need it to be:
> 
> void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> 		       struct ftrace_ops *op, struct ftrace_regs *fregs)
> {
> 	unsigned long parent;
> 
> 	parent = prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
> 	fregs->regs.ctr = parent;
> }
> 

STill the same Oops, below
I will look more closely tomorrow.

[    8.018219] Testing tracer function_graph:
[    8.043884] Kernel attempted to read user page (4) - exploit attempt? 
(uid: 0)
[    8.055074] Kernel attempted to read user page (4) - exploit attempt? 
(uid: 0)
[    8.062204] BUG: Kernel NULL pointer dereference on read at 0x00000004
[    8.068643] Faulting instruction address: 0xc0014694
[    8.073556] Oops: Kernel access of bad area, sig: 11 [#1]
[    8.078884] BE PAGE_SIZE=16K PREEMPT CMPC885
[    8.083109] Modules linked in:
[    8.086120] CPU: 0 PID: 1 Comm: swapper Not tainted 
5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty #733
[    8.095240] NIP:  c0014694 LR: c00c8434 CTR: c0014674
[    8.100227] REGS: c902b9e0 TRAP: 0300   Not tainted 
(5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty)
[    8.109178] MSR:  00001032 <ME,IR,DR,RI>  CR: 88022242  XER: 20000000
[    8.115632] DAR: 00000004 DSISR: c0000000
[    8.115632] GPR00: c00c8434 c902baa0 c2140000 c0015278 c0003ac4 
c122db78 00000000 00000300
[    8.115632] GPR08: c2140000 c0014674 c0015278 00000000 2802b242 
00000000 c0004f38 00000000
[    8.115632] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000010 c1037d1c c12d0000
[    8.115632] GPR24: c121c440 c12b5380 c12b0000 c0003ac4 c0015278 
00000000 00000000 c122db78
[    8.154272] NIP [c0014694] ftrace_graph_func+0x20/0x8c
[    8.159351] LR [c00c8434] arch_ftrace_ops_list_func+0x118/0x230
[    8.165208] Call Trace:
[    8.167616] [c902baa0] [c006c048] vprintk_emit+0x188/0x2a4 (unreliable)
[    8.174158] [c902bac0] [c00c8434] arch_ftrace_ops_list_func+0x118/0x230
[    8.180699] [c902bb10] [c0014774] ftrace_call+0x4/0x44
[    8.185776] [c902bb40] [c0003ac4] DataTLBError_virt+0x114/0x118
[    8.191627] --- interrupt: 300 at ftrace_graph_func+0x20/0x8c
[    8.197306] NIP:  c0014694 LR: c00c8434 CTR: c0014674
[    8.202296] REGS: c902bb50 TRAP: 0300   Not tainted 
(5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty)
[    8.211245] MSR:  00001032 <ME,IR,DR,RI>  CR: 82002842  XER: 20000000
[    8.217699] DAR: 00000004 DSISR: c0000000
[    8.217699] GPR00: c00c8434 c902bc10 c2140000 c0015754 c0016264 
c122db78 00000000 00000100
[    8.217699] GPR08: c2140000 c0014674 c0015754 00000000 22004842 
00000000 c0004f38 00000000
[    8.217699] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000010 c1037d1c c12d0000
[    8.217699] GPR24: c121c440 c12b5380 c12b0000 c0016264 c0015754 
00000000 00000000 c122db78
[    8.256340] NIP [c0014694] ftrace_graph_func+0x20/0x8c
[    8.261418] LR [c00c8434] arch_ftrace_ops_list_func+0x118/0x230
[    8.267270] --- interrupt: 300
[    8.270288] [c902bc10] [c00adb98] 
clockevents_program_event+0x108/0x254 (unreliable)
[    8.277947] [c902bc30] [c00c8434] arch_ftrace_ops_list_func+0x118/0x230
[    8.284488] [c902bc80] [c0014774] ftrace_call+0x4/0x44
[    8.289565] [c902bcb0] [c0016264] map_kernel_page+0xc8/0x12c
[    8.295159] [c902bd00] [c0019cc8] patch_instruction+0xbc/0x278
[    8.300926] [c902bd30] [c0013964] ftrace_modify_code+0x38/0xc4
[    8.306691] [c902bd70] [c00c2c2c] ftrace_replace_code+0x78/0xec
[    8.312543] [c902bd90] [c00c2e50] ftrace_modify_all_code+0xd0/0x148
[    8.318740] [c902bdb0] [c00c2f58] ftrace_run_update_code+0x28/0x88
[    8.324850] [c902bdc0] [c00c75fc] ftrace_startup+0x118/0x1e0
[    8.330443] [c902bde0] [c00e8330] register_ftrace_graph+0x334/0x3c0
[    8.336640] [c902be20] [c100ccf4] 
trace_selftest_startup_function_graph+0x64/0x164
[    8.344127] [c902be50] [c00debe0] run_tracer_selftest+0x120/0x1b4
[    8.350152] [c902be70] [c100c74c] register_tracer+0x14c/0x218
[    8.355832] [c902be90] [c0004a30] do_one_initcall+0x44/0x1e8
[    8.361425] [c902bef0] [c10011f4] kernel_init_freeable+0x1a8/0x250
[    8.367536] [c902bf20] [c0004f68] kernel_init+0x30/0x150
[    8.372785] [c902bf30] [c001322c] ret_from_kernel_thread+0x5c/0x64
[    8.378898] Instruction dump:
[    8.381821] 386348b4 7c0803a6 38210020 4e800020 9421ffe0 7c0802a6 
93a10014 93c10018
[    8.389737] 93e1001c 90010024 93810010 7cde3378 <83860004> 7c7d1b78 
7c9f2378 480d344d
[    8.397859] ---[ end trace 93333951fba49ac1 ]---


Thanks
Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Mon, 13 Dec 2021 19:33:47 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> STill the same Oops, below

Unfortunately, I don't have a PPC machine (32 nor 64 bit) to help debug
this.


> I will look more closely tomorrow.

OK, thanks.

-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 13/12/2021 à 20:46, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 19:33:47 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> STill the same Oops, below
> 
> Unfortunately, I don't have a PPC machine (32 nor 64 bit) to help debug
> this.
> 
> 
>> I will look more closely tomorrow.
> 
> OK, thanks.
> 

The Oops was due to ftrace_caller() setting the regs argument to NULL.

After fixing that, I'm back into a situation where I get "Testing tracer 
function_graph: FAILED!"

Will continue investigating.

Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 14/12/2021 à 07:09, Christophe Leroy a écrit :
> 
> 
> Le 13/12/2021 à 20:46, Steven Rostedt a écrit :
>> On Mon, 13 Dec 2021 19:33:47 +0000
>> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>>
>>> STill the same Oops, below
>>
>> Unfortunately, I don't have a PPC machine (32 nor 64 bit) to help debug
>> this.
>>
>>
>>> I will look more closely tomorrow.
>>
>> OK, thanks.
>>
> 
> The Oops was due to ftrace_caller() setting the regs argument to NULL.
> 
> After fixing that, I'm back into a situation where I get "Testing tracer 
> function_graph: FAILED!"
> 
> Will continue investigating.
> 

trace_selftest_startup_function_graph() calls register_ftrace_direct() 
which returns -ENOSUPP because powerpc doesn't select 
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS.

Should TEST_DIRECT_TRAMP depend on CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS ?

Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Tue, 14 Dec 2021 08:35:14 +0100
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> > Will continue investigating.
> >   
> 
> trace_selftest_startup_function_graph() calls register_ftrace_direct() 
> which returns -ENOSUPP because powerpc doesn't select 
> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS.
> 
> Should TEST_DIRECT_TRAMP depend on CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS ?

Yes, that should be:

#if defined(CONFIG_DYNAMIC_FTRACE) && \
    defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS)
#define TEST_DIRECT_TRAMP
noinline __noclone static void trace_direct_tramp(void) { }
#endif


And make it test it with or without the args.

Thanks for finding this.

-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Heiko Carstens]

On Mon, Dec 13, 2021 at 05:50:52PM +0000, Christophe Leroy wrote:
> Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
> > On Mon, 13 Dec 2021 17:30:48 +0000
> > Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> > 
> >> Thanks, I will try that.
> >>
> >> I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
> >> have a working function tracer anymore ?
> >>
> >> I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
> >> ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
> >> add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
> > 
> > Hmm, maybe not. I can't test it.
> > 
> > This needs to be fixed if that's the case.
> > 
> > Thanks for bringing it up!

It still works, we run the full ftrace/kprobes selftests from the
kernel every day on multiple machines with several kernels (besides
other Linus' tree, but also linux-next). That said, I wanted to change
s390's code follow what x86 is currently doing anyway.

One thing to note: commit 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support") looks only that simple because
ftrace_caller _and_ ftrace_regs_caller used to save all register
contents into the pt_regs structure, which never was a requirement,
but implicitly fulfills the HAVE_DYNAMIC_FTRACE_WITH_ARGS
requirements.
Not sure if powerpc passes enough register contents via pt_regs for
HAVE_DYNAMIC_FTRACE_WITH_ARGS though. Might be something to check?

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 14/12/2021 à 15:25, Heiko Carstens a écrit :
> On Mon, Dec 13, 2021 at 05:50:52PM +0000, Christophe Leroy wrote:
>> Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
>>> On Mon, 13 Dec 2021 17:30:48 +0000
>>> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>>>
>>>> Thanks, I will try that.
>>>>
>>>> I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
>>>> have a working function tracer anymore ?
>>>>
>>>> I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
>>>> ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
>>>> add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
>>>
>>> Hmm, maybe not. I can't test it.
>>>
>>> This needs to be fixed if that's the case.
>>>
>>> Thanks for bringing it up!
> 
> It still works, we run the full ftrace/kprobes selftests from the
> kernel every day on multiple machines with several kernels (besides
> other Linus' tree, but also linux-next). That said, I wanted to change
> s390's code follow what x86 is currently doing anyway.
> 
> One thing to note: commit 5740a7c71ab6 ("s390/ftrace: add
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support") looks only that simple because
> ftrace_caller _and_ ftrace_regs_caller used to save all register
> contents into the pt_regs structure, which never was a requirement,
> but implicitly fulfills the HAVE_DYNAMIC_FTRACE_WITH_ARGS
> requirements.
> Not sure if powerpc passes enough register contents via pt_regs for
> HAVE_DYNAMIC_FTRACE_WITH_ARGS though. Might be something to check?
> 

In fact there is no need to rework the function graph logic. It still 
works as is with HAVE_DYNAMIC_FTRACE_WITH_ARGS.

The problem was that the sefltests were failing with 
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS not being selected on powerpc.

As s390 selects CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, there is no 
problem.

Thanks
Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 14/12/2021 à 15:01, Steven Rostedt a écrit :
> On Tue, 14 Dec 2021 08:35:14 +0100
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>>> Will continue investigating.
>>>    
>>
>> trace_selftest_startup_function_graph() calls register_ftrace_direct()
>> which returns -ENOSUPP because powerpc doesn't select
>> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS.
>>
>> Should TEST_DIRECT_TRAMP depend on CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS ?
> 
> Yes, that should be:
> 
> #if defined(CONFIG_DYNAMIC_FTRACE) && \
>      defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS)
> #define TEST_DIRECT_TRAMP
> noinline __noclone static void trace_direct_tramp(void) { }
> #endif
> 
> 
> And make it test it with or without the args.
> 

Shouldn't it just be:

#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS

Because

register_ftrace_direct() depends on that symbol, so if you have 
CONFIG_DYNAMIC_FTRACE && CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS 
but not DYNAMIC_FTRACE_WITH_REGS then 
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is unset and 
register_ftrace_direct() returns -ENOTSUPP

Christophe