Messages in this thread | | | Date | Wed, 10 Aug 2016 19:04:51 +0000 (UTC) | From | Mathieu Desnoyers <> | Subject | Re: [RFC PATCH v7 1/7] Restartable sequences system call |
| |
----- On Aug 10, 2016, at 4:10 AM, Andy Lutomirski luto@amacapital.net wrote:
> On Tue, Aug 9, 2016 at 1:06 PM, Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote:
<snip>
>> Actually, we want copy_from_user() there. This executes upon >> resume to user-space, so we can take a page fault is needed, so >> no "inatomic" needed. I therefore suggest: > > Running the code below via exit_to_usermode_loop... > >> >> static bool rseq_get_rseq_cs(struct task_struct *t, >> void __user **start_ip, >> void __user **post_commit_ip, >> void __user **abort_ip) >> { >> unsigned long ptr; >> struct rseq_cs __user *urseq_cs; >> struct rseq_cs rseq_cs; >> >> if (__get_user(ptr, &t->rseq->rseq_cs)) >> return false; >> if (!ptr) >> return true; >> #ifdef CONFIG_COMPAT >> if (in_compat_syscall()) { >> urseq_cs = compat_ptr((compat_uptr_t)ptr); >> if (copy_from_user(&rseq_cs, urseq_cs, sizeof(*rseq_cs))) >> return false; >> *start_ip = compat_ptr((compat_uptr_t)rseq_cs.start_ip); >> *post_commit_ip = compat_ptr((compat_uptr_t)rseq_cs.post_commit_ip); >> *abort_ip = compat_ptr((compat_uptr_t)rseq_cs.abort_ip); >> return true; >> } >> #endif > > ...means that in_compat_syscall() is nonsense. (It *works* there, but > I can't imagine that it does anything that is actually sensible for > this use.)
Agreed that we are not per-se in a system call here. It works for in_ia32_syscall(), but it may not work for in_x32_syscall().
Then should we test for this ?
if (!is_64bit_mm(current->mm))
This is currently x86-specific. Is this how we are expected to test the user-space pointer size in the current mm in arch-agnostic code ? If so, we should implement is_64bit_mm() on all other architectures.
> > Can't you just define the ABI so that no compat junk is needed? > (Also, CRIU will thank you for doing that.)
We are dealing with user-space pointers here, so AFAIU we need to be aware of their size, which involves compat code. Am I missing something ?
> > >>>> +SYSCALL_DEFINE2(rseq, struct rseq __user *, rseq, int, flags) >>>> +{ >>>> + if (unlikely(flags)) >>>> + return -EINVAL; >>> >>> (add whitespace) >> >> fixed. >> >>> >>>> + if (!rseq) { >>>> + if (!current->rseq) >>>> + return -ENOENT; >>>> + return 0; >>>> + } > > This looks entirely wrong. Setting rseq to NULL fails if it's already > NULL but silently does nothing if rseq is already set? Surely it > should always succeed and it should actually do something if rseq is > set.
From the proposed rseq(2) manpage:
"A NULL rseq value can be used to check whether rseq is registered for the current thread."
The implementation does just that: it returns -1, errno=ENOENT if no rseq is currently registered, or 0 if rseq is currently registered.
Thanks,
Mathieu
> > > -- > Andy Lutomirski > AMA Capital Management, LLC
-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
| |