linux-kernel - Re: [GIT PULL] tracing: Fixes for v6.17

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250910160451.76a9b926@gandalf.local.home>
Date: Wed, 10 Sep 2025 16:04:51 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 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 Wed, 10 Sep 2025 12:19:44 -0700
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Tue, 9 Sept 2025 at 13:21, Steven Rostedt <rostedt@...dmis.org> wrote:
> >
> >         Back in 2016, the get_user_pages_fast() and
> >   the kmap() logic was replaced by a __copy_from_user_inatomic(). But the
> >   _inatomic() is somewhat a misnomer, as if the data being read faults, it can
> >   cause a schedule. This is not something you want to do in an atomic context.  
> 
> Somebody is very very confused, and this "explanation" is just wrong
> and entirely misleading.
> 
> __copy_from_user_inatomic() is very much atomic. But it is - as the
> dual underscores indicate - a *HELPER* function that needs the caller
> to do the appropriate coding around it.
> 
> 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.

It would have been nice if there was some comments by that function to let
one know that. This solution was suggested to me back then (2016) by a very
competent kernel developer but I was never told I needed to disable page
faults when using it.

> 
> You also need to actually verify that the user address is valid - as
> is typical with all the double-undercore user access functions.

Well, that is some tribal knowledge I didn't fully understand at the time.
Which is why I also suggested that we add comments to the code to state
that this call expects to have page faults disabled. That way others will
know why the function has double underscores.

The __copy_to_user_inatomic() has some mention of it, but it's not obvious
that those comments pertain to the _from_ variant.

> 
> >   Since the time this was added, copy_from_user_nofault() was added which is
> >   what is actually needed here. Replace the inatomic() with the nofault().  
> 
> I'm not disagreeing with the change, because that "nofault()" helper
> (without the double underscores) does do all the "appropriate coding
> around it".
> 
> And then it actually *uses* __copy_from_user_inatomic() to do the copy
> - because that function really is meant for atomic contents.
> 
> So that explanation really is very very wrong and entirely confused.
> 
> Because it was never about __copy_from_user_inatomic() not being
> appropriate from atomic context. It was about the tracing code
> apparently just using it wrong.

Agreed. But it was incorrectly used because there's no documentation on how
to use it properly.

Let me go and remedy that.

-- Steve