Re: [PATCH v5 0/4] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

On Thu, Mar 03, 2022 at 01:07:54PM +0100, Kevin Wolf wrote:
> Am 22.02.2022 um 15:01 hat Stefan Hajnoczi geschrieben:
> > v5:
> > - Added explicit "#include "qemu/coroutine-tls.h" in patch 4 [Philippe]
> > - Updated patch 1 commit description and comments to describe the current
> >   noinline plus asm volatile approach [Peter]
> > v4:
> > - Dropped '[RFC]'.
> > - Dropped inline asm for now. -fPIC versions of the code are missing and I
> >   hit several issues including a clang LTO bug where thread local variables 
> > are
> >   incorrectly discarded because inline asm is not analyzed to find symbol
> >   dependencies (Serge Guelton is aware).
> > - Fixed CI failures.
> > v3:
> > - Added __attribute__((weak)) to get_ptr_*() [Florian]
> > - Replace rdfsbase with mov %%fs:0,%0 [Florian]
> > 
> > This patch series solves the coroutines TLS problem. Coroutines re-entered 
> > from
> > another thread sometimes see stale TLS values. This happens because 
> > compilers
> > may cache values across yield points, so a value from the previous thread 
> > will
> > be used when the coroutine is re-entered in another thread.
> > 
> > Serge Guelton developed a portable technique, see the first patch for 
> > details.
> > 
> > I have audited all __thread variables in QEMU and converted those that can 
> > be
> > used from coroutines. Most actually look safe to me.
> 
> Hm, what about the ones in the coroutine implementation itself?
> 
> static __thread CoroutineUContext leader;
> static __thread Coroutine *current;
> 
> Both of them are used in qemu_coroutine_self(), which is a
> coroutine_fn, and in qemu_in_coroutine(), which may be called from
> coroutine context.
> 
> And I seem to remember I've seen crashes related to this in one of the
> bug reports we got, where the stack trace clearly showed that one of
> these functions had returned a wrong result.
> 
> I'm applying this series anyway, it doesn't make the patches incorrect.
> But it feels incomplete, so we may need a follow-up patch.

Thanks for pointing this out. I will send a patch.

current is always the address of our own Coroutine, regardless of which
thread it's executing in. Using the old value is fine because it's
identical to the new value.

However, if the compiler caches the address of current and loads from
the old address then the value will be wrong.

leader could be stale if LTO inlines multiple calls to
coroutine-ucontext.c functions.

So overall it looks unsafe to me.

Stefan

signature.asc
Description: PGP signature