Re: [PATCH] coroutine-ucontext: Save fake stack for pooled coroutine

[Akihiko Odaki]

On 2024/01/16 17:42, Marc-André Lureau wrote:
> Hi
>
> On Mon, Jan 15, 2024 at 10:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Fri, Jan 12, 2024 at 07:36:19PM +0900, Akihiko Odaki wrote:
> > > Coroutine may be pooled even after COROUTINE_TERMINATE if
> > > CONFIG_COROUTINE_POOL is enabled and fake stack should be saved in
> > > such a case to keep AddressSanitizerUseAfterReturn working. Even worse,
> > > I'm seeing stack corruption without fake stack being saved.
> > >
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> Thanks Akihiko, this is solving a crash when enabling ASAN!
>
> > > ---
> > >   util/coroutine-ucontext.c | 21 +++++++++++++++++++--
> > >   1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > Adding Marc-André Lureau and Lingfeng Yang, who authored the code in
> > question.
>
> Side note:
> I am surprised that commit 0aebab04b9  "configure: add --enable-tsan
> flag + fiber annotations" changed code like this:
>   {
>   #ifdef CONFIG_ASAN
> -    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
> +    __sanitizer_start_switch_fiber(
> +            action == COROUTINE_TERMINATE ? NULL : fake_stack_save,
> +            bottom, size);
> +#endif
> +#ifdef CONFIG_TSAN
> +    void *curr_fiber =
> +        __tsan_get_current_fiber();
> +    __tsan_acquire(curr_fiber);
> +
> +    *fake_stack_save = curr_fiber;
> +    __tsan_switch_to_fiber(new_fiber, 0);  /* 0=synchronize */
>   #endif
>
> *fake_stack_save = curr_fiber:
> Is TSAN compatible with ASAN ? I guess not.

meson.build has the following:
if get_option('tsan')
  if get_option('sanitizers')
    error('TSAN is not supported with other sanitizers')
  endif

> It would probably help to have more explicit comments & errors if such
> a case happens.

Added G_STATIC_ASSERT(!IS_ENABLED(CONFIG_TSAN)) in v2.

> > Stefan
> >
> > > diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
> > > index 7b304c79d942..e62ced5d6779 100644
> > > --- a/util/coroutine-ucontext.c
> > > +++ b/util/coroutine-ucontext.c
> > > @@ -124,8 +124,9 @@ void start_switch_fiber_asan(CoroutineAction action, void 
> > > **fake_stack_save,
> > >   {
> > >   #ifdef CONFIG_ASAN
> > >       __sanitizer_start_switch_fiber(
> > > -            action == COROUTINE_TERMINATE ? NULL : fake_stack_save,
> > > -            bottom, size);
> > > +        !IS_ENABLED(CONFIG_COROUTINE_POOL) && action == COROUTINE_TERMINATE ?
> > > +            NULL : fake_stack_save,
> > > +        bottom, size);
>
>
> Ok, changing back the commit from Lingfeng when coroutine pools are enabled.
>
> > >   #endif
> > >   }
> > >
> > > @@ -269,10 +270,26 @@ static inline void 
> > > valgrind_stack_deregister(CoroutineUContext *co)
> > >   #endif
> > >   #endif
> > >
> > > +#if defined(CONFIG_ASAN) && defined(CONFIG_COROUTINE_POOL)
> > > +static void coroutine_fn terminate(void *opaque)
> > > +{
> > > +    CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, opaque);
> > > +
> > > +    __sanitizer_start_switch_fiber(NULL, to->stack, to->stack_size);
> > > +    siglongjmp(to->env, COROUTINE_ENTER);
> > > +}
>
> looking at 
> https://github.com/llvm/llvm-project/blob/main/compiler-rt/test/asan/TestCases/Linux/swapcontext_annotation.cpp,
> that seems correct to me to destroy the fake_stack.
>
> However, not switching back with qemu_coroutine_switch() may create
> issues: set_current() (and tsan) not being called appropriately.

Thanks for catching this. Added missing set_current() call and 
G_STATIC_ASSERT(!IS_ENABLED(CONFIG_TSAN)) in v2 as I described earlier.

> Should we introduce another action like COROUTINE_DELETE?

I don't think so. CoroutineAction is part of the common interface for 
coroutine backends. The need to switch back to the destroyed coroutine 
is a peculiarity unique to ucontext and better to be contained in 
coroutine-ucontext.c.

Alternatively we can add a bool parameter to qemu_coroutine_switch() to 
tell to destroy the fake stack, but probably it's not that worth to 
share qemu_coroutine_switch() code; while coroutine-ucontext.c already 
have a few places that call start_switch_fiber_asan() or siglongjmp() 
and they are somewhat similar, they are still too diverged to unify.