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

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

[Akihiko Odaki]

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>
---
 util/coroutine-ucontext.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

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);
 #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);
+}
+#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>

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

[Stefan Hajnoczi]

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

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

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);
>  #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);
> +}
> +#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>
> 

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

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

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.