From: Masami Hiramatsu <mhiramat@kernel.org> To: Peter Zijlstra <peterz@infradead.org> Cc: Linus Torvalds <torvalds@linux-foundation.org>, kernel test robot <lkp@intel.com>, Steven Rostedt <rostedt@goodmis.org>, Shuah Khan <shuah@kernel.org>, Linux List Kernel Mailing <linux-kernel@vger.kernel.org>, Andy Lutomirski <luto@amacapital.net>, Ingo Molnar <mingo@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, Changbin Du <changbin.du@gmail.com>, Jann Horn <jannh@google.com>, Kees Cook <keescook@chromium.org>, Andy Lutomirski <luto@kernel.org>, Alexei Starovoitov <alexei.starovoitov@gmail.com>, Nadav Amit <namit@vmware.com>, Joel Fernandes <joel@joelfernandes.org>, yhs@fb.com, lkp@01.org Subject: Re: [uaccess] 780464aed0: WARNING:at_arch/x86/include/asm/uaccess.h:#strnlen_user/0x Date: Tue, 5 Mar 2019 22:58:01 +0900 [thread overview] Message-ID: <20190305225801.a63ac8712105ab2e673be1bc@kernel.org> (raw) In-Reply-To: <20190305090729.GF32477@hirez.programming.kicks-ass.net> On Tue, 5 Mar 2019 10:07:29 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, Mar 05, 2019 at 11:36:35AM +0900, Masami Hiramatsu wrote: > > I think the better way to do this is allowing strncpy_from_user() > O > > if some conditions are match, like > > > > - strncpy_from_user() will be able to copy user memory with set_fs(USER_DS) > > - strncpy_from_user() can copy kernel memory with set_fs(KERNEL_DS) > > - strncpy_from_user() can access unsafe memory in IRQ context if > > pagefault is disabled. > > > > This is almost done, except for CONFIG_DEBUG_ATOMIC_SLEEP=y on x86. > > > > So, what about adding a condition to WARN_ON_IN_IRQ() like below > > instead of introducing user_access_ok() ? > > > > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h > > index 780f2b42c8ef..ec0f0b74c9ab 100644 > > --- a/arch/x86/include/asm/uaccess.h > > +++ b/arch/x86/include/asm/uaccess.h > > @@ -70,7 +70,7 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un > > }) > > > > #ifdef CONFIG_DEBUG_ATOMIC_SLEEP > > -# define WARN_ON_IN_IRQ() WARN_ON_ONCE(!in_task()) > > +# define WARN_ON_IN_IRQ() WARN_ON_ONCE(pagefault_disabled() && !in_task()) > > That doesn't make any kind of sense to me; see faulthandler_disabled(). > IOW. interrupt (and any atomic context really) won't take faults anyway. Hmm, I thought CONFIG_DEBUG_ATOMIC_SLEEP=y tries to detect that some operations which can sleep in atomic, like IRQ context, doesn't it? (note that above should be !pagefault_disabled() anyway) So I guessed WARN_ON_IN_IRQ() intended to detect the access_ok() was used in atomic, because it might follow some copy_from_user() like operation which can sleep when it hits a pagefault. Is my guess wrong? If correct, I think if pagefault is disabled, the caller never sleep, so we don't need to take care of that. Could you tell me why WARN_ON_ONCE(!in_task()) is needed in access_ok()? > > I dislike that whole KERNEL_DS thing, but obviously that's not something > that's going away. > > Would something like: > > WARN_ON_ONCE(!(in_task || segment_eq(get_fs(), USER_DS))) > > Work? Then we allow KERNEL_DS in task context, but for interrupt and > others require USER_DS. But what would this mean? I can't understand why we limit using access_ok() so strictly and narrow the cases. Thank you, -- Masami Hiramatsu <mhiramat@kernel.org>
WARNING: multiple messages have this Message-ID (diff)
From: Masami Hiramatsu <mhiramat@kernel.org> To: lkp@lists.01.org Subject: Re: [uaccess] 780464aed0: WARNING:at_arch/x86/include/asm/uaccess.h:#strnlen_user/0x Date: Tue, 05 Mar 2019 22:58:01 +0900 [thread overview] Message-ID: <20190305225801.a63ac8712105ab2e673be1bc@kernel.org> (raw) In-Reply-To: <20190305090729.GF32477@hirez.programming.kicks-ass.net> [-- Attachment #1: Type: text/plain, Size: 2492 bytes --] On Tue, 5 Mar 2019 10:07:29 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, Mar 05, 2019 at 11:36:35AM +0900, Masami Hiramatsu wrote: > > I think the better way to do this is allowing strncpy_from_user() > O > > if some conditions are match, like > > > > - strncpy_from_user() will be able to copy user memory with set_fs(USER_DS) > > - strncpy_from_user() can copy kernel memory with set_fs(KERNEL_DS) > > - strncpy_from_user() can access unsafe memory in IRQ context if > > pagefault is disabled. > > > > This is almost done, except for CONFIG_DEBUG_ATOMIC_SLEEP=y on x86. > > > > So, what about adding a condition to WARN_ON_IN_IRQ() like below > > instead of introducing user_access_ok() ? > > > > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h > > index 780f2b42c8ef..ec0f0b74c9ab 100644 > > --- a/arch/x86/include/asm/uaccess.h > > +++ b/arch/x86/include/asm/uaccess.h > > @@ -70,7 +70,7 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un > > }) > > > > #ifdef CONFIG_DEBUG_ATOMIC_SLEEP > > -# define WARN_ON_IN_IRQ() WARN_ON_ONCE(!in_task()) > > +# define WARN_ON_IN_IRQ() WARN_ON_ONCE(pagefault_disabled() && !in_task()) > > That doesn't make any kind of sense to me; see faulthandler_disabled(). > IOW. interrupt (and any atomic context really) won't take faults anyway. Hmm, I thought CONFIG_DEBUG_ATOMIC_SLEEP=y tries to detect that some operations which can sleep in atomic, like IRQ context, doesn't it? (note that above should be !pagefault_disabled() anyway) So I guessed WARN_ON_IN_IRQ() intended to detect the access_ok() was used in atomic, because it might follow some copy_from_user() like operation which can sleep when it hits a pagefault. Is my guess wrong? If correct, I think if pagefault is disabled, the caller never sleep, so we don't need to take care of that. Could you tell me why WARN_ON_ONCE(!in_task()) is needed in access_ok()? > > I dislike that whole KERNEL_DS thing, but obviously that's not something > that's going away. > > Would something like: > > WARN_ON_ONCE(!(in_task || segment_eq(get_fs(), USER_DS))) > > Work? Then we allow KERNEL_DS in task context, but for interrupt and > others require USER_DS. But what would this mean? I can't understand why we limit using access_ok() so strictly and narrow the cases. Thank you, -- Masami Hiramatsu <mhiramat@kernel.org>
next prev parent reply other threads:[~2019-03-05 13:58 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-28 16:02 [PATCH v5 0/6] tracing/probes: uaccess: Add support user-space access Masami Hiramatsu 2019-02-28 16:02 ` [PATCH v5 1/6] uaccess: Add user_access_ok() Masami Hiramatsu 2019-02-28 16:03 ` [PATCH v5 2/6] uaccess: Use user_access_ok() in user_access_begin() Masami Hiramatsu 2019-03-03 17:39 ` [uaccess] 780464aed0: WARNING:at_arch/x86/include/asm/uaccess.h:#strnlen_user/0x kernel test robot 2019-03-03 17:39 ` kernel test robot 2019-03-03 19:53 ` Linus Torvalds 2019-03-03 19:53 ` Linus Torvalds 2019-03-04 1:14 ` Masami Hiramatsu 2019-03-04 1:14 ` Masami Hiramatsu 2019-03-04 2:37 ` Linus Torvalds 2019-03-04 2:37 ` Linus Torvalds 2019-03-04 9:06 ` Masami Hiramatsu 2019-03-04 9:06 ` Masami Hiramatsu 2019-03-04 15:16 ` Masami Hiramatsu 2019-03-04 15:16 ` Masami Hiramatsu 2019-03-04 15:58 ` Jann Horn 2019-03-04 15:58 ` Jann Horn 2019-03-04 18:59 ` Linus Torvalds 2019-03-04 18:59 ` Linus Torvalds 2019-03-05 2:36 ` Masami Hiramatsu 2019-03-05 2:36 ` Masami Hiramatsu 2019-03-05 8:22 ` Masami Hiramatsu 2019-03-05 8:22 ` Masami Hiramatsu 2019-03-05 9:01 ` Masami Hiramatsu 2019-03-05 9:01 ` Masami Hiramatsu 2019-03-05 9:07 ` Peter Zijlstra 2019-03-05 9:07 ` Peter Zijlstra 2019-03-05 13:58 ` Masami Hiramatsu [this message] 2019-03-05 13:58 ` Masami Hiramatsu 2019-03-05 14:53 ` Peter Zijlstra 2019-03-05 14:53 ` Peter Zijlstra 2019-03-05 15:18 ` Masami Hiramatsu 2019-03-05 15:18 ` Masami Hiramatsu 2019-03-04 3:20 ` [LKP] " Rong Chen 2019-03-04 3:20 ` Rong Chen 2019-02-28 16:03 ` [PATCH v5 3/6] uaccess: Add non-pagefault user-space read functions Masami Hiramatsu 2019-02-28 22:49 ` Yonghong Song 2019-03-01 2:29 ` Masami Hiramatsu 2019-03-01 6:30 ` Yonghong Song 2019-02-28 16:04 ` [PATCH v5 4/6] tracing/probe: Add ustring type for user-space string Masami Hiramatsu 2019-02-28 16:04 ` [PATCH v5 5/6] tracing/probe: Support user-space dereference Masami Hiramatsu 2019-02-28 16:05 ` [PATCH v5 6/6] selftests/ftrace: Add user-memory access syntax testcase Masami Hiramatsu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190305225801.a63ac8712105ab2e673be1bc@kernel.org \ --to=mhiramat@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=alexei.starovoitov@gmail.com \ --cc=changbin.du@gmail.com \ --cc=jannh@google.com \ --cc=joel@joelfernandes.org \ --cc=keescook@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lkp@01.org \ --cc=lkp@intel.com \ --cc=luto@amacapital.net \ --cc=luto@kernel.org \ --cc=mingo@kernel.org \ --cc=namit@vmware.com \ --cc=peterz@infradead.org \ --cc=rostedt@goodmis.org \ --cc=shuah@kernel.org \ --cc=torvalds@linux-foundation.org \ --cc=yhs@fb.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.