diff mbox series

[v2,01/33] ftrace: Unpoison ftrace_regs in ftrace_ops_list_func()

Message ID 20231121220155.1217090-2-iii@linux.ibm.com
State New
Headers
Series kmsan: Enable on s390 |

Commit Message

Ilya Leoshkevich Nov. 21, 2023, 10 p.m. UTC 
  Architectures use assembly code to initialize ftrace_regs and call
ftrace_ops_list_func(). Therefore, from the KMSAN's point of view,
ftrace_regs is poisoned on ftrace_ops_list_func entry(). This causes
KMSAN warnings when running the ftrace testsuite.

Fix by trusting the architecture-specific assembly code and always
unpoisoning ftrace_regs in ftrace_ops_list_func.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 kernel/trace/ftrace.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Steven Rostedt Nov. 22, 2023, 11:32 p.m. UTC | #1 
On Tue, 21 Nov 2023 23:00:55 +0100
Ilya Leoshkevich <iii@linux.ibm.com> wrote:

> Architectures use assembly code to initialize ftrace_regs and call
> ftrace_ops_list_func(). Therefore, from the KMSAN's point of view,
> ftrace_regs is poisoned on ftrace_ops_list_func entry(). This causes
> KMSAN warnings when running the ftrace testsuite.
> 
> Fix by trusting the architecture-specific assembly code and always
> unpoisoning ftrace_regs in ftrace_ops_list_func.

You must be very trusting to trust architecture-specific assembly code ;-)

Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve


> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  kernel/trace/ftrace.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 8de8bec5f366..dfb8b26966aa 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -7399,6 +7399,7 @@ __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
>  void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
>  			       struct ftrace_ops *op, struct ftrace_regs *fregs)
>  {
> +	kmsan_unpoison_memory(fregs, sizeof(*fregs));
>  	__ftrace_ops_list_func(ip, parent_ip, NULL, fregs);
>  }
>  #else
Alexander Potapenko Dec. 8, 2023, 2:16 p.m. UTC | #2 
On Tue, Nov 21, 2023 at 11:02 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Architectures use assembly code to initialize ftrace_regs and call
> ftrace_ops_list_func(). Therefore, from the KMSAN's point of view,
> ftrace_regs is poisoned on ftrace_ops_list_func entry(). This causes
> KMSAN warnings when running the ftrace testsuite.

I couldn't reproduce these warnings on x86, hope you really need this
change on s390 :)

> Fix by trusting the architecture-specific assembly code and always
> unpoisoning ftrace_regs in ftrace_ops_list_func.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Alexander Potapenko <glider@google.com>
Steven Rostedt Dec. 8, 2023, 2:31 p.m. UTC | #3 
On Fri, 8 Dec 2023 15:16:10 +0100
Alexander Potapenko <glider@google.com> wrote:

> On Tue, Nov 21, 2023 at 11:02 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >
> > Architectures use assembly code to initialize ftrace_regs and call
> > ftrace_ops_list_func(). Therefore, from the KMSAN's point of view,
> > ftrace_regs is poisoned on ftrace_ops_list_func entry(). This causes
> > KMSAN warnings when running the ftrace testsuite.  
> 
> I couldn't reproduce these warnings on x86, hope you really need this
> change on s390 :)

On x86, ftrace_regs sits on the stack. And IIUC, s390 doesn't have the same
concept of a "stack" as other architectures. Perhaps that's the reason s390
needs this?

-- Steve
Ilya Leoshkevich Dec. 12, 2023, 2:05 a.m. UTC | #4 
On Fri, 2023-12-08 at 09:31 -0500, Steven Rostedt wrote:
> On Fri, 8 Dec 2023 15:16:10 +0100
> Alexander Potapenko <glider@google.com> wrote:
> 
> > On Tue, Nov 21, 2023 at 11:02 PM Ilya Leoshkevich
> > <iii@linux.ibm.com> wrote:
> > > 
> > > Architectures use assembly code to initialize ftrace_regs and
> > > call
> > > ftrace_ops_list_func(). Therefore, from the KMSAN's point of
> > > view,
> > > ftrace_regs is poisoned on ftrace_ops_list_func entry(). This
> > > causes
> > > KMSAN warnings when running the ftrace testsuite.  
> > 
> > I couldn't reproduce these warnings on x86, hope you really need
> > this
> > change on s390 :)

I just double-checked, and it's still needed. Without it, I get:

[    4.140184] =====================================================  
[    4.140416] BUG: KMSAN: uninit-value in
arch_ftrace_ops_list_func+0x8e6/0x14b0            
[    4.140484]  arch_ftrace_ops_list_func+0x8e6/0x14b0                
[    4.140546]  ftrace_graph_caller+0x0/0x34                          
[    4.140614]  read_tod_clock+0x6/0x1e0                              
[    4.140671]  ktime_get+0x3a4/0x670                                 
[    4.140727]  clockevents_program_event+0x1c8/0xb10                 
[    4.140785]  tick_program_event+0x11e/0x230                        
[    4.140842]  hrtimer_interrupt+0x118a/0x1d10                       
[    4.140898]  do_IRQ+0x108/0x150                                    
[    4.140959]  do_irq_async+0xfc/0x270                               
[    4.141021]  do_ext_irq+0x98/0x120                                 
[    4.141080]  ext_int_handler+0xc4/0xf0                             
[    4.141141]  _raw_spin_unlock_irqrestore+0xfa/0x190                
[    4.141207]  _raw_spin_unlock_irqrestore+0xf6/0x190                
[    4.141271]  s390_kernel_write+0x218/0x250                         
[    4.141328]  ftrace_make_call+0x362/0x4a0                          
[    4.141386]  __ftrace_replace_code+0xb44/0xbd0                     
[    4.141442]  ftrace_replace_code+0x1d8/0x440                       
[    4.141497]  ftrace_modify_all_code+0xfe/0x510                     
[    4.141555]  ftrace_startup+0x4f0/0xcf0                            
[    4.141609]  register_ftrace_function+0x1316/0x1440                
[    4.141670]  function_trace_init+0x2c0/0x3d0                       
[    4.141732]  tracer_init+0x282/0x370                               
[    4.141789]  trace_selftest_startup_function+0x104/0x19d0          
[    4.141857]  run_tracer_selftest+0x7c8/0xab0                       
[    4.141918]  init_trace_selftests+0x200/0x820
[    4.141977]  do_one_initcall+0x35e/0x1090
[    4.142032]  do_initcall_level+0x276/0x660
[    4.142095]  do_initcalls+0x16a/0x2d0
[    4.142153]  kernel_init_freeable+0x632/0x960
[    4.142216]  kernel_init+0x36/0x1810
[    4.142277]  __ret_from_fork+0xc0/0x180
[    4.142333]  ret_from_fork+0xa/0x30
[    4.142431] Local variable agg.tmp.i.i created at:                 
02:06:55 [30/1836]
[    4.142476]  timekeeping_advance+0x79a/0x2870
[    4.142394] 
[    4.142431] Local variable agg.tmp.i.i created at:
[    4.142476]  timekeeping_advance+0x79a/0x2870
[    4.142534] 
[    4.142573] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W      
6.7.0-rc4-g7657d31dc545 #4
[    4.142638] Hardware name: IBM 3931 A01 704 (KVM/Linux)
[    4.142686] =====================================================
[    4.142734] Kernel panic - not syncing: kmsan.panic set ...
[    4.142734] =====================================================

> On x86, ftrace_regs sits on the stack. And IIUC, s390 doesn't have
> the same
> concept of a "stack" as other architectures. Perhaps that's the
> reason s390
> needs this?

It's not that different on s390x. There is indeed no architecture-
mandated stack pointer and no push/pop, but other than that it's fairly
normal. Linux uses %r15 as a stack pointer.

On s390x ftrace_regs is allocated on stack by mcount.S. From what I can
see, Intel's ftrace_64.S does the same thing, so I don't immediately
see why uninit-value is not detected on Intel, even though I think it
should.

> -- Steve
diff mbox series

Patch

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 8de8bec5f366..dfb8b26966aa 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -7399,6 +7399,7 @@  __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
 void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
 			       struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
+	kmsan_unpoison_memory(fregs, sizeof(*fregs));
 	__ftrace_ops_list_func(ip, parent_ip, NULL, fregs);
 }
 #else