Re: [PATCH] Add Prefer_MAP_32BIT_EXEC for Silvermont

["Carlos O'Donell" <carlos at redhat dot com>]

On 12/15/2015 03:08 PM, H.J. Lu wrote:
> On Tue, Dec 15, 2015 at 10:38 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> > On 12/15/2015 01:27 PM, Carlos O'Donell wrote:
>>> >> +  cpu_features->feature[index_Prefer_MAP_32BIT_EXEC]
>>> >> +    |= get_prefer_map_32bit_exec ();
>>> >>
>>> >> You wouldn't need get_prefer_map_32bit_exec, since this is all part of
>>> >> the code, like dl-librecon.h, which parses the extra env var.
>> >
>> > To be clear:
>> >
>> > * Add new bit flag definitions for cpu_features.
>> > * Add a sysdeps/unix/sysv/linux/x86_64/dl-silvermont.h
>> >   * Fill in EXTRA_LD_ENVVARS or add new ones.
>> >   * Write to rtld's GLRO cpu_features the bit you need based
>> >     on __libc_enable_secure.
>> >
>> > That should simplify and concentrate the Silvermont fixes to
>> > just two files, making future maintenance and documentation
>> > easier.
>> >
>> >
> This is the updated patch.  I put EXTRA_LD_ENVVARS and
> EXTRA_UNSECURE_ENVVARS in x86_64/64/dl-librecon.h
> to be consistent with i386/dl-librecon.h.  If we ever need to
> update EXTRA_LD_ENVVARS/EXTRA_UNSECURE_ENVVARS
> in the future, we have a single file to change.
> 
> Tested on x86-64.  OK for master?
> 
> Thanks for all the feedbacks and suggestions.

This looks much better and much cleaner. Looks good to me now. It also
appears you have consesnsus with this last change.

It needs a bug # please since you're fixing a user-visible performance
problem on Silvermont.

It appears to meet Zack's requirement to choose a security safe default
at the expense of performance (I agree with that).

I *strongly* urge you to immediately submit a patch to the linux man
pages project to document that as of 2.23 this new flag exists and
does what you describe it does.

> -- H.J.
> 
> 
> 0001-Add-Prefer_MAP_32BIT_EXEC-to-map-executable-pages-wi.patch
> 
> 
> From 940a29897c4018a5c13bef99d0918311629b5f10 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Wed, 21 Oct 2015 14:44:23 -0700
> Subject: [PATCH] Add Prefer_MAP_32BIT_EXEC to map executable pages with
>  MAP_32BIT
> 
> According to Silvermont software optimization guide, for 64-bit
> applications, branch prediction performance can be negatively impacted
> when the target of a branch is more than 4GB away from the branch.  Add
> the Prefer_MAP_32BIT_EXEC bit so that mmap will try to map executable
> pages with MAP_32BIT first.  NB: MAP_32BIT will map to lower 2GB, not
> lower 4GB, address.  Prefer_MAP_32BIT_EXEC reduces bits available for
> address space layout randomization (ASLR), which is always disabled for
> SUID programs and can only be enabled by setting environment variable,
> LD_PREFER_MAP_32BIT_EXEC.
> 
> On Fedora 23, this patch speeds up GCC 5 testsuite by 3% on Silvermont.
> 
> 	* sysdeps/unix/sysv/linux/wordsize-64/mmap.c: New file.
> 	* sysdeps/unix/sysv/linux/x86_64/64/dl-librecon.h: Likewise.
> 	* sysdeps/unix/sysv/linux/x86_64/64/mmap.c: Likewise.
> 	* sysdeps/x86/cpu-features.h (bit_Prefer_MAP_32BIT_EXEC): New.
> 	(index_Prefer_MAP_32BIT_EXEC): Likewise.
> ---
>  sysdeps/unix/sysv/linux/wordsize-64/mmap.c      | 40 ++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/x86_64/64/dl-librecon.h | 44 +++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/x86_64/64/mmap.c        | 37 +++++++++++++++++++++
>  sysdeps/x86/cpu-features.h                      |  3 ++
>  4 files changed, 124 insertions(+)
>  create mode 100644 sysdeps/unix/sysv/linux/wordsize-64/mmap.c
>  create mode 100644 sysdeps/unix/sysv/linux/x86_64/64/dl-librecon.h
>  create mode 100644 sysdeps/unix/sysv/linux/x86_64/64/mmap.c
> 
> diff --git a/sysdeps/unix/sysv/linux/wordsize-64/mmap.c b/sysdeps/unix/sysv/linux/wordsize-64/mmap.c
> new file mode 100644
> index 0000000..e098976
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/wordsize-64/mmap.c
> @@ -0,0 +1,40 @@
> +/* Linux mmap system call.  64-bit version.
> +   Copyright (C) 2015 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/>.  */
> +
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +#include <errno.h>
> +#include <sysdep.h>

OK.
> +
> +/* An architecture may override this.  */
> +#ifndef MMAP_PREPARE
> +# define MMAP_PREPARE(addr, len, prot, flags, fd, offset)
> +#endif
> +
> +__ptr_t
> +__mmap (__ptr_t addr, size_t len, int prot, int flags, int fd, off_t offset)
> +{
> +  MMAP_PREPARE (addr, len, prot, flags, fd, offset);
> +  return (__ptr_t) INLINE_SYSCALL (mmap, 6, addr, len, prot, flags,
> +				   fd, offset);
> +}

OK.

> +
> +weak_alias (__mmap, mmap)
> +weak_alias (__mmap, mmap64)
> +weak_alias (__mmap, __mmap64)

OK.

> diff --git a/sysdeps/unix/sysv/linux/x86_64/64/dl-librecon.h b/sysdeps/unix/sysv/linux/x86_64/64/dl-librecon.h
> new file mode 100644
> index 0000000..159d0f1
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86_64/64/dl-librecon.h
> @@ -0,0 +1,44 @@
> +/* Optional code to distinguish library flavours.  x86-64 version.
> +   Copyright (C) 2015 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 _DL_LIBRECON_H
> +
> +#include <sysdeps/unix/sysv/linux/dl-librecon.h>

OK.

> +
> +/* Recognizing extra environment variables.  For 64-bit applications,
> +   branch prediction performance may be negatively impacted when the
> +   target of a branch is more than 4GB away from the branch.  Add the
> +   Prefer_MAP_32BIT_EXEC bit so that mmap will try to map executable
> +   pages with MAP_32BIT first.  NB: MAP_32BIT will map to lower 2GB,
> +   not lower 4GB, address.  Prefer_MAP_32BIT_EXEC reduces bits available
> +   for address space layout randomization (ASLR).  Prefer_MAP_32BIT_EXEC
> +   is always disabled for SUID programs and can be enabled by setting
> +   environment variable, LD_PREFER_MAP_32BIT_EXEC.  */
> +#define EXTRA_LD_ENVVARS \
> +  case 21:							      \
> +    if (memcmp (envline, "PREFER_MAP_32BIT_EXEC", 21) == 0)	      \
> +      GLRO(dl_x86_cpu_features).feature[index_Prefer_MAP_32BIT_EXEC]  \
> +	= bit_Prefer_MAP_32BIT_EXEC;				      \
> +    break;
> +

OK.

> +/* Extra unsecure variables.  The names are all stuffed in a single
> +   string which means they have to be terminated with a '\0' explicitly.  */
> +#define EXTRA_UNSECURE_ENVVARS \
> +  "LD_PREFER_MAP_32BIT_EXEC\0"

OK.

> +
> +#endif /* dl-librecon.h */
> diff --git a/sysdeps/unix/sysv/linux/x86_64/64/mmap.c b/sysdeps/unix/sysv/linux/x86_64/64/mmap.c
> new file mode 100644
> index 0000000..031316c
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86_64/64/mmap.c
> @@ -0,0 +1,37 @@
> +/* Linux mmap system call.  x86-64 version.
> +   Copyright (C) 2015 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/>.  */
> +
> +#include <ldsodefs.h>
> +
> +/* If the Prefer_MAP_32BIT_EXEC bit is set, try to map executable pages
> +   with MAP_32BIT first.  */
> +#define MMAP_PREPARE(addr, len, prot, flags, fd, offset)		\
> +  if ((addr) == NULL							\
> +      && ((prot) & PROT_EXEC) != 0					\
> +      && HAS_ARCH_FEATURE (Prefer_MAP_32BIT_EXEC))			\
> +    {									\
> +      __ptr_t ret = (__ptr_t) INLINE_SYSCALL (mmap, 6, (addr), (len),	\
> +					      (prot),			\
> +					      (flags) | MAP_32BIT,	\
> +					      (fd), (offset));		\
> +      if (ret != MAP_FAILED)						\
> +	return ret;							\
> +    }

OK.

> +
> +#include <sysdeps/unix/sysv/linux/wordsize-64/mmap.c>
> diff --git a/sysdeps/x86/cpu-features.h b/sysdeps/x86/cpu-features.h
> index 80edbee..93bee69 100644
> --- a/sysdeps/x86/cpu-features.h
> +++ b/sysdeps/x86/cpu-features.h
> @@ -33,6 +33,7 @@
>  #define bit_AVX512DQ_Usable		(1 << 13)
>  #define bit_I586			(1 << 14)
>  #define bit_I686			(1 << 15)
> +#define bit_Prefer_MAP_32BIT_EXEC	(1 << 16)
>  
>  /* CPUID Feature flags.  */
>  
> @@ -97,6 +98,7 @@
>  # define index_AVX512DQ_Usable		FEATURE_INDEX_1*FEATURE_SIZE
>  # define index_I586			FEATURE_INDEX_1*FEATURE_SIZE
>  # define index_I686			FEATURE_INDEX_1*FEATURE_SIZE
> +# define index_Prefer_MAP_32BIT_EXEC	FEATURE_INDEX_1*FEATURE_SIZE

OK.

>  
>  # if defined (_LIBC) && !IS_IN (nonlib)
>  #  ifdef __x86_64__
> @@ -248,6 +250,7 @@ extern const struct cpu_features *__get_cpu_features (void)
>  # define index_AVX512DQ_Usable		FEATURE_INDEX_1
>  # define index_I586			FEATURE_INDEX_1
>  # define index_I686			FEATURE_INDEX_1
> +# define index_Prefer_MAP_32BIT_EXEC	FEATURE_INDEX_1

OK.

>  
>  #endif	/* !__ASSEMBLER__ */
>  
> -- 2.5.0

Cheers,
Carlos.