Re: [PATCH v2] coroutine-ucontext: Save fake stack for pooled coroutine
On Wed, Jan 17, 2024 at 04:06:02PM +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 > --- > Changes in v2: > - Added missing set_current() (Marc-André Lureau) > - Added G_STATIC_ASSERT(!IS_ENABLED(CONFIG_TSAN)) (Marc-André Lureau) > - Renamed terminate() to terminate_asan() for clarity and consistency. > - Changed terminate_asan() to call start_switch_fiber_asan() for > consistency. > - Link to v1: > https://lore.kernel.org/r/20240112-asan-v1-1-e330f0d00...@daynix.com > --- > util/coroutine-ucontext.c | 35 ++- > 1 file changed, 26 insertions(+), 9 deletions(-) Thanks, applied to my block tree: https://gitlab.com/stefanha/qemu/commits/block Stefan signature.asc Description: PGP signature
Re: [PATCH v2] coroutine-ucontext: Save fake stack for pooled coroutine
Hi On Wed, Jan 17, 2024 at 11:26 AM Marc-André Lureau wrote: > > Hi > > On Wed, Jan 17, 2024 at 11:06 AM 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 > > --- > > Changes in v2: > > - Added missing set_current() (Marc-André Lureau) > > - Added G_STATIC_ASSERT(!IS_ENABLED(CONFIG_TSAN)) (Marc-André Lureau) > > - Renamed terminate() to terminate_asan() for clarity and consistency. > > - Changed terminate_asan() to call start_switch_fiber_asan() for > > consistency. > > - Link to v1: > > https://lore.kernel.org/r/20240112-asan-v1-1-e330f0d00...@daynix.com > > --- > > util/coroutine-ucontext.c | 35 ++- > > 1 file changed, 26 insertions(+), 9 deletions(-) > > > > diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c > > index 7b304c79d942..8ef603d081ea 100644 > > --- a/util/coroutine-ucontext.c > > +++ b/util/coroutine-ucontext.c > > @@ -119,13 +119,11 @@ void finish_switch_fiber(void *fake_stack_save) > > > > /* always_inline is required to avoid TSan runtime fatal errors. */ > > static inline __attribute__((always_inline)) > > -void start_switch_fiber_asan(CoroutineAction action, void > > **fake_stack_save, > > +void start_switch_fiber_asan(void **fake_stack_save, > > const void *bottom, size_t size) > > { > > #ifdef CONFIG_ASAN > > -__sanitizer_start_switch_fiber( > > -action == COROUTINE_TERMINATE ? NULL : fake_stack_save, > > -bottom, size); > > +__sanitizer_start_switch_fiber(fake_stack_save, bottom, size); > > #endif > > } > > > > @@ -165,7 +163,7 @@ static void coroutine_trampoline(int i0, int i1) > > if (!sigsetjmp(self->env, 0)) { > > CoroutineUContext *leaderp = get_ptr_leader(); > > > > -start_switch_fiber_asan(COROUTINE_YIELD, _stack_save, > > +start_switch_fiber_asan(_stack_save, > > leaderp->stack, leaderp->stack_size); > > start_switch_fiber_tsan(_stack_save, self, true); /* > > true=caller */ > > siglongjmp(*(sigjmp_buf *)co->entry_arg, 1); > > @@ -226,8 +224,7 @@ Coroutine *qemu_coroutine_new(void) > > > > /* swapcontext() in, siglongjmp() back out */ > > if (!sigsetjmp(old_env, 0)) { > > -start_switch_fiber_asan(COROUTINE_YIELD, _stack_save, > > co->stack, > > -co->stack_size); > > +start_switch_fiber_asan(_stack_save, co->stack, > > co->stack_size); > > start_switch_fiber_tsan(_stack_save, > > co, false); /* false=not caller */ > > > > @@ -269,10 +266,28 @@ static inline void > > valgrind_stack_deregister(CoroutineUContext *co) > > #endif > > #endif > > > > +#if defined(CONFIG_ASAN) && defined(CONFIG_COROUTINE_POOL) > > +static void coroutine_fn terminate_asan(void *opaque) > > +{ > > +CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, opaque); > > + > > +set_current(opaque); > > +start_switch_fiber_asan(NULL, to->stack, to->stack_size); > > +G_STATIC_ASSERT(!IS_ENABLED(CONFIG_TSAN)); > > +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_asan; > > +qemu_coroutine_switch(co_->entry_arg, co_, COROUTINE_ENTER); > > +#endif > > + > > #ifdef CONFIG_VALGRIND_H > > valgrind_stack_deregister(co); > > #endif > > @@ -305,8 +320,10 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_, > > > > ret = sigsetjmp(from->env, 0); > > if (ret == 0) { > > -start_switch_fiber_asan(action, _stack_save, to->stack, > > -to->stack_size); > > +start_switch_fiber_asan(IS_ENABLED(CONFIG_COROUTINE_POOL) || > > +action != COROUTINE_TERMINATE ? > > +_stack_save : NULL, > > +to->stack, to->stack_size); > > given that the coroutine is reentered on delete to clear the fake > stack, can we just pass _stack_save here? > Ah, terminate_asan() is only called when the pool is enabled. Reviewed-by: Marc-André Lureau -- Marc-André Lureau
Re: [PATCH v2] coroutine-ucontext: Save fake stack for pooled coroutine
Hi On Wed, Jan 17, 2024 at 11:06 AM 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 > --- > Changes in v2: > - Added missing set_current() (Marc-André Lureau) > - Added G_STATIC_ASSERT(!IS_ENABLED(CONFIG_TSAN)) (Marc-André Lureau) > - Renamed terminate() to terminate_asan() for clarity and consistency. > - Changed terminate_asan() to call start_switch_fiber_asan() for > consistency. > - Link to v1: > https://lore.kernel.org/r/20240112-asan-v1-1-e330f0d00...@daynix.com > --- > util/coroutine-ucontext.c | 35 ++- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c > index 7b304c79d942..8ef603d081ea 100644 > --- a/util/coroutine-ucontext.c > +++ b/util/coroutine-ucontext.c > @@ -119,13 +119,11 @@ void finish_switch_fiber(void *fake_stack_save) > > /* always_inline is required to avoid TSan runtime fatal errors. */ > static inline __attribute__((always_inline)) > -void start_switch_fiber_asan(CoroutineAction action, void **fake_stack_save, > +void start_switch_fiber_asan(void **fake_stack_save, > const void *bottom, size_t size) > { > #ifdef CONFIG_ASAN > -__sanitizer_start_switch_fiber( > -action == COROUTINE_TERMINATE ? NULL : fake_stack_save, > -bottom, size); > +__sanitizer_start_switch_fiber(fake_stack_save, bottom, size); > #endif > } > > @@ -165,7 +163,7 @@ static void coroutine_trampoline(int i0, int i1) > if (!sigsetjmp(self->env, 0)) { > CoroutineUContext *leaderp = get_ptr_leader(); > > -start_switch_fiber_asan(COROUTINE_YIELD, _stack_save, > +start_switch_fiber_asan(_stack_save, > leaderp->stack, leaderp->stack_size); > start_switch_fiber_tsan(_stack_save, self, true); /* > true=caller */ > siglongjmp(*(sigjmp_buf *)co->entry_arg, 1); > @@ -226,8 +224,7 @@ Coroutine *qemu_coroutine_new(void) > > /* swapcontext() in, siglongjmp() back out */ > if (!sigsetjmp(old_env, 0)) { > -start_switch_fiber_asan(COROUTINE_YIELD, _stack_save, co->stack, > -co->stack_size); > +start_switch_fiber_asan(_stack_save, co->stack, co->stack_size); > start_switch_fiber_tsan(_stack_save, > co, false); /* false=not caller */ > > @@ -269,10 +266,28 @@ static inline void > valgrind_stack_deregister(CoroutineUContext *co) > #endif > #endif > > +#if defined(CONFIG_ASAN) && defined(CONFIG_COROUTINE_POOL) > +static void coroutine_fn terminate_asan(void *opaque) > +{ > +CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, opaque); > + > +set_current(opaque); > +start_switch_fiber_asan(NULL, to->stack, to->stack_size); > +G_STATIC_ASSERT(!IS_ENABLED(CONFIG_TSAN)); > +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_asan; > +qemu_coroutine_switch(co_->entry_arg, co_, COROUTINE_ENTER); > +#endif > + > #ifdef CONFIG_VALGRIND_H > valgrind_stack_deregister(co); > #endif > @@ -305,8 +320,10 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_, > > ret = sigsetjmp(from->env, 0); > if (ret == 0) { > -start_switch_fiber_asan(action, _stack_save, to->stack, > -to->stack_size); > +start_switch_fiber_asan(IS_ENABLED(CONFIG_COROUTINE_POOL) || > +action != COROUTINE_TERMINATE ? > +_stack_save : NULL, > +to->stack, to->stack_size); given that the coroutine is reentered on delete to clear the fake stack, can we just pass _stack_save here? otherwise, looks ok to me > start_switch_fiber_tsan(_stack_save, > to, false); /* false=not caller */ > siglongjmp(to->env, action); > > --- > base-commit: f614acb7450282a119d85d759f27eae190476058 > change-id: 20240112-asan-eb695c769f40 > > Best regards, > -- > Akihiko Odaki > -- Marc-André Lureau
[PATCH v2] coroutine-ucontext: Save fake stack for pooled coroutine
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 --- Changes in v2: - Added missing set_current() (Marc-André Lureau) - Added G_STATIC_ASSERT(!IS_ENABLED(CONFIG_TSAN)) (Marc-André Lureau) - Renamed terminate() to terminate_asan() for clarity and consistency. - Changed terminate_asan() to call start_switch_fiber_asan() for consistency. - Link to v1: https://lore.kernel.org/r/20240112-asan-v1-1-e330f0d00...@daynix.com --- util/coroutine-ucontext.c | 35 ++- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c index 7b304c79d942..8ef603d081ea 100644 --- a/util/coroutine-ucontext.c +++ b/util/coroutine-ucontext.c @@ -119,13 +119,11 @@ void finish_switch_fiber(void *fake_stack_save) /* always_inline is required to avoid TSan runtime fatal errors. */ static inline __attribute__((always_inline)) -void start_switch_fiber_asan(CoroutineAction action, void **fake_stack_save, +void start_switch_fiber_asan(void **fake_stack_save, const void *bottom, size_t size) { #ifdef CONFIG_ASAN -__sanitizer_start_switch_fiber( -action == COROUTINE_TERMINATE ? NULL : fake_stack_save, -bottom, size); +__sanitizer_start_switch_fiber(fake_stack_save, bottom, size); #endif } @@ -165,7 +163,7 @@ static void coroutine_trampoline(int i0, int i1) if (!sigsetjmp(self->env, 0)) { CoroutineUContext *leaderp = get_ptr_leader(); -start_switch_fiber_asan(COROUTINE_YIELD, _stack_save, +start_switch_fiber_asan(_stack_save, leaderp->stack, leaderp->stack_size); start_switch_fiber_tsan(_stack_save, self, true); /* true=caller */ siglongjmp(*(sigjmp_buf *)co->entry_arg, 1); @@ -226,8 +224,7 @@ Coroutine *qemu_coroutine_new(void) /* swapcontext() in, siglongjmp() back out */ if (!sigsetjmp(old_env, 0)) { -start_switch_fiber_asan(COROUTINE_YIELD, _stack_save, co->stack, -co->stack_size); +start_switch_fiber_asan(_stack_save, co->stack, co->stack_size); start_switch_fiber_tsan(_stack_save, co, false); /* false=not caller */ @@ -269,10 +266,28 @@ static inline void valgrind_stack_deregister(CoroutineUContext *co) #endif #endif +#if defined(CONFIG_ASAN) && defined(CONFIG_COROUTINE_POOL) +static void coroutine_fn terminate_asan(void *opaque) +{ +CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, opaque); + +set_current(opaque); +start_switch_fiber_asan(NULL, to->stack, to->stack_size); +G_STATIC_ASSERT(!IS_ENABLED(CONFIG_TSAN)); +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_asan; +qemu_coroutine_switch(co_->entry_arg, co_, COROUTINE_ENTER); +#endif + #ifdef CONFIG_VALGRIND_H valgrind_stack_deregister(co); #endif @@ -305,8 +320,10 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_, ret = sigsetjmp(from->env, 0); if (ret == 0) { -start_switch_fiber_asan(action, _stack_save, to->stack, -to->stack_size); +start_switch_fiber_asan(IS_ENABLED(CONFIG_COROUTINE_POOL) || +action != COROUTINE_TERMINATE ? +_stack_save : NULL, +to->stack, to->stack_size); start_switch_fiber_tsan(_stack_save, to, false); /* false=not caller */ siglongjmp(to->env, action); --- base-commit: f614acb7450282a119d85d759f27eae190476058 change-id: 20240112-asan-eb695c769f40 Best regards, -- Akihiko Odaki