Thread (18 messages) 18 messages, 5 authors, 2025-09-02

Re: [PATCH] tracing: Fix tracing_marker may trigger page fault during preempt_disable

From: Catalin Marinas <catalin.marinas@arm.com>
Date: 2025-08-29 19:53:45
Also in: linux-arm-kernel, lkml

On Fri, Aug 29, 2025 at 08:36:55AM -0400, Steven Rostedt wrote:
On Fri, 29 Aug 2025 08:26:04 -0400
Steven Rostedt [off-list ref] wrote:
quoted
BTW, the reason not to fault is because this might be called in code that is
already doing a fault and could cause deadlocks. The no sleeping part is a
side effect.
The difference between __copy_from_user_inatomic() and
copy_from_user_nofault() is the above. It is possible to fault in memory
without sleeping. For instance, the memory is already in the page cache,
but not the user space page tables. Where that would be OK for
__copy_from_user_inatomic() but not OK with copy_from_user_nofault(), due
to the mentioned locking.
The semantics of __copy_from_user_inatomic() are not entirely clear. The
name implies it is to be used in atomic contexts but the documentation
also says that the caller should ensure there's no fault (well, the
comment is further down in uaccess.h for __copy_to_user_inatomic()).

The generic implementation uses raw_copy_from_user() in both atomic and
non-atomic variants. The difference is pretty much a might_fault() call.
So it's nothing arm64 specific here.
For things like trace events and kprobes, copy_from_user_nofault() must be
used because they can be added to code that is doing a fault, and this version
must be used to prevent deadlocks.

But here, the __copy_from_user_inatomic() is in the code to handle writing
to the trace_marker file. It is directly called from a user space system
call, and will never be called within code that faults. Thus,
__copy_from_user_inatomic() *is* the correct operation, as there's no
problem if it needs to fault. It just can't sleep when doing so.
The problem is that it's the responsibility of the caller to ensure it
doesn't fault. In most cases, that's a pagefault_disable(). Or you just
go for copy_from_user_nofault() instead which actually checks that it's
a valid user address.

BTW, arm64 also bails out early in do_page_fault() if in_atomic() but I
suspect that's not the case here.

Adding Al Viro since since he wrote a large part of uaccess.h.

-- 
Catalin
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help