Re: [tip:x86/asm] x86/entry: Add enter_from_user_mode() and use it in syscalls

Andy Lutomirski
2015-07-14T23:05:11Z

On Tue, Jul 14, 2015 at 4:00 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Tue, Jul 07, 2015 at 03:51:29AM -0700, tip-bot for Andy Lutomirski wrote:
>> Commit-ID: feed36cde0a10adb957445a37e48f957f30b2273
>> Gitweb: http://git.kernel.org/tip/feed36cde0a10adb957445a37e48f957f30b2273
>> Author: Andy Lutomirski <luto@kernel.org>
>> AuthorDate: Fri, 3 Jul 2015 12:44:25 -0700
>> Committer: Ingo Molnar <mingo@kernel.org>
>> CommitDate: Tue, 7 Jul 2015 10:59:06 +0200
>>
>> x86/entry: Add enter_from_user_mode() and use it in syscalls
>>
>> Changing the x86 context tracking hooks is dangerous because
>> there are no good checks that we track our context correctly.
>> Add a helper to check that we're actually in CONTEXT_USER when
>> we enter from user mode and wire it up for syscall entries.
>>
>> Subsequent patches will wire this up for all non-NMI entries as
>> well. NMIs are their own special beast and cannot currently
>> switch overall context tracking state. Instead, they have their
>> own special RCU hooks.
>>
>> This is a tiny speedup if !CONFIG_CONTEXT_TRACKING (removes a
>> branch) and a tiny slowdown if CONFIG_CONTEXT_TRACING (adds a
>> layer of indirection). Eventually, we should fix up the core
>> context tracking code to supply a function that does what we
>> want (and can be much simpler than user_exit), which will enable
>> us to get rid of the extra call.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: Brian Gerst <brgerst@gmail.com>
>> Cc: Denys Vlasenko <dvlasenk@redhat.com>
>> Cc: Denys Vlasenko <vda.linux@googlemail.com>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: H. Peter Anvin <hpa@zytor.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Rik van Riel <riel@redhat.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: paulmck@linux.vnet.ibm.com
>> Link: http://lkml.kernel.org/r/853b42420066ec3fb856779cdc223a6dcb5d355b.1435952415.git.luto@kernel.org
>> Signed-off-by: Ingo Molnar <mingo@kernel.org>
>> ---
>> arch/x86/entry/common.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
>> index 917d0c3..9a327ee 100644
>> --- a/arch/x86/entry/common.c
>> +++ b/arch/x86/entry/common.c
>> @@ -28,6 +28,15 @@
>> #define CREATE_TRACE_POINTS
>> #include <trace/events/syscalls.h>
>>
>> +#ifdef CONFIG_CONTEXT_TRACKING
>> +/* Called on entry from user mode with IRQs off. */
>> +__visible void enter_from_user_mode(void)
>> +{
>> + CT_WARN_ON(ct_state() != CONTEXT_USER);
>> + user_exit();
>> +}
>> +#endif
>> +
>> static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
>> {
>> #ifdef CONFIG_X86_64
>> @@ -65,14 +74,16 @@ unsigned long syscall_trace_enter_phase1(struct pt_regs *regs, u32 arch)
>> work = ACCESS_ONCE(current_thread_info()->flags) &
>> _TIF_WORK_SYSCALL_ENTRY;
>>
>> +#ifdef CONFIG_CONTEXT_TRACKING
>> /*
>> * If TIF_NOHZ is set, we are required to call user_exit() before
>> * doing anything that could touch RCU.
>> */
>> if (work & _TIF_NOHZ) {
>> - user_exit();
>> + enter_from_user_mode();
>> work &= ~_TIF_NOHZ;
>
> We should move the sanity check to user_exit/enter() and use user_exit/enter()
> only when we actually enter/exit user.

I agree, but I don't know what other arches to.

> Here it's the case but syscall_trace_leave()
> and do_notify_resume() are special case that should probably use exception_enter/exit()
> unless your patchset have changed things such that there is only one call to user_exit()
> once we completed everything before resuming userspace. I need to review the rest of
> the patchset to discover that :-)

syscall_trace_leave and do_notify_resume may be so screwed up that
your suggestion wouldn't even work. However, the next set of patches
(out for review but currently stalled pending Brian Gerst's vm86 work)
remove those functions entirely.

--Andy