Date:   Wed, 27 Feb 2019 16:41:04 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org,
        Andy Lutomirski <luto@...capital.net>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Changbin Du <changbin.du@...il.com>,
        Jann Horn <jannh@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Nadav Amit <namit@...are.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC PATCH 0/4] tracing/probes: uaccess: Add support user-space
 access

On Tue, 26 Feb 2019 16:38:50 -0500
Joel Fernandes <joel@...lfernandes.org> wrote:

> On Mon, Feb 25, 2019 at 11:04:42PM +0900, Masami Hiramatsu wrote:

> > Note that kprobe event provides these methods, but it doesn't
> > change it from kernel to user automatically because we do not
> > know whether the given address is in userspace or kernel on
> > some arch.
> > Moreover, from perf-probe, at this moment it is not able to
> > switch. Since __user is not for compiler but checker, we have
> > no clue which data structure is in user-space, in debuginfo.
> > 
> > BTW, according to Linus's comment, I implemented probe_user_read()
> > and strncpy_from_unsafe_user() APIs. And since those use
> > "access_ok()" inside it, if CONFIG_DEBUG_ATOMIC_SLEEP=y on x86,
> > it will get a warn message at once. It should be solved before
> > merging this series.
> 
> I was wondering why access_ok() can sleep. In the arm64 and x86
> implementation, I don't see access_ok() itself causing a user pointer
> dereference access that can cause a page fault. It seems to just be checking
> the validity of the ranges.
> 
> Any idea why the access_ok() code has these comments?
> "Context: User context only. This function may sleep if pagefaults are
> enabled."

Because access_ok() is used only for preparing accessing user-space,
and the user-space access may cause page-fault and sleep.

IMHO, checking in access_ok() inside it is reasonable, but as it
commented, it is for "if pagefaults are enabled.". What we need another 
access_ok() for the case when pagefaults are disabled, that is what PeterZ
suggested in below mail.

https://lore.kernel.org/lkml/20190225150603.GE32494@hirez.programming.kicks-ass.net/T/#u


Thank you,

> 
> My _guess_ is this is because whatever calls access_ok() may also call
> something else that *does* fault next, if that's the case then that
> WARN_ON_IN_IRQ() in access_ok() is fine, but at least I guess the comments
> should be more clear that it is not access_ok() itself that sleeps.
> 
> thanks for any help on understanding this,
> 
>  - Joel
> 
> 
> > 
> > Thank you,
> > 
> > ---
> > 
> > Masami Hiramatsu (4):
> >       uaccess: Make sure kernel_uaccess_faults_ok is updated before pagefault
> >       uaccess: Add non-pagefault user-space read functions
> >       tracing/probe: Add ustring type for user-space string
> >       tracing/probe: Support user-space dereference
> > 
> > 
> >  Documentation/trace/kprobetrace.rst  |   13 ++-
> >  Documentation/trace/uprobetracer.rst |    9 +-
> >  fs/namespace.c                       |    2 
> >  include/linux/uaccess.h              |   13 +++
> >  kernel/trace/trace.c                 |    7 +-
> >  kernel/trace/trace_kprobe.c          |   65 ++++++++++++++++
> >  kernel/trace/trace_probe.c           |   39 ++++++++--
> >  kernel/trace/trace_probe.h           |    3 +
> >  kernel/trace/trace_probe_tmpl.h      |   36 +++++++--
> >  kernel/trace/trace_uprobe.c          |   19 +++++
> >  mm/maccess.c                         |  138 ++++++++++++++++++++++++++++++----
> >  11 files changed, 302 insertions(+), 42 deletions(-)
> > 
> > --
> > Masami Hiramatsu (Linaro) <mhiramat@...nel.org>


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists