Re: [PATCH v1] Revert "tracing: Fix tracing_marker may trigger page fault during preempt_disable"
From: Runping Lai <hidden>
Date: 2025-10-07 18:19:47
Also in:
lkml
On Mon, Oct 6, 2025 at 7:08 PM Steven Rostedt [off-list ref] wrote:
On Tue, 7 Oct 2025 00:34:17 +0000 Runping Lai [off-list ref] wrote:quoted
This reverts commit 3d62ab32df065e4a7797204a918f6489ddb8a237. It's observed on Pixel 6 that this commit causes a severe functional regression: all user-space writes to trace_marker now fail. The write does not goes through at all. The error is observed in the shell as 'printf: write: Bad address'. This breaks a primary ftrace interface for user-space debugging and profiling. In kernel trace file, it's logged as 'tracing_mark_write: <faulted>'. After reverting this commit, functionality is restored.This is very interesting. The copy is being done in an atomic context. If the fault had to do anything other than update a page table, it is likely not to do anything and return a fault. What preemption model is Pixel 6 running in? CONFIG_PREEMPT_NONE?
Hey Steve, On Pixel6, CONFIG_PREEMPT is set. And CONFIG_PREEMPT_NONE is not set. I'll paste the full PREEMPT configs: ~/aosp_kernel > common/scripts/extract-ikconfig out/slider/dist/vmlinux | less | grep PREEMPT CONFIG_PREEMPT_BUILD=y CONFIG_ARCH_HAS_PREEMPT_LAZY=y # CONFIG_PREEMPT_NONE is not set # CONFIG_PREEMPT_VOLUNTARY is not set CONFIG_PREEMPT=y # CONFIG_PREEMPT_LAZY is not set # CONFIG_PREEMPT_RT is not set CONFIG_PREEMPT_COUNT=y CONFIG_PREEMPTION=y # CONFIG_PREEMPT_DYNAMIC is not set CONFIG_PREEMPT_RCU=y CONFIG_HAVE_PREEMPT_DYNAMIC=y CONFIG_HAVE_PREEMPT_DYNAMIC_KEY=y CONFIG_PREEMPT_NOTIFIERS=y # CONFIG_DEBUG_PREEMPT is not set CONFIG_PREEMPTIRQ_TRACEPOINTS=y # CONFIG_PREEMPT_TRACER is not set # CONFIG_PREEMPTIRQ_DELAY_TEST is not set
The original code is buggy, but if this is causing a regression, then we likely need to do something else, like copy in a pre-allocated buffer?
Sounds like a good plan. Before the long term fix, can we please revert this commit? Best, Runping
-- Stevequoted
Signed-off-by: Runping Lai <redacted> Reported-by: Wattson CI <redacted> --- kernel/trace/trace.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 156e7e0bf559..bb9a6284a629 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c@@ -7213,7 +7213,7 @@ static ssize_t write_marker_to_buffer(struct trace_array *tr, const char __user entry = ring_buffer_event_data(event); entry->ip = ip; - len = copy_from_user_nofault(&entry->buf, ubuf, cnt); + len = __copy_from_user_inatomic(&entry->buf, ubuf, cnt); if (len) { memcpy(&entry->buf, FAULTED_STR, FAULTED_SIZE); cnt = FAULTED_SIZE;@@ -7310,7 +7310,7 @@ static ssize_t write_raw_marker_to_buffer(struct trace_array *tr, entry = ring_buffer_event_data(event); - len = copy_from_user_nofault(&entry->id, ubuf, cnt); + len = __copy_from_user_inatomic(&entry->id, ubuf, cnt); if (len) { entry->id = -1; memcpy(&entry->buf, FAULTED_STR, FAULTED_SIZE);