is stdlib/tst-setcontext7 a bad test?

[Szabolcs Nagy]

The 03/27/2020 08:10, H.J. Lu wrote:
> On Fri, Mar 27, 2020 at 8:01 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> >
> > The 03/27/2020 06:16, H.J. Lu wrote:
> > > On Fri, Mar 27, 2020 at 5:49 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Fri, Mar 27, 2020 at 5:46 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > >
> > > > > On Fri, Mar 27, 2020 at 4:04 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > >
> > > > > > The 03/27/2020 11:10, Florian Weimer wrote:
> > > > > > > * Szabolcs Nagy:
> > > > > > > > i'm planing to change swapcontext on aarch64 and i see
> > > > > > > >
> > > > > > > > FAIL: stdlib/tst-setcontext7
> > > > > > > >
> > > > > > > > that test case seems wrong to me, a simplified version is
> > > > > > > >
> > > > > > > > #include <ucontext.h>
> > > > > > > > #include <stdio.h>
> > > > > > > >
> > > > > > > > ucontext_t uc[2];
> > > > > > > > volatile int count = 0;
> > > > > > > >
> > > > > > > > int main ()
> > > > > > > > {
> > > > > > > >     getcontext (uc+0);
> > > > > > > >     printf ("%d\n", count);
> > > > > > > >     if (count)
> > > > > > > >             setcontext (uc+1);
> > > > > > > >     count++;
> > > > > > > >     swapcontext (uc+1, uc+0);
> > > > > > > >     printf ("done\n");
> > > > > > > >     return 0;
> > > > > > > > }
> > > > > > > >
> > > > > > > > this seems to work today and prints
> > > > > > > >
> > > > > > > > 0
> > > > > > > > 1
> > > > > > > > done
> > > > > > > >
> > > > > > > > but i don't think we can guarantee that after a
> > > > > > > > swapcontext to a getcontext on the same stack frame
> > > > > > > > resuming the context works: if swapcontext uses the
> > > > > > > > stack (which is what i plan to do) then that stack
> > > > > > > > will be corrupted.
> > > > > > > >
> > > > > > > > does glibc plan to support this usage?
> > > > > > >
> > > > > > > I think it's expected to work.  This seems to be one of the more
> > > > > > > harmless cases.
> > > > > > >
> > > > > > > > i.e. should i ensure swapcontext does not use the stack? (and should
> > > > > > > > gcc ensure no stack is used in the example between getcontext and
> > > > > > > > swapcontext?)
> > > > > > >
> > > > > > > Does the test case pass if you add the returns_twice attribute (or
> > > > > > > __INDIRECT_RETURN) to getcontext as well (swapcontext already has it)?
> > > > > >
> > > > > > yes, the returns_twice attribute on swapcontext
> > > > > > makes bti work, but that seems ugly (swapcontext
> > > > > > only returns once).
> > > > > >
> > > > > > (getcontext has the return_twice attribute because
> > > > > > it's a gcc builtin.)
> > > > > >
> > > > > > the way i planed to handle swapcontext is to do
> > > > > >
> > > > > > swapcontext:
> > > > > >         bti c
> > > > > >         stp x29, x30, [sp, #-16]! // save return address
> > > > > >         bl internal_swapcontext
> > > > > >         bti j
> > > > > >         ldp x29, x30, [sp], #16 // restore return address
> > > > > >         ret
> > > > > >
> > > > > > then bti j (landing pad for indirect jump) is
> > > > > > in libc code, otherwise the compiler has to
> > > > > > special case swapcontext and emit a bti j after
> > > > > > every call site (getcontext is already special),
> > > > > > with my solution swapcontext is not special.
> > > > > >
> > > > > > but if glibc plans to support jumping around
> > > > > > within the same stack frame then my solution
> > > > > > does not work so i will have to rely on the
> > > > > > attribute.
> > > > > >
> > > > >
> > > > > Can't you use indirect_return function attribute like the fix for
> > > > >
> > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85620
> > > > >
> > > >
> > > > You should also check if c-c++-common/asan/swapcontext-test-1.c
> > > > is OK:
> > > >
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86560
> > > >
> > >
> > > You also need to provide bits/indirect-return.h for
> > >
> > > /* __INDIRECT_RETURN is used on swapcontext to indicate if it requires
> > >    special compiler treatment.  */
> > > #define __INDIRECT_RETURN
> > >
> > > See sysdeps/x86/bits/indirect-return.h for example.
> >
> > thanks for the heads up.
> >
> > i've seen bits/indirec-return.h. i would still prefer
> > not to special case swapcontext, that can avoid all
> > sorts of bugs and compatible with existing compilers.
> >
> > the context switch to a place on the same stack and
> > back is what seems unsupportable to me: it requires
> > the compiler to know the callers of swapcontext and
> > generate code specially: this is already broken on
> > all targets without special annotation, since there
> > is nothing preventing the compiler to use the stack
> > in ways that breaks such usage.
> >
> > if we want to fix this then the attribute has to be
> > introduced across all targets and gcc has to handle
> > that in a generic way (i.e. not conditionally adding
> > the attribute in libc only when cet/bti is enabled)
> >
> > but that's a libc api change (changing the type of
> > swapcontext) affecting users who take the address
> > of swapcontext.
> 
> We haven't run into any problems so far.
> 
> > it's not clear to me if going the x86 way is more
> > future proof or just no longer supporting usage
> > like stdlib/tst-setcontext7.
> >
> 
> Please take a closer look at these GCC bug reports.
> You will also see
> 
> FAIL: c-c++-common/asan/swapcontext-test-1.c
> 
> if swapcontext isn't properly handled.

i dont think that test would be a problem if the
BTI J (or ENDBR) landing pad was inside libc in a
wrapper around the internal swapcontext where
context switching calls can return to.

the only problem is that currently swapcontext is
not allowed to use the stack which to me seems like
an unusual restriction since the compiler can make
transformations at the call site that adds stack
usage around the call so it does not seem to be
worth supporting.