Re: [PATCH v3 01/34] ftrace: Unpoison ftrace_regs in ftrace_ops_list_func()
From: Steven Rostedt <rostedt@goodmis.org>
Date: 2024-01-02 15:16:15
Also in:
linux-mm, linux-s390, lkml
On Thu, 14 Dec 2023 00:24:21 +0100 Ilya Leoshkevich [off-list ref] 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.
BTW, why is this only a problem for s390 and no other architectures?
If it is only a s390 thing, then we should do this instead:
in include/linux/ftrace.h:
/* Add a comment here to why this is needed */
#ifndef ftrace_list_func_unpoison
# define ftrace_list_func_unpoison(fregs) do { } while(0)
#endif
In arch/s390/include/asm/ftrace.h:
/* Add a comment to why s390 is special */
# define ftrace_list_func_unpoison(fregs) kmsan_unpoison_memory(fregs, sizeof(*fregs))
Fix by trusting the architecture-specific assembly code and always unpoisoning ftrace_regs in ftrace_ops_list_func. Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
I'm taking my ack away for this change in favor of what I'm suggesting now.
quoted hunk ↗ jump to hunk
Reviewed-by: Alexander Potapenko <glider@google.com> 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));
And here have: ftrace_list_func_unpoison(fregs); That way we only do it for archs that really need it, and do not affect archs that do not. I want to know why this only affects s390, because if we are just doing this because "it works", it could be just covering up a symptom of something else and not actually doing the "right thing". -- Steve
__ftrace_ops_list_func(ip, parent_ip, NULL, fregs); } #else