This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 03/12] Add <bits/indirect-return.h>
On 07/21/2018 10:20 AM, H.J. Lu wrote:
> Add <bits/indirect-return.h> and include it in <ucontext.h>.
> __INDIRECT_RETURN defined in <bits/indirect-return.h> indicates if
> swapcontext requires special compiler treatment. The default
> __INDIRECT_RETURN is empty.
>
> On x86, when shadow stack is enabled, __INDIRECT_RETURN is defined
> with indirect_return attribute, which has been added to GCC 9, to
> indicate that swapcontext returns via indirect branch. Otherwise
> __INDIRECT_RETURN is defined with returns_twice attribute.
I saw this discussion and I think a returns_twice is the right solution
for older compilers since it removes possibly incorrect optimizations.
> When shadow stack is enabled, remove always_inline attribute from
> prepare_test_buffer in string/tst-xbzero-opt.c to avoid:
>
> tst-xbzero-opt.c: In function ‘prepare_test_buffer’:
> tst-xbzero-opt.c:105:1: error: function ‘prepare_test_buffer’ can never be inlined because it uses setjmp
> prepare_test_buffer (unsigned char *buf)
OK.
> when indirect_return attribute isn't available.
>
> * bits/indirect-return.h: New file.
> * misc/sys/cdefs.h (__glibc_has_attribute): New.
> * sysdeps/x86/bits/indirect-return.h: Likewise.
> * stdlib/Makefile (headers): Add bits/indirect-return.h.
> * stdlib/ucontext.h: Include <bits/indirect-return.h>.
> (swapcontext): Add __INDIRECT_RETURN.
> * string/tst-xbzero-opt.c (ALWAYS_INLINE): New.
> (prepare_test_buffer): Use it.
OK with suggestion.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ---
> bits/indirect-return.h | 25 +++++++++++++++++++++
> misc/sys/cdefs.h | 6 +++++
> stdlib/Makefile | 2 +-
> stdlib/ucontext.h | 6 ++++-
> string/tst-xbzero-opt.c | 10 ++++++++-
> sysdeps/x86/bits/indirect-return.h | 35 ++++++++++++++++++++++++++++++
> 6 files changed, 81 insertions(+), 3 deletions(-)
> create mode 100644 bits/indirect-return.h
> create mode 100644 sysdeps/x86/bits/indirect-return.h
>
> diff --git a/bits/indirect-return.h b/bits/indirect-return.h
> new file mode 100644
> index 0000000000..47f6f15a6e
> --- /dev/null
> +++ b/bits/indirect-return.h
> @@ -0,0 +1,25 @@
> +/* Definition of __INDIRECT_RETURN. Generic version.
> + Copyright (C) 2018 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#ifndef _UCONTEXT_H
> +# error "Never include <bits/indirect-return.h> directly; use <ucontext.h> instead."
> +#endif
> +
> +/* __INDIRECT_RETURN is used on swapcontext to indicate if it requires
> + special compiler treatment. */
> +#define __INDIRECT_RETURN
OK. Does nothing.
> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index e80a45ca68..3f6fe3cc85 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
> @@ -406,6 +406,12 @@
> # define __glibc_likely(cond) (cond)
> #endif
>
> +#ifdef __has_attribute
> +# define __glibc_has_attribute(attr) __has_attribute (attr)
> +#else
> +# define __glibc_has_attribute(attr) 0
> +#endif
OK. In gcc < 5 which lacks __has_attribute this will resolve to 0,
and with that we will get returns_twice attribute being used, and
that's fine.
> +
> #if (!defined _Noreturn \
> && (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) < 201112 \
> && !__GNUC_PREREQ (4,7))
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 808a8ceab7..b5e55b0a55 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -26,7 +26,7 @@ headers := stdlib.h bits/stdlib.h bits/stdlib-ldbl.h bits/stdlib-float.h \
> monetary.h bits/monetary-ldbl.h \
> inttypes.h stdint.h bits/wordsize.h \
> errno.h sys/errno.h bits/errno.h bits/types/error_t.h \
> - ucontext.h sys/ucontext.h \
> + ucontext.h sys/ucontext.h bits/indirect-return.h \
OK.
> alloca.h fmtmsg.h \
> bits/stdlib-bsearch.h sys/random.h bits/stdint-intn.h \
> bits/stdint-uintn.h
> diff --git a/stdlib/ucontext.h b/stdlib/ucontext.h
> index eec7611631..ec630038f6 100644
> --- a/stdlib/ucontext.h
> +++ b/stdlib/ucontext.h
> @@ -22,6 +22,9 @@
>
> #include <features.h>
>
> +/* Get definition of __INDIRECT_RETURN. */
> +#include <bits/indirect-return.h>
OK.
> +
> /* Get machine dependent definition of data structures. */
> #include <sys/ucontext.h>
>
> @@ -36,7 +39,8 @@ extern int setcontext (const ucontext_t *__ucp) __THROWNL;
> /* Save current context in context variable pointed to by OUCP and set
> context from variable pointed to by UCP. */
> extern int swapcontext (ucontext_t *__restrict __oucp,
> - const ucontext_t *__restrict __ucp) __THROWNL;
> + const ucontext_t *__restrict __ucp)
> + __THROWNL __INDIRECT_RETURN;
OK.
>
> /* Manipulate user context UCP to continue with calling functions FUNC
> and the ARGC-1 parameters following ARGC when the context is used
> diff --git a/string/tst-xbzero-opt.c b/string/tst-xbzero-opt.c
> index cf7041f37a..aab4a7f715 100644
> --- a/string/tst-xbzero-opt.c
> +++ b/string/tst-xbzero-opt.c
> @@ -100,7 +100,15 @@ static ucontext_t uc_main, uc_co;
> /* Always check the test buffer immediately after filling it; this
> makes externally visible side effects depend on the buffer existing
> and having been filled in. */
> -static inline __attribute__ ((always_inline)) void
> +#if defined __CET__ && !__glibc_has_attribute (__indirect_return__)
> +/* Note: swapcontext returns via indirect branch when SHSTK is enabled.
> + Without indirect_return attribute, swapcontext is marked with
> + returns_twice attribute, which prevents always_inline to work. */
> +# define ALWAYS_INLINE
> +#else
> +# define ALWAYS_INLINE __attribute__ ((always_inline))
> +#endif
> +static inline ALWAYS_INLINE void
OK. I don't think this will impact the semantics of the test, the compiler
should still be able to see the local call and carry out any optimizations
it needs to do.
> prepare_test_buffer (unsigned char *buf)
> {
> for (unsigned int i = 0; i < PATTERN_REPS; i++)
> diff --git a/sysdeps/x86/bits/indirect-return.h b/sysdeps/x86/bits/indirect-return.h
> new file mode 100644
> index 0000000000..0587e687ac
> --- /dev/null
> +++ b/sysdeps/x86/bits/indirect-return.h
> @@ -0,0 +1,35 @@
> +/* Definition of __INDIRECT_RETURN. x86 version.
> + Copyright (C) 2018 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#ifndef _UCONTEXT_H
> +# error "Never include <bits/indirect-return.h> directly; use <ucontext.h> instead."
> +#endif
> +
> +/* On x86, swapcontext returns via indirect branch when the shadow stack
> + is enabled. Define __INDIRECT_RETURN to indicate whether swapcontext
> + returns via indirect branch. */
> +#if defined __CET__ && (__CET__ & 2) != 0
> +# if __glibc_has_attribute (__indirect_return__)
> +# define __INDIRECT_RETURN __attribute__ ((__indirect_return__))
> +# else
> +/* Without indirect_return attribute, use returns_twice attribute. */
Suggest:
/* Newer compilers provide the indirect_return attribute, but without
it we can use returns_twice to affect the optimizer in the same
way and avoid unsafe optimizations. */
> +# define __INDIRECT_RETURN __attribute__ ((__returns_twice__))
> +# endif
> +#else
> +# define __INDIRECT_RETURN
> +#endif
>