On Sat, Aug 30, 2025 at 11:22:51AM +0100, Catalin Marinas wrote:

On Fri, Aug 29, 2025 at 06:13:11PM -0400, Steven Rostedt wrote:

quoted

On Fri, 29 Aug 2025 20:53:40 +0100
Catalin Marinas [off-list ref] wrote:
valid user address.

quoted

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.

So, __copy_from_user_inatomic() is supposed to be called if
pagefault_disable() has already been called? If this is the case, can we
add more comments to this code? I've been using the inatomic() version this
way in preempt disabled locations since 2016.

This should work as long as in_atomic() returns true as it's checked in
the arm64 fault code. With PREEMPT_NONE, however, I don't think this
works.

Sorry, what exactly breaks for the PREEMPT_NONE case?

__copy_from_user_inatomic() could be changed to call
pagefault_disable() if !in_atomic() but you might as well call
copy_from_user_nofault() in the trace code directly as per Luo's patch.

That makes sense to me. I'll go check the arm64 users of
__copy_from_user_inatomic(), in case they're doing something dodgy.

quoted

I just wanted to figure out why __copy_from_user_inatomic() wasn't atomic.
If anything, it needs to be better documented.

Yeah, I had no idea until I looked at the code. I guess it means it can
be safely used if in_atomic() == true (well, making it up, not sure what
the intention was).

I think that was the intention -- it's the caller asserting that they
know the access won't fault (and hence won't sleep), and that's why
__copy_to_user_inatomic() and __copy_to_user() only differ by the latter
calling might_sleep().

It looks like other inconsistencies have crept in by accident. AFAICT
the should_fail_usercopy() check in __copy_from_user() was accidentally
missed from __copy_from_user_inatomic() when it was introduced in
commit:

  4d0e9df5e43dba52 ("lib, uaccess: add failure injection to usercopy functions")

... and the instrumentation is ordered inconsistently w.r.t.
should_fail_usercopy() since commit:

  33b75c1d884e81ec ("instrumented.h: allow instrumenting both sides of copy_from_user()")

... so there's a bunch of scope for cleanup, and we could probably have:

	/*
	 * TODO: comment saying to only call this directly when you know
	 * that the fault handler won't sleep.
	 */
	static __always_inline __must_check unsigned long
	__copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
	{
		...
	}

	static __always_inline __must_check unsigned long
	__copy_from_user(void *to, const void __user *from, unsigned long n)
	{
		might_fault();
		return __copy_from_user_inatomic();
	}

... to avoid the inconsistency.

Mark.