util/coroutine-ucontext.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)
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>
---
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-e330f0d0032c@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, &fake_stack_save,
+ start_switch_fiber_asan(&fake_stack_save,
leaderp->stack, leaderp->stack_size);
start_switch_fiber_tsan(&fake_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, &fake_stack_save, co->stack,
- co->stack_size);
+ start_switch_fiber_asan(&fake_stack_save, co->stack, co->stack_size);
start_switch_fiber_tsan(&fake_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, &fake_stack_save, to->stack,
- to->stack_size);
+ start_switch_fiber_asan(IS_ENABLED(CONFIG_COROUTINE_POOL) ||
+ action != COROUTINE_TERMINATE ?
+ &fake_stack_save : NULL,
+ to->stack, to->stack_size);
start_switch_fiber_tsan(&fake_stack_save,
to, false); /* false=not caller */
siglongjmp(to->env, action);
---
base-commit: f614acb7450282a119d85d759f27eae190476058
change-id: 20240112-asan-eb695c769f40
Best regards,
--
Akihiko Odaki <akihiko.odaki@daynix.com>
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 <akihiko.odaki@daynix.com> > --- > 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-e330f0d0032c@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
Hi On Wed, Jan 17, 2024 at 11:06 AM Akihiko Odaki <akihiko.odaki@daynix.com> 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> > --- > 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-e330f0d0032c@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, &fake_stack_save, > + start_switch_fiber_asan(&fake_stack_save, > leaderp->stack, leaderp->stack_size); > start_switch_fiber_tsan(&fake_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, &fake_stack_save, co->stack, > - co->stack_size); > + start_switch_fiber_asan(&fake_stack_save, co->stack, co->stack_size); > start_switch_fiber_tsan(&fake_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, &fake_stack_save, to->stack, > - to->stack_size); > + start_switch_fiber_asan(IS_ENABLED(CONFIG_COROUTINE_POOL) || > + action != COROUTINE_TERMINATE ? > + &fake_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 &fake_stack_save here? otherwise, looks ok to me > start_switch_fiber_tsan(&fake_stack_save, > to, false); /* false=not caller */ > siglongjmp(to->env, action); > > --- > base-commit: f614acb7450282a119d85d759f27eae190476058 > change-id: 20240112-asan-eb695c769f40 > > Best regards, > -- > Akihiko Odaki <akihiko.odaki@daynix.com> > -- Marc-André Lureau
Hi On Wed, Jan 17, 2024 at 11:26 AM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Wed, Jan 17, 2024 at 11:06 AM Akihiko Odaki <akihiko.odaki@daynix.com> 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> > > --- > > 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-e330f0d0032c@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, &fake_stack_save, > > + start_switch_fiber_asan(&fake_stack_save, > > leaderp->stack, leaderp->stack_size); > > start_switch_fiber_tsan(&fake_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, &fake_stack_save, co->stack, > > - co->stack_size); > > + start_switch_fiber_asan(&fake_stack_save, co->stack, co->stack_size); > > start_switch_fiber_tsan(&fake_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, &fake_stack_save, to->stack, > > - to->stack_size); > > + start_switch_fiber_asan(IS_ENABLED(CONFIG_COROUTINE_POOL) || > > + action != COROUTINE_TERMINATE ? > > + &fake_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 &fake_stack_save here? > Ah, terminate_asan() is only called when the pool is enabled. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> -- Marc-André Lureau
© 2016 - 2024 Red Hat, Inc.