[lkml]   [2016]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [RFC PATCH v7 1/7] Restartable sequences system call
----- On Aug 10, 2016, at 3:16 PM, Andy Lutomirski wrote:

> On Wed, Aug 10, 2016 at 12:04 PM, Mathieu Desnoyers
> <> wrote:
>> ----- On Aug 10, 2016, at 4:10 AM, Andy Lutomirski wrote:
>>> On Tue, Aug 9, 2016 at 1:06 PM, Mathieu Desnoyers
>>> <> 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.
> There is no universal concept of the user-space pointer size on x86
> because x86 code can change it via long jumps.
> What are you actually trying to do? I would guess that
> user_64bit_mode(regs) is the right thing here, because the rseq data
> structure is describing the currently executing code.

Yes, that's correct, we care about the pointer size of currently executing
code. On x86 user_64bit_mode(regs) would appear to be the right thing to do.

>>> 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 ?
> u64 is a perfectly valid, if odd, userspace pointer on all
> architecures that I know of, and it's certainly a valid userspace
> pointer on x86 32-bit userspace (the high bits will just all be zero).
> Can you just use u64?

My concern is about a 32-bit user-space putting garbage rather than zeroes
(on purpose) to fool the kernel on those upper 32 bits. Doing


effectively ends up clearing the upper 32 bits.

But since we only use those pointer values for comparisons, perhaps we
just don't care if a 32-bit userspace app try to shoot itself in
the foot by passing garbage upper 32 bits ?

> If this would be a performance problem on ARM, then maybe that's a
> reason to use compat helpers.

We already use 64-bit values for the pointers, even on 32-bit. Normally
userspace just puts zeroes in the top bits. It's mostly a question of
clearing the top 32 bits or not when loading them in the kernel. If we
don't need to, then I can remove the compat code entirely, and we don't
care about user_64bit_mode() anymore, as you initially recommended.
Does it make sense ?

>>>>>> +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.
> I think that's problematic. Why can't you unregister an existing
> rseq? If you can't, how is a thread supposed to clean up after
> itself?

Unregistering an existing thread rseq would require that we keep reference
counting, in case multiple libs and/or the app are using rseq. I am
trying to keep things as simple as needed.

If I understand your concern, the problematic scenario would be at
thread exit (this is my current approximate understanding of glibc
handling of library TLS variable reclaim at thread exit):

thread exits in userspace:
- glibc frees its rseq TLS memory area (in case the TLS is in a library),
- thread preempted before really exiting,
- kernel reads/writes to freed TLS memory.
- corruption may occur (e.g. memory re-allocated by another thread already)

Am I getting it right ?



> --Andy

Mathieu Desnoyers
EfficiOS Inc.

 \ /
  Last update: 2016-08-10 22:41    [W:0.274 / U:1.500 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site