Message-ID: <77ad4c78-d579-4219-a8aa-a8d4acb8a9c7@efficios.com>
Date: Wed, 10 Sep 2025 15:49:24 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>,
 Steven Rostedt <rostedt@...dmis.org>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
 Masami Hiramatsu <mhiramat@...nel.org>, Guenter Roeck <linux@...ck-us.net>,
 Luo Gengkun <luogengkun@...weicloud.com>, Pu Lehui <pulehui@...wei.com>,
 Qianfeng Rong <rongqianfeng@...o.com>,
 Vladimir Riabchun <ferr.lambarginio@...il.com>,
 Wang Liang <wangliang74@...wei.com>
Subject: Re: [GIT PULL] tracing: Fixes for v6.17

On 2025-09-10 15:37, Linus Torvalds wrote:
> On Wed, 10 Sept 2025 at 12:19, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
>>
>> In this case, the appropriate coding is to typically surround it with
>> a pagefault_{disable,enable}() pattern to let the page faulting code
>> know to not actually do the fault.
> 
> Btw, I say "typically", because you don't have to do that. The page
> fault code uses
> 
>          if (faulthandler_disabled() ..)
> 
> to decide if it should handle the fault or not, and that checks not
> just if page faults are explicitly disabled, but also checks -
> surprise surprise - "in_atomic()".
> 
> So just being in an explicitly atomic context automatically means that
> __copy_from_user_inatomic() is atomic.
> 
> Which makes me wonder if there is something entirely wrong.
> 
> Because the explanation for this in commit 3d62ab32df06 ("tracing: Fix
> tracing_marker may trigger page fault during preempt_disable") talks
> about the task being preempted in between the
> 
>    ring_buffer_lock_reserve
>    ring_buffer_unlock_commit
> 
> and it sounds like maybe the tracing code isn't disabling preemption
> for the whole sequence?
> 
> Because "in_atomic()" does check the preempt count, and so just being
> non-preemptible should already have disabled page faults.
> 
> Maybe the page fault just ends up being expensive enough that it
> exposes preemption being more *likely* just because the window now is
> much wider.
> 
> Alternatively, this is perhaps an arm64-specific bug where the page
> fault disabling doesn't honor the preemption disable of
> faulthandler_disabled()?
> 
> I did *not* go through the whole arm64 page faulting code: that commit
> talks about do_mem_abort() which is done as part of the common arm64
> fault handling, and if that then doesn't honor
> faulthandler_disabled(), then honestly, that perf fix isn't actually
> fixing anything either. It would still do the same reschedule even
> with an explicit "pagefault_disable()/enable()" if
> faulthandler_disabled() simply isn't honored properly.
> 
> Adding Catalin and Will to the participants to see if they have input
> on that whole do_mem_abort() angle.

See include/linux/uaccess.h:

/*
  * The pagefault handler is in general disabled by pagefault_disable() or
  * when in irq context (via in_atomic()).
  *
  * This function should only be used by the fault handlers. Other users should
  * stick to pagefault_disabled().
  * Please NEVER use preempt_disable() to disable the fault handler. With
  * !CONFIG_PREEMPT_COUNT, this is like a NOP. So the handler won't be disabled.
  * in_atomic() will report different values based on !CONFIG_PREEMPT_COUNT.
  */
#define faulthandler_disabled() (pagefault_disabled() || in_atomic())

Especially the part where in_atomic() is not so useful with
!CONFIG_PREEMPT_COUNT. AFAIU, this is why we rely on explicit
pagefault disable/enable in tracing code.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Powered by blists - more mailing lists