Re: [PATCH 2/4] MIPS: Add syscall detection for restartable sequences

From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: Paul Burton <paul.burton@mips.com>
Cc: linux-mips <linux-mips@linux-mips.org>,
	Peter Zijlstra <peterz@infradead.org>,
	James Hogan <jhogan@kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Ralf Baechle <ralf@linux-mips.org>
Subject: Re: [PATCH 2/4] MIPS: Add syscall detection for restartable sequences
Date: Sat, 16 Jun 2018 16:28:22 -0400 (EDT)	[thread overview]
Message-ID: <1395794863.16071.1529180902679.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <20180615184317.vogskh6rg53kfuzz@pburton-laptop>

----- On Jun 15, 2018, at 2:43 PM, Paul Burton paul.burton@mips.com wrote:

> Hi Mathieu,
> 
> On Fri, Jun 15, 2018 at 01:41:25PM -0400, Mathieu Desnoyers wrote:
>> > diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S
>> > index 38a302919e6b..d7de8adcfcc8 100644
>> > --- a/arch/mips/kernel/entry.S
>> > +++ b/arch/mips/kernel/entry.S
>> > @@ -79,6 +79,10 @@ FEXPORT(ret_from_fork)
>> > 	jal	schedule_tail		# a0 = struct task_struct *prev
>> > 
>> > FEXPORT(syscall_exit)
>> > +#ifdef CONFIG_DEBUG_RSEQ
>> > +	move	a0, sp
>> > +	jal	rseq_syscall
>> > +#endif
>> > 	local_irq_disable		# make sure need_resched and
>> > 					# signals dont change between
>> > 					# sampling and return
>> > @@ -141,6 +145,10 @@ work_notifysig:				# deal with pending signals and
>> > 	j	resume_userspace_check
>> > 
>> > FEXPORT(syscall_exit_partial)
>> > +#ifdef CONFIG_DEBUG_RSEQ
>> > +	move	a0, sp
>> > +	jal	rseq_syscall
>> > +#endif
>> > 	local_irq_disable		# make sure need_resched doesn't
>> > 					# change between and return
>> > 	LONG_L	a2, TI_FLAGS($28)	# current->work
>> 
>> Just to double-check: you did test with CONFIG_DEBUG_RSEQ=y, right ?
> 
> Yes, I did. Although I only ran the selftests, which I don't believe
> would actually trigger the SIGSEGV condition.

Yeah, I typically hand-craft a critical section that generate a
system call in order to trigger this. It's hackish however, and
only triggers the SIGSEGV on kernels with CONFIG_DEBUG_RSEQ=y.

> 
> Side-note: maybe it'd be useful to have a test that does intentionally
> perform a syscall within a restartable sequence & checks that it
> actually receives a SIGSEGV?.

We'd have to craft asm code for each architecture issuing a system
call in a rseq c.s. to test this. And we'd need to make this test
only runs on a kernel with CONFIG_DEBUG_RSEQ=y.

I'm not convinced yet it's worth the effort to cleanly integrate this
in selftests, but I'm very open to the idea.

> 
>> Are there any live registers that need to be saved before calling
>> rseq_syscall ?
> 
> No - we just need gp/$28 & sp/$29, and the calling convention means
> rseq_syscall() should return with those unmodified. Everything else that
> we or userland care about is about to be loaded from the stack anyway.

Sounds good!

Thanks,

Mathieu

> 
> Thanks,
>     Paul

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com