Re: [RFC PATCH v7 1/7] Restartable sequences system call

----- On Aug 10, 2016, at 4:10 AM, Andy Lutomirski luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org wrote:

On Tue, Aug 9, 2016 at 1:06 PM, Mathieu Desnoyers
[off-list ref] wrote:

<snip>

quoted

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...

quoted

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 ?

quoted

quoted

quoted

+SYSCALL_DEFINE2(rseq, struct rseq __user *, rseq, int, flags)
+{
+    if (unlikely(flags))
+            return -EINVAL;

(add whitespace)

fixed.

quoted

quoted

+    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