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

[Marc-André Lureau]

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.

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

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

Should we introduce another action like COROUTINE_DELETE?

> > +#endif
> > +
> >  void qemu_coroutine_delete(Coroutine *co_)
> >  {
> >      CoroutineUContext *co = DO_UPCAST(CoroutineUContext, base, co_);
> >
> > +#if defined(CONFIG_ASAN) && defined(CONFIG_COROUTINE_POOL)
> > +    co_->entry_arg = qemu_coroutine_self();
> > +    co_->entry = terminate;
> > +    qemu_coroutine_switch(co_->entry_arg, co_, COROUTINE_ENTER);
> > +#endif
> > +
> >  #ifdef CONFIG_VALGRIND_H
> >      valgrind_stack_deregister(co);
> >  #endif
> >
> > ---
> > base-commit: f614acb7450282a119d85d759f27eae190476058
> > change-id: 20240112-asan-eb695c769f40
> >
> > Best regards,
> > --
> > Akihiko Odaki <akihiko.odaki@daynix.com>
> >



-- 
Marc-André Lureau