Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

On Fri, May 22, 2020 at 11:18:20AM -0400, Daniele Buono wrote:
> On 5/21/2020 5:44 AM, Stefan Hajnoczi wrote:
> > On Wed, Apr 29, 2020 at 03:44:17PM -0400, Daniele Buono wrote:
> > > @@ -160,6 +169,19 @@ Coroutine *qemu_coroutine_new(void)
> > >       /* swapcontext() in, siglongjmp() back out */
> > >       if (!sigsetjmp(old_env, 0)) {
> > >           start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
> > > +#ifdef CONFIG_SAFESTACK
> > > +        /*
> > > +         * Before we swap the context, set the new unsafe stack
> > > +         * The unsafe stack grows just like the normal stack, so start from
> > > +         * the last usable location of the memory area.
> > > +         * NOTE: we don't have to re-set it afterwards because sigsetjmp was
> > > +         * called with the original usp. Since we are not coming back with a
> > > +         * swapcontext, but with a siglongjmp, when we are back here we
> > > +         * already have usp restored to the valid one for this function
> > 
> > I don't understand this comment. __safestack_unsafe_stack_ptr is a
> > thread-local variable, not a CPU register. How will siglongjmp()
> > automatically restore it?
> > 
> Correct, setjmp/longjmp have no visibility of the unsafe stack. What I
> meant is that it is not automatically restored by the longjmp itself,
> but by code the compiler adds around the sigsetjmp.
> 
> Specifically, every sigsetjmp/sigjmp is intercepted by the compiler, the
> current value of __safestack_unsafe_stack_ptr is saved on the normal
> (safe) stack.
> Right after the sigsetjmp call it is then restored.
> 
> I will change the comment to make it clearer.
> 
> In practice, this is what happens:
> 
> Original clang implementation in qemu_coroutine_new:
> ----
> 40130c:  callq  4008d0 <__sigsetjmp@plt>
> 401311:  cmp    $0x0,%eax
> 401314:  jne    40132d <qemu_coroutine_new+0x12d>
> ----
> Clang Implementation with safestack:
> ----
> 4027a7:  mov    %rdx,-0x38(%rbp) <- Save unsafe ptr onto the safe stack
> [...]
> 40289c:  callq  401410 <__sigsetjmp@plt>
> 4028a1:  mov    0x201738(%rip),%rdi        # 603fe0
> <__safestack_unsafe_stack_ptr@@Base+0x603fe0>
> 4028a8:  mov    -0x38(%rbp),%rbx
> 4028ac:  mov    %rbx,%fs:(%rdi) <- Restore the unsafe ptr
> 4028b0:  cmp    $0x0,%eax
> 4028b3:  jne    4028d9 <qemu_coroutine_new+0x179>

Oh, that's interesting. Thanks for explaining and updating the comment.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]