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: <660888761.4077.1435345446678.JavaMail.zimbra@efficios.com>
Date:	Fri, 26 Jun 2015 19:04:06 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Paul Turner <pjt@...gle.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Andrew Hunter <ahh@...gle.com>,
	Andi Kleen <andi@...stfloor.org>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	linux-api <linux-api@...r.kernel.org>,
	linux-kernel@...r.kernel.org, rostedt <rostedt@...dmis.org>,
	Josh Triplett <josh@...htriplett.org>,
	Ingo Molnar <mingo@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Chris Lameter <cl@...ux.com>
Subject: Re: [RFC PATCH 2/3] restartable sequences: x86 ABI

----- On Jun 26, 2015, at 2:09 PM, Mathieu Desnoyers mathieu.desnoyers@...icios.com wrote:

> ----- On Jun 24, 2015, at 6:26 PM, Paul Turner pjt@...gle.com wrote:
> 
>> Implements the x86 (i386 & x86-64) ABIs for interrupting and restarting
>> execution within restartable sequence sections.
>> 
>> With respect to the x86-specific ABI:
>>  On 32-bit:           Upon restart, the interrupted rip is placed in %ecx
>>  On 64-bit (or x32):  Upon restart, the interrupted rip is placed in %r10
>> 
>> While potentially surprising at first glance, this choice is strongly motivated
>> by the fact that the available scratch registers under the i386 function call
>> ABI overlap with those used as argument registers under x86_64.
>> 
>> Given that sequences are already personality specific and that we always want
>> the arguments to be available for sequence restart, it's much more natural to
>> ultimately differentiate the ABI in these two cases.
>> 
>> Signed-off-by: Paul Turner <pjt@...gle.com>
>> ---
>> arch/x86/include/asm/restartable_sequences.h |   50 +++++++++++++++++++
>> arch/x86/kernel/Makefile                     |    2 +
>> arch/x86/kernel/restartable_sequences.c      |   69 ++++++++++++++++++++++++++
>> arch/x86/kernel/signal.c                     |   12 +++++
>> kernel/restartable_sequences.c               |   11 +++-
>> 5 files changed, 141 insertions(+), 3 deletions(-)
>> create mode 100644 arch/x86/include/asm/restartable_sequences.h
>> create mode 100644 arch/x86/kernel/restartable_sequences.c
>> 
>> diff --git a/arch/x86/include/asm/restartable_sequences.h
>> b/arch/x86/include/asm/restartable_sequences.h
>> new file mode 100644
>> index 0000000..0ceb024
>> --- /dev/null
>> +++ b/arch/x86/include/asm/restartable_sequences.h
>> @@ -0,0 +1,50 @@
>> +#ifndef _ASM_X86_RESTARTABLE_SEQUENCES_H
>> +#define _ASM_X86_RESTARTABLE_SEQUENCES_H
>> +
>> +#include <asm/processor.h>
>> +#include <asm/ptrace.h>
>> +#include <linux/sched.h>
>> +
>> +#ifdef CONFIG_RESTARTABLE_SEQUENCES
>> +
>> +static inline bool arch_rseq_in_crit_section(struct task_struct *p,
>> +					     struct pt_regs *regs)
>> +{
>> +	struct task_struct *leader = p->group_leader;
>> +	struct restartable_sequence_state *rseq_state = &leader->rseq_state;
>> +
>> +	unsigned long ip = (unsigned long)regs->ip;
>> +	if (unlikely(ip < (unsigned long)rseq_state->crit_end &&
>> +		     ip >= (unsigned long)rseq_state->crit_start))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +static inline bool arch_rseq_needs_notify_resume(struct task_struct *p)
>> +{
>> +#ifdef CONFIG_PREEMPT
>> +	/*
>> +	 * Under CONFIG_PREEMPT it's possible for regs to be incoherent in the
>> +	 * case that we took an interrupt during syscall entry.  Avoid this by
>> +	 * always deferring to our notify-resume handler.
>> +	 */
>> +	return true;
> 
> I'm a bit puzzled about this. If I look at perf_get_regs_user() in the perf
> code, task_pt_regs() seems to return the user-space pt_regs for a task with
> a current->mm set (iow, not a kernel thread), even if an interrupt nests on
> top of a system call. The only corner-case is NMIs, where an NMI may interrupt
> in the middle of setting up the task pt_regs, but scheduling should never happen
> there, right ?
> 
> Since it's impossible for kernel threads to have a rseq critical section,
> we should be able to assume that every time task_pt_regs() returns a
> non-userspace (user_mode(regs) != 0) pt_regs implies that scheduling applies
> to a kernel thread. Therefore, following this line of thoughts,
> arch_rseq_in_crit_section() should work for CONFIG_PREEMPT kernels too.
> 
> So what I am missing here ?

AFAIU, the comment near this check in perf_get_regs_user() is bogus.
It does not only apply to NMIs, but also applies to normal interrupt
handlers that nest over the stack setup on syscall entry (below
entry_SYSCALL_64_after_swapgs in entry_64.S):

        struct pt_regs *user_regs = task_pt_regs(current);

        /*
         * If we're in an NMI that interrupted task_pt_regs setup, then
         * we can't sample user regs at all.  This check isn't really
         * sufficient, though, as we could be in an NMI inside an interrupt
         * that happened during task_pt_regs setup.
         */
        if (regs->sp > (unsigned long)&user_regs->r11 &&
            regs->sp <= (unsigned long)(user_regs + 1)) {
                regs_user->abi = PERF_SAMPLE_REGS_ABI_NONE;
                regs_user->regs = NULL;
                return;
        }

That would be how, for tracing, those races can be avoided. It
might not be a huge issue for perf samples to lose one sample once
in a while, but I understand that this statistical approach would
be incorrect in the context of RSEQ.

Moving ENABLE_INTERRUPTS(CLBR_NONE) 3 instructions down, just after
pushq   %rcx                            /* pt_regs->ip */
might solve your issue here. (in entry_SYSCALL_64_after_swapgs)

Thoughts ?

Thanks,

Mathieu


> 
> Thanks,
> 
> Mathieu
> 
>> +#else
>> +	return arch_rseq_in_crit_section(p, task_pt_regs(p));
>> +#endif
>> +}
>> +
>> +void arch_rseq_handle_notify_resume(struct pt_regs *regs);
>> +void arch_rseq_check_critical_section(struct task_struct *p,
>> +				      struct pt_regs *regs);
>> +
>> +#else /* !CONFIG_RESTARTABLE_SEQUENCES */
>> +
>> +static inline void arch_rseq_handle_notify_resume(struct pt_regs *regs) {}
>> +static inline void arch_rseq_check_critical_section(struct task_struct *p,
>> +						    struct pt_regs *regs) {}
>> +
>> +#endif
>> +
>> +#endif /* _ASM_X86_RESTARTABLE_SEQUENCES_H */
>> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>> index febaf18..bd7827d 100644
>> --- a/arch/x86/kernel/Makefile
>> +++ b/arch/x86/kernel/Makefile
>> @@ -113,6 +113,8 @@ obj-$(CONFIG_TRACING)			+= tracepoint.o
>> obj-$(CONFIG_IOSF_MBI)			+= iosf_mbi.o
>> obj-$(CONFIG_PMC_ATOM)			+= pmc_atom.o
>> 
>> +obj-$(CONFIG_RESTARTABLE_SEQUENCES)	+= restartable_sequences.o
>> +
>> ###
>> # 64 bit specific files
>> ifeq ($(CONFIG_X86_64),y)
>> diff --git a/arch/x86/kernel/restartable_sequences.c
>> b/arch/x86/kernel/restartable_sequences.c
>> new file mode 100644
>> index 0000000..3b38013
>> --- /dev/null
>> +++ b/arch/x86/kernel/restartable_sequences.c
>> @@ -0,0 +1,69 @@
>> +/*
>> + * Restartable Sequences: x86 ABI.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
>> + *
>> + * Copyright (C) 2015, Google, Inc.,
>> + * Paul Turner <pjt@...gle.com> and Andrew Hunter <ahh@...gle.com>
>> + *
>> + */
>> +
>> +#include <linux/sched.h>
>> +#include <linux/uaccess.h>
>> +#include <asm/restartable_sequences.h>
>> +
>> +void arch_rseq_check_critical_section(struct task_struct *p,
>> +				      struct pt_regs *regs)
>> +{
>> +	if (!arch_rseq_in_crit_section(p, regs))
>> +		return;
>> +
>> +	/* RSEQ only applies to user-mode execution */
>> +	BUG_ON(!user_mode(regs));
>> +
>> +	/*
>> +	 * The ABI is slightly different for {32,64}-bit threads on x86
>> +	 *
>> +	 * Short version:
>> +	 *   x86-64 (or x32): interrupted rip => %r10
>> +	 *   i386:            interrupted rip => %ecx
>> +	 *
>> +	 * Longer version:
>> +	 * The scratch registers available under the i386 function call ABI
>> +	 * overlap with those used by argument registers under the x86_64 ABI.
>> +	 *
>> +	 * Given that the sequence block is already personality specific in
>> +	 * that it must be entered by 'call' and that we always want the
>> +	 * arguments available for a sequence restart; it's more natural to
>> +	 * differentiate the ABI in these two cases.
>> +	 */
>> +	if (unlikely(test_tsk_thread_flag(p, TIF_IA32)))
>> +		regs->cx = regs->ip; /* i386 */
>> +	else
>> +		regs->r10 = regs->ip; /* x86-64/x32 */
>> +
>> +	regs->ip = (unsigned long)p->group_leader->rseq_state.crit_restart;
>> +}
>> +
>> +void arch_rseq_handle_notify_resume(struct pt_regs *regs)
>> +{
>> +	struct restartable_sequence_state *rseq_state = &current->rseq_state;
>> +
>> +	/* If this update fails our user-state is incoherent. */
>> +	if (put_user(task_cpu(current), rseq_state->cpu_pointer))
>> +		force_sig(SIGSEGV, current);
>> +
>> +	arch_rseq_check_critical_section(current, regs);
>> +}
>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>> index 206996c..987c50b 100644
>> --- a/arch/x86/kernel/signal.c
>> +++ b/arch/x86/kernel/signal.c
>> @@ -31,6 +31,7 @@
>> #include <asm/vdso.h>
>> #include <asm/mce.h>
>> #include <asm/sighandling.h>
>> +#include <asm/restartable_sequences.h>
>> 
>> #ifdef CONFIG_X86_64
>> #include <asm/proto.h>
>> @@ -617,6 +618,15 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
>> 	sigset_t *set = sigmask_to_save();
>> 	compat_sigset_t *cset = (compat_sigset_t *) set;
>> 
>> +	/*
>> +	 * If we are executing in the critical section of a restartable
>> +	 * sequence we need to fix up the user's stack saved ip at this point
>> +	 * so that signal handler return does not allow us to jump back into
>> +	 * the block across a context switch boundary.
>> +	 */
>> +	if (rseq_active(current))
>> +		arch_rseq_check_critical_section(current, regs);
>> +
>> 	/* Set up the stack frame */
>> 	if (is_ia32_frame()) {
>> 		if (ksig->ka.sa.sa_flags & SA_SIGINFO)
>> @@ -755,6 +765,8 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32
>> thread_info_flags)
>> 	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
>> 		clear_thread_flag(TIF_NOTIFY_RESUME);
>> 		tracehook_notify_resume(regs);
>> +		if (rseq_active(current))
>> +			arch_rseq_handle_notify_resume(regs);
>> 	}
>> 	if (thread_info_flags & _TIF_USER_RETURN_NOTIFY)
>> 		fire_user_return_notifiers();
>> diff --git a/kernel/restartable_sequences.c b/kernel/restartable_sequences.c
>> index 72945f2..9102241 100644
>> --- a/kernel/restartable_sequences.c
>> +++ b/kernel/restartable_sequences.c
>> @@ -24,17 +24,22 @@
>> 
>> #ifdef CONFIG_RESTARTABLE_SEQUENCES
>> 
>> +#include <asm/restartable_sequences.h>
>> #include <linux/uaccess.h>
>> #include <linux/preempt.h>
>> #include <linux/syscalls.h>
>> 
>> static void rseq_sched_in_nop(struct preempt_notifier *pn, int cpu) {}
>> -static void rseq_sched_out_nop(struct preempt_notifier *pn,
>> -			       struct task_struct *next) {}
>> +static void rseq_sched_out(struct preempt_notifier *pn,
>> +			   struct task_struct *next)
>> +{
>> +	if (arch_rseq_needs_notify_resume(current))
>> +		set_thread_flag(TIF_NOTIFY_RESUME);
>> +}
>> 
>> static __read_mostly struct preempt_ops rseq_preempt_ops = {
>> 	.sched_in = rseq_sched_in_nop,
>> -	.sched_out = rseq_sched_out_nop,
>> +	.sched_out = rseq_sched_out,
>> };
>> 
>>  int rseq_register_cpu_pointer_current(int __user *cpu_pointer)
> 
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ