* [PATCH] coroutine-ucontext: Save fake stack for pooled coroutine @ 2024-01-12 10:36 Akihiko Odaki 2024-01-15 18:47 ` Stefan Hajnoczi 0 siblings, 1 reply; 4+ messages in thread From: Akihiko Odaki @ 2024-01-12 10:36 UTC (permalink / raw) To: Stefan Hajnoczi, Kevin Wolf; +Cc: qemu-devel, 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> ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] coroutine-ucontext: Save fake stack for pooled coroutine 2024-01-12 10:36 [PATCH] coroutine-ucontext: Save fake stack for pooled coroutine Akihiko Odaki @ 2024-01-15 18:47 ` Stefan Hajnoczi 2024-01-16 8:42 ` Marc-André Lureau 0 siblings, 1 reply; 4+ messages in thread From: Stefan Hajnoczi @ 2024-01-15 18:47 UTC (permalink / raw) To: Marc-André Lureau, Lingfeng Yang Cc: Kevin Wolf, qemu-devel, Akihiko Odaki [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] coroutine-ucontext: Save fake stack for pooled coroutine 2024-01-15 18:47 ` Stefan Hajnoczi @ 2024-01-16 8:42 ` Marc-André Lureau 2024-01-17 7:29 ` Akihiko Odaki 0 siblings, 1 reply; 4+ messages in thread From: Marc-André Lureau @ 2024-01-16 8:42 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Lingfeng Yang, Kevin Wolf, qemu-devel, Akihiko Odaki 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] coroutine-ucontext: Save fake stack for pooled coroutine 2024-01-16 8:42 ` Marc-André Lureau @ 2024-01-17 7:29 ` Akihiko Odaki 0 siblings, 0 replies; 4+ messages in thread From: Akihiko Odaki @ 2024-01-17 7:29 UTC (permalink / raw) To: Marc-André Lureau, Stefan Hajnoczi Cc: Lingfeng Yang, Kevin Wolf, qemu-devel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-01-17 7:30 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-12 10:36 [PATCH] coroutine-ucontext: Save fake stack for pooled coroutine Akihiko Odaki 2024-01-15 18:47 ` Stefan Hajnoczi 2024-01-16 8:42 ` Marc-André Lureau 2024-01-17 7:29 ` Akihiko Odaki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).