Message-ID: <fc3099b8-9f12-3e47-08a0-05abc37a0482@csgroup.eu>
Date:   Mon, 13 Dec 2021 17:50:52 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Steven Rostedt <rostedt@...dmis.org>
CC:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Petr Mladek <pmladek@...e.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        "Naveen N . Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "live-patching@...r.kernel.org" <live-patching@...r.kernel.org>,
        "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>
Subject: Re: [PATCH v1 0/5] Implement livepatch on PPC32



Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 17:30:48 +0000
> Christophe Leroy <christophe.leroy@...roup.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

Powered by blists - more mailing lists