[Qemu-devel] [PATCH 1/3] configure: add option to disable -fstack-protector flags

[Qemu-devel] [PATCH 1/3] configure: add option to disable -fstack-protector flags

[Steven Noonan]

From: Steven Noonan <snoonan@amazon.com>

The -fstack-protector flag family is useful for ensuring safety and for
debugging, but has a performance impact. Here's a boot time comparison between
a QEMU build of qemu-system-arm with and without the -fstack-protector-all
flag:

    # WITHOUT -fstack-protector-all
    [root@localhost ~]# systemd-analyze
    Startup finished in 1.744s (kernel) + 11.345s (initrd) + 47.164s (userspace) = 1min 255ms

    # WITH -fstack-protector-all
    [root@localhost ~]# systemd-analyze
    Startup finished in 1.843s (kernel) + 12.262s (initrd) + 1min 3.480s (userspace) = 1min 17.587s

This is a sizable delta, and some users may wish to disable the flag.

Signed-off-by: Steven Noonan <snoonan@amazon.com>
Cc: Anthony Liguori <aliguori@amazon.com>
---
 configure | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 07b6be3..c091cdf 100755
--- a/configure
+++ b/configure
@@ -147,6 +147,7 @@ audio_win_int=""
 cc_i386=i386-pc-linux-gnu-gcc
 libs_qga=""
 debug_info="yes"
+stack_protector="yes"
 
 # Don't accept a target_list environment variable.
 unset target_list
@@ -879,6 +880,10 @@ for opt do
   ;;
   --disable-werror) werror="no"
   ;;
+  --enable-stack-protector) stack_protector="yes"
+  ;;
+  --disable-stack-protector) stack_protector="no"
+  ;;
   --disable-curses) curses="no"
   ;;
   --enable-curses) curses="yes"
@@ -1117,6 +1122,7 @@ echo "  --enable-sparse          enable sparse checker"
 echo "  --disable-sparse         disable sparse checker (default)"
 echo "  --disable-strip          disable stripping binaries"
 echo "  --disable-werror         disable compilation abort on warning"
+echo "  --disable-stack-protector disable GCC-provided stack protection"
 echo "  --disable-sdl            disable SDL"
 echo "  --enable-sdl             enable SDL"
 echo "  --disable-gtk            disable gtk UI"
@@ -1298,9 +1304,11 @@ for flag in $gcc_flags; do
     fi
 done
 
-if compile_prog "-Werror -fstack-protector-all" "" ; then
+if test "$stack_protector" = "yes" ; then
+  if compile_prog "-Werror -fstack-protector-all" "" ; then
     QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
     LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
+  fi
 fi
 
 # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
-- 
1.8.5.2

[Qemu-devel] [PATCH 2/3] virtfs-proxy-helper.c: fix compile error

[Steven Noonan]

From: Steven Noonan <snoonan@amazon.com>

This is caused by a linux/xattr.h and sys/xattr.h incompatibility:

    In file included from /home/snoonan/Development/qemu/include/qemu/xattr.h:27:0,
		     from fsdev/virtfs-proxy-helper.c:25:
    /usr/include/sys/xattr.h:31:3: error: expected identifier before numeric constant
       XATTR_CREATE = 1, /* set value, fail if attr already exists.  */
       ^
    /home/snoonan/Development/qemu/qemu/rules.mak:25: recipe for target 'fsdev/virtfs-proxy-helper.o' failed

Moving the include around resolves it.

Signed-off-by: Steven Noonan <snoonan@amazon.com>
Cc: Anthony Liguori <aliguori@amazon.com>
---
 fsdev/virtfs-proxy-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 713a7b2..c10a085 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -12,7 +12,6 @@
 #include <sys/resource.h>
 #include <getopt.h>
 #include <syslog.h>
-#include <sys/capability.h>
 #include <sys/fsuid.h>
 #include <sys/vfs.h>
 #include <sys/ioctl.h>
@@ -23,6 +22,7 @@
 #include "qemu-common.h"
 #include "qemu/sockets.h"
 #include "qemu/xattr.h"
+#include <sys/capability.h>
 #include "virtio-9p-marshal.h"
 #include "hw/9pfs/virtio-9p-proxy.h"
 #include "fsdev/virtio-9p-marshal.h"
-- 
1.8.5.2

[Qemu-devel] [PATCH 3/3] Makefile: add bios-256k.bin to BLOBS

[Steven Noonan]

From: Steven Noonan <snoonan@amazon.com>

Signed-off-by: Steven Noonan <snoonan@amazon.com>
Cc: Anthony Liguori <aliguori@amazon.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index bdff4e4..807054b 100644
--- a/Makefile
+++ b/Makefile
@@ -290,7 +290,7 @@ common  de-ch  es     fo  fr-ca  hu     ja  mk  nl-be      pt  sl     tr \
 bepo    cz
 
 ifdef INSTALL_BLOBS
-BLOBS=bios.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
+BLOBS=bios.bin bios-256k.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
 vgabios-stdvga.bin vgabios-vmware.bin vgabios-qxl.bin \
 acpi-dsdt.aml q35-acpi-dsdt.aml \
 ppc_rom.bin openbios-sparc32 openbios-sparc64 openbios-ppc QEMU,tcx.bin \
-- 
1.8.5.2

Re: [Qemu-devel] [PATCH 1/3] configure: add option to disable -fstack-protector flags

[Peter Maydell]

On 9 January 2014 21:55, Steven Noonan <steven@uplinklabs.net> wrote:
> From: Steven Noonan <snoonan@amazon.com>
>
> The -fstack-protector flag family is useful for ensuring safety and for
> debugging, but has a performance impact. Here's a boot time comparison between
> a QEMU build of qemu-system-arm with and without the -fstack-protector-all
> flag:
>
>     # WITHOUT -fstack-protector-all
>     [root@localhost ~]# systemd-analyze
>     Startup finished in 1.744s (kernel) + 11.345s (initrd) + 47.164s (userspace) = 1min 255ms
>
>     # WITH -fstack-protector-all
>     [root@localhost ~]# systemd-analyze
>     Startup finished in 1.843s (kernel) + 12.262s (initrd) + 1min 3.480s (userspace) = 1min 17.587s
>
> This is a sizable delta, and some users may wish to disable the flag.

How about benchmarking the intermediate level of protection, ie
just "-fstack-protector"? Maybe that's a good enough compromise
between security and speed that we don't need to mess with
configure... (IIRC there have been discussions before
about why we have the -all variant specifically but I don't recall
anybody coming up with a convincing argument.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] configure: add option to disable -fstack-protector flags

[Paolo Bonzini]

Il 09/01/2014 22:55, Steven Noonan ha scritto:
> From: Steven Noonan <snoonan@amazon.com>
> 
> The -fstack-protector flag family is useful for ensuring safety and for
> debugging, but has a performance impact. Here's a boot time comparison between
> a QEMU build of qemu-system-arm with and without the -fstack-protector-all
> flag:
> 
>     # WITHOUT -fstack-protector-all
>     [root@localhost ~]# systemd-analyze
>     Startup finished in 1.744s (kernel) + 11.345s (initrd) + 47.164s (userspace) = 1min 255ms
> 
>     # WITH -fstack-protector-all
>     [root@localhost ~]# systemd-analyze
>     Startup finished in 1.843s (kernel) + 12.262s (initrd) + 1min 3.480s (userspace) = 1min 17.587s

Can you try -fstack-protector-strong?

Probably the right thing to do is to pick in order
-fstack-protector-strong, -fstack-protector, and nothing.

Paolo

Re: [Qemu-devel] [PATCH 1/3] configure: add option to disable -fstack-protector flags

[Brad Smith]

On 09/01/14 5:40 PM, Paolo Bonzini wrote:
> Il 09/01/2014 22:55, Steven Noonan ha scritto:
>> From: Steven Noonan <snoonan@amazon.com>
>>
>> The -fstack-protector flag family is useful for ensuring safety and for
>> debugging, but has a performance impact. Here's a boot time comparison between
>> a QEMU build of qemu-system-arm with and without the -fstack-protector-all
>> flag:
>>
>>      # WITHOUT -fstack-protector-all
>>      [root@localhost ~]# systemd-analyze
>>      Startup finished in 1.744s (kernel) + 11.345s (initrd) + 47.164s (userspace) = 1min 255ms
>>
>>      # WITH -fstack-protector-all
>>      [root@localhost ~]# systemd-analyze
>>      Startup finished in 1.843s (kernel) + 12.262s (initrd) + 1min 3.480s (userspace) = 1min 17.587s
>
> Can you try -fstack-protector-strong?
>
> Probably the right thing to do is to pick in order
> -fstack-protector-strong, -fstack-protector, and nothing.

+1


-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [Qemu-devel] [PATCH 3/3] Makefile: add bios-256k.bin to BLOBS

[Peter Maydell]

On 9 January 2014 21:55, Steven Noonan <steven@uplinklabs.net> wrote:
> From: Steven Noonan <snoonan@amazon.com>
>
> Signed-off-by: Steven Noonan <snoonan@amazon.com>
> Cc: Anthony Liguori <aliguori@amazon.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index bdff4e4..807054b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -290,7 +290,7 @@ common  de-ch  es     fo  fr-ca  hu     ja  mk  nl-be      pt  sl     tr \
>  bepo    cz
>
>  ifdef INSTALL_BLOBS
> -BLOBS=bios.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
> +BLOBS=bios.bin bios-256k.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \

We already have this patch on list and reviewed:
http://patchwork.ozlabs.org/patch/299233/

Anthony should just apply that one :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] Makefile: add bios-256k.bin to BLOBS

[Noonan, Steven]

On Thu, Jan 09, 2014 at 11:27:47PM +0000, Peter Maydell wrote:
> On 9 January 2014 21:55, Steven Noonan <steven@uplinklabs.net> wrote:
> > From: Steven Noonan <snoonan@amazon.com>
> >
> > Signed-off-by: Steven Noonan <snoonan@amazon.com>
> > Cc: Anthony Liguori <aliguori@amazon.com>
> > ---
> >  Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index bdff4e4..807054b 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -290,7 +290,7 @@ common  de-ch  es     fo  fr-ca  hu     ja  mk  nl-be      pt  sl     tr \
> >  bepo    cz
> >
> >  ifdef INSTALL_BLOBS
> > -BLOBS=bios.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
> > +BLOBS=bios.bin bios-256k.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
> 
> We already have this patch on list and reviewed:
> http://patchwork.ozlabs.org/patch/299233/
> 
> Anthony should just apply that one :-)

Agreed. This is what happens when I don't search the list before writing
a patch. :)

Re: [Qemu-devel] [PATCH 1/3] configure: add option to disable -fstack-protector flags

[Noonan, Steven]

On Thu, Jan 09, 2014 at 06:18:07PM -0500, Brad Smith wrote:
> On 09/01/14 5:40 PM, Paolo Bonzini wrote:
> >Il 09/01/2014 22:55, Steven Noonan ha scritto:
> >>From: Steven Noonan <snoonan@amazon.com>
> >>
> >>The -fstack-protector flag family is useful for ensuring safety and for
> >>debugging, but has a performance impact. Here's a boot time comparison between
> >>a QEMU build of qemu-system-arm with and without the -fstack-protector-all
> >>flag:
> >>
> >>     # WITHOUT -fstack-protector-all
> >>     [root@localhost ~]# systemd-analyze
> >>     Startup finished in 1.744s (kernel) + 11.345s (initrd) + 47.164s (userspace) = 1min 255ms
> >>
> >>     # WITH -fstack-protector-all
> >>     [root@localhost ~]# systemd-analyze
> >>     Startup finished in 1.843s (kernel) + 12.262s (initrd) + 1min 3.480s (userspace) = 1min 17.587s
> >
> >Can you try -fstack-protector-strong?
> >
> >Probably the right thing to do is to pick in order
> >-fstack-protector-strong, -fstack-protector, and nothing.
> 
> +1
> 

I think there should still be an option to turn it off, but I agree that
there are probably better flags than -fstack-protector-all. I'll try
those out and post an update here probably later this evening.

Re: [Qemu-devel] [PATCH 3/3] Makefile: add bios-256k.bin to BLOBS

[Markus Armbruster]

"Noonan, Steven" <snoonan@amazon.com> writes:

> On Thu, Jan 09, 2014 at 11:27:47PM +0000, Peter Maydell wrote:
>> On 9 January 2014 21:55, Steven Noonan <steven@uplinklabs.net> wrote:
>> > From: Steven Noonan <snoonan@amazon.com>
>> >
>> > Signed-off-by: Steven Noonan <snoonan@amazon.com>
>> > Cc: Anthony Liguori <aliguori@amazon.com>
>> > ---
>> >  Makefile | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/Makefile b/Makefile
>> > index bdff4e4..807054b 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -290,7 +290,7 @@ common de-ch es fo fr-ca hu ja mk nl-be pt sl
>> > tr \
>> >  bepo    cz
>> >
>> >  ifdef INSTALL_BLOBS
>> > -BLOBS=bios.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
>> > +BLOBS=bios.bin bios-256k.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
>> 
>> We already have this patch on list and reviewed:
>> http://patchwork.ozlabs.org/patch/299233/
>> 
>> Anthony should just apply that one :-)
>
> Agreed. This is what happens when I don't search the list before writing
> a patch. :)

Actually, it's what happens when patches don't get applied in a timely
manner.

Re: [Qemu-devel] [PATCH 1/3] configure: add option to disable -fstack-protector flags

[Noonan, Steven]

[-- Attachment #1: Type: text/plain, Size: 3019 bytes --]

On Thu, Jan 09, 2014 at 06:18:07PM -0500, Brad Smith wrote:
> On 09/01/14 5:40 PM, Paolo Bonzini wrote:
> >Il 09/01/2014 22:55, Steven Noonan ha scritto:
> >>From: Steven Noonan <snoonan@amazon.com>
> >>
> >>The -fstack-protector flag family is useful for ensuring safety and for
> >>debugging, but has a performance impact. Here's a boot time comparison between
> >>a QEMU build of qemu-system-arm with and without the -fstack-protector-all
> >>flag:
> >>
> >>     # WITHOUT -fstack-protector-all
> >>     [root@localhost ~]# systemd-analyze
> >>     Startup finished in 1.744s (kernel) + 11.345s (initrd) + 47.164s (userspace) = 1min 255ms
> >>
> >>     # WITH -fstack-protector-all
> >>     [root@localhost ~]# systemd-analyze
> >>     Startup finished in 1.843s (kernel) + 12.262s (initrd) + 1min 3.480s (userspace) = 1min 17.587s
> >
> >Can you try -fstack-protector-strong?
> >
> >Probably the right thing to do is to pick in order
> >-fstack-protector-strong, -fstack-protector, and nothing.
> 
> +1
> 

OK, in order to get -fstack-protector-strong I had to get a Fedora box
up and running, so the compiler and build/execution environment are
different. I took three samples from each build, applying a clean 
QCOW2 snapshot before each run to avoid cross-boot confounding
variables:

    # -fstack-protector-all
    Startup finished in 1.810s (kernel) + 12.331s (initrd) + 49.016s (userspace) = 1min 3.159s
    Startup finished in 1.801s (kernel) + 12.287s (initrd) + 47.925s (userspace) = 1min 2.013s
    Startup finished in 1.812s (kernel) + 12.302s (initrd) + 47.995s (userspace) = 1min 2.111s
 
    # -fstack-protector-strong
    Startup finished in 1.744s (kernel) + 11.223s (initrd) + 44.688s (userspace) = 57.657s
    Startup finished in 1.721s (kernel) + 11.222s (initrd) + 44.194s (userspace) = 57.138s
    Startup finished in 1.693s (kernel) + 11.250s (initrd) + 44.426s (userspace) = 57.370s
    
    # -fstack-protector
    Startup finished in 1.705s (kernel) + 11.409s (initrd) + 43.563s (userspace) = 56.677s
    Startup finished in 1.877s (kernel) + 11.137s (initrd) + 43.719s (userspace) = 56.734s
    Startup finished in 1.708s (kernel) + 11.141s (initrd) + 43.628s (userspace) = 56.478s
    
    # no stack protector
    Startup finished in 1.743s (kernel) + 11.190s (initrd) + 43.709s (userspace) = 56.643s
    Startup finished in 1.763s (kernel) + 11.216s (initrd) + 43.767s (userspace) = 56.747s
    Startup finished in 1.711s (kernel) + 11.283s (initrd) + 43.878s (userspace) = 56.873s

There are a few points to note:

  - stack-protector-all doesn't cost nearly as much in this build, for
	reasons unclear to me. It looks like the kernel/initrd timings are
	rather closely reproduced, but the userspace timing varies quite a
	bit.

  - stack-protector-strong's performance hit is indeed much less than
	stack-protector-all.

  - The builds with the plain old -fstack-protector flag and without any
	stack protector are tied for the lead in terms of performance.

Amended patch attached.

[-- Attachment #2: 0001-configure-add-option-to-disable-fstack-protector-fla.patch --]
[-- Type: text/x-diff, Size: 3726 bytes --]

>From cad2b5990debede2bc8dc8552904b2ef9e14af22 Mon Sep 17 00:00:00 2001
From: Steven Noonan <snoonan@amazon.com>
Date: Mon, 6 Jan 2014 13:39:20 -0800
Subject: [PATCH] configure: add option to disable -fstack-protector flags

The -fstack-protector flag family is useful for ensuring safety and for
debugging, but has a performance impact. Here are some boot time comparisons of
the various versions of -fstack-protector using qemu-system-arm on an x86_64
host:

    # -fstack-protector-all
    Startup finished in 1.810s (kernel) + 12.331s (initrd) + 49.016s (userspace) = 1min 3.159s
    Startup finished in 1.801s (kernel) + 12.287s (initrd) + 47.925s (userspace) = 1min 2.013s
    Startup finished in 1.812s (kernel) + 12.302s (initrd) + 47.995s (userspace) = 1min 2.111s

    # -fstack-protector-strong
    Startup finished in 1.744s (kernel) + 11.223s (initrd) + 44.688s (userspace) = 57.657s
    Startup finished in 1.721s (kernel) + 11.222s (initrd) + 44.194s (userspace) = 57.138s
    Startup finished in 1.693s (kernel) + 11.250s (initrd) + 44.426s (userspace) = 57.370s

    # -fstack-protector
    Startup finished in 1.705s (kernel) + 11.409s (initrd) + 43.563s (userspace) = 56.677s
    Startup finished in 1.877s (kernel) + 11.137s (initrd) + 43.719s (userspace) = 56.734s
    Startup finished in 1.708s (kernel) + 11.141s (initrd) + 43.628s (userspace) = 56.478s

    # no stack protector
    Startup finished in 1.743s (kernel) + 11.190s (initrd) + 43.709s (userspace) = 56.643s
    Startup finished in 1.763s (kernel) + 11.216s (initrd) + 43.767s (userspace) = 56.747s
    Startup finished in 1.711s (kernel) + 11.283s (initrd) + 43.878s (userspace) = 56.873s

This patch introduces a configure option to disable the stack protector
entirely, and conditional stack protector flag selection (in order, based on
availability): -fstack-protector-strong, -fstack-protector, no stack protector.

Signed-off-by: Steven Noonan <snoonan@amazon.com>
---
 configure | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 07b6be3..a485e94 100755
--- a/configure
+++ b/configure
@@ -147,6 +147,7 @@ audio_win_int=""
 cc_i386=i386-pc-linux-gnu-gcc
 libs_qga=""
 debug_info="yes"
+stack_protector="yes"
 
 # Don't accept a target_list environment variable.
 unset target_list
@@ -879,6 +880,10 @@ for opt do
   ;;
   --disable-werror) werror="no"
   ;;
+  --enable-stack-protector) stack_protector="yes"
+  ;;
+  --disable-stack-protector) stack_protector="no"
+  ;;
   --disable-curses) curses="no"
   ;;
   --enable-curses) curses="yes"
@@ -1117,6 +1122,7 @@ echo "  --enable-sparse          enable sparse checker"
 echo "  --disable-sparse         disable sparse checker (default)"
 echo "  --disable-strip          disable stripping binaries"
 echo "  --disable-werror         disable compilation abort on warning"
+echo "  --disable-stack-protector disable GCC-provided stack protection"
 echo "  --disable-sdl            disable SDL"
 echo "  --enable-sdl             enable SDL"
 echo "  --disable-gtk            disable gtk UI"
@@ -1298,9 +1304,15 @@ for flag in $gcc_flags; do
     fi
 done
 
-if compile_prog "-Werror -fstack-protector-all" "" ; then
-    QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
-    LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
+if test "$stack_protector" = "yes" ; then
+  gcc_flags="-fstack-protector-strong -fstack-protector"
+  for flag in $gcc_flags; do
+    if compile_prog "-Werror $flag" "" ; then
+      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
+      break
+    fi
+  done
 fi
 
 # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
-- 
1.8.5.2

Re: [Qemu-devel] [PATCH 1/3] configure: add option to disable -fstack-protector flags

[Stefan Weil]

Hi Steven,

--disable-stack-protector would also be useful for platforms which make
debugging of executables with stack protection difficult. When I must
debug Windows executables, I always disable stack protection, because
otherwise the stack back traces are unreadable.

So, for MinGW it might be reasonable to set the default to no stack
protection if --enable-debug is selected. This requires some
modifications in your patch.

# Don't set it to "yes" initially:
stack_protector=""

# Do the compile test if it is not "no":
if test "$stack_protector" != "no"; then

The usual logic is do nothing if the user says "no". Run the compile
tests otherwise. Show an error message if the compile tests fail and the
user said "yes". See the code which handles $pie for an example.

Please send your next patch inline - this makes it easier to add comments.

Best regards

Stefan

Re: [Qemu-devel] [PATCH 1/3] configure: add option to disable -fstack-protector flags

[Paolo Bonzini]

Il 11/01/2014 08:46, Stefan Weil ha scritto:
> Hi Steven,
> 
> --disable-stack-protector would also be useful for platforms which make
> debugging of executables with stack protection difficult. When I must
> debug Windows executables, I always disable stack protection, because
> otherwise the stack back traces are unreadable.
> 
> So, for MinGW it might be reasonable to set the default to no stack
> protection if --enable-debug is selected. This requires some
> modifications in your patch.
> 
> # Don't set it to "yes" initially:
> stack_protector=""
> 
> # Do the compile test if it is not "no":
> if test "$stack_protector" != "no"; then

Apart from this little detail, the patch looks good.

Steven, please resend the patch as a top-level message with the patch
inline rather than attached.  This is needed so that Anthony's script
will pick it up.  Including these changes would be nice too.

Paolo

> The usual logic is do nothing if the user says "no". Run the compile
> tests otherwise. Show an error message if the compile tests fail and the
> user said "yes". See the code which handles $pie for an example.
> 
> Please send your next patch inline - this makes it easier to add comments.
> 
> Best regards
> 
> Stefan
> 

[Qemu-devel] [PATCH] configure: add option to disable -fstack-protector flags

[Steven Noonan]

The -fstack-protector flag family is useful for ensuring safety and for
debugging, but has a performance impact. Here are some boot time comparisons of
the various versions of -fstack-protector using qemu-system-arm on an x86_64
host:

    # -fstack-protector-all
    Startup finished in 1.810s (kernel) + 12.331s (initrd) + 49.016s (userspace) = 1min 3.159s
    Startup finished in 1.801s (kernel) + 12.287s (initrd) + 47.925s (userspace) = 1min 2.013s
    Startup finished in 1.812s (kernel) + 12.302s (initrd) + 47.995s (userspace) = 1min 2.111s

    # -fstack-protector-strong
    Startup finished in 1.744s (kernel) + 11.223s (initrd) + 44.688s (userspace) = 57.657s
    Startup finished in 1.721s (kernel) + 11.222s (initrd) + 44.194s (userspace) = 57.138s
    Startup finished in 1.693s (kernel) + 11.250s (initrd) + 44.426s (userspace) = 57.370s

    # -fstack-protector
    Startup finished in 1.705s (kernel) + 11.409s (initrd) + 43.563s (userspace) = 56.677s
    Startup finished in 1.877s (kernel) + 11.137s (initrd) + 43.719s (userspace) = 56.734s
    Startup finished in 1.708s (kernel) + 11.141s (initrd) + 43.628s (userspace) = 56.478s

    # no stack protector
    Startup finished in 1.743s (kernel) + 11.190s (initrd) + 43.709s (userspace) = 56.643s
    Startup finished in 1.763s (kernel) + 11.216s (initrd) + 43.767s (userspace) = 56.747s
    Startup finished in 1.711s (kernel) + 11.283s (initrd) + 43.878s (userspace) = 56.873s

This patch introduces a configure option to disable the stack protector
entirely, and conditional stack protector flag selection (in order, based on
availability): -fstack-protector-strong, -fstack-protector, no stack protector.

Signed-off-by: Steven Noonan <snoonan@amazon.com>
---
 configure | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 07b6be3..c9c0b2c 100755
--- a/configure
+++ b/configure
@@ -147,6 +147,7 @@ audio_win_int=""
 cc_i386=i386-pc-linux-gnu-gcc
 libs_qga=""
 debug_info="yes"
+stack_protector=""
 
 # Don't accept a target_list environment variable.
 unset target_list
@@ -879,6 +880,10 @@ for opt do
   ;;
   --disable-werror) werror="no"
   ;;
+  --enable-stack-protector) stack_protector="yes"
+  ;;
+  --disable-stack-protector) stack_protector="no"
+  ;;
   --disable-curses) curses="no"
   ;;
   --enable-curses) curses="yes"
@@ -1117,6 +1122,7 @@ echo "  --enable-sparse          enable sparse checker"
 echo "  --disable-sparse         disable sparse checker (default)"
 echo "  --disable-strip          disable stripping binaries"
 echo "  --disable-werror         disable compilation abort on warning"
+echo "  --disable-stack-protector disable GCC-provided stack protection"
 echo "  --disable-sdl            disable SDL"
 echo "  --enable-sdl             enable SDL"
 echo "  --disable-gtk            disable gtk UI"
@@ -1298,9 +1304,15 @@ for flag in $gcc_flags; do
     fi
 done
 
-if compile_prog "-Werror -fstack-protector-all" "" ; then
-    QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
-    LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
+if test "$stack_protector" != "no" ; then
+  gcc_flags="-fstack-protector-strong -fstack-protector"
+  for flag in $gcc_flags; do
+    if compile_prog "-Werror $flag" "" ; then
+      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
+      break
+    fi
+  done
 fi
 
 # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
-- 
1.8.5.2

Re: [Qemu-devel] [PATCH] configure: add option to disable -fstack-protector flags

[Stefan Weil]

Hi,

please see comments below.

Am 13.01.2014 21:00, schrieb Steven Noonan:
> The -fstack-protector flag family is useful for ensuring safety and for
> debugging, but has a performance impact. Here are some boot time comparisons of
> the various versions of -fstack-protector using qemu-system-arm on an x86_64
> host:
> 
>     # -fstack-protector-all
>     Startup finished in 1.810s (kernel) + 12.331s (initrd) + 49.016s (userspace) = 1min 3.159s
>     Startup finished in 1.801s (kernel) + 12.287s (initrd) + 47.925s (userspace) = 1min 2.013s
>     Startup finished in 1.812s (kernel) + 12.302s (initrd) + 47.995s (userspace) = 1min 2.111s
> 
>     # -fstack-protector-strong
>     Startup finished in 1.744s (kernel) + 11.223s (initrd) + 44.688s (userspace) = 57.657s
>     Startup finished in 1.721s (kernel) + 11.222s (initrd) + 44.194s (userspace) = 57.138s
>     Startup finished in 1.693s (kernel) + 11.250s (initrd) + 44.426s (userspace) = 57.370s
> 
>     # -fstack-protector
>     Startup finished in 1.705s (kernel) + 11.409s (initrd) + 43.563s (userspace) = 56.677s
>     Startup finished in 1.877s (kernel) + 11.137s (initrd) + 43.719s (userspace) = 56.734s
>     Startup finished in 1.708s (kernel) + 11.141s (initrd) + 43.628s (userspace) = 56.478s
> 
>     # no stack protector
>     Startup finished in 1.743s (kernel) + 11.190s (initrd) + 43.709s (userspace) = 56.643s
>     Startup finished in 1.763s (kernel) + 11.216s (initrd) + 43.767s (userspace) = 56.747s
>     Startup finished in 1.711s (kernel) + 11.283s (initrd) + 43.878s (userspace) = 56.873s
> 
> This patch introduces a configure option to disable the stack protector
> entirely, and conditional stack protector flag selection (in order, based on
> availability): -fstack-protector-strong, -fstack-protector, no stack protector.
> 
> Signed-off-by: Steven Noonan <snoonan@amazon.com>
> ---
>  configure | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 07b6be3..c9c0b2c 100755
> --- a/configure
> +++ b/configure
> @@ -147,6 +147,7 @@ audio_win_int=""
>  cc_i386=i386-pc-linux-gnu-gcc
>  libs_qga=""
>  debug_info="yes"
> +stack_protector=""
>  
>  # Don't accept a target_list environment variable.
>  unset target_list
> @@ -879,6 +880,10 @@ for opt do
>    ;;
>    --disable-werror) werror="no"
>    ;;
> +  --enable-stack-protector) stack_protector="yes"
> +  ;;
> +  --disable-stack-protector) stack_protector="no"
> +  ;;
>    --disable-curses) curses="no"
>    ;;
>    --enable-curses) curses="yes"
> @@ -1117,6 +1122,7 @@ echo "  --enable-sparse          enable sparse checker"
>  echo "  --disable-sparse         disable sparse checker (default)"
>  echo "  --disable-strip          disable stripping binaries"
>  echo "  --disable-werror         disable compilation abort on warning"
> +echo "  --disable-stack-protector disable GCC-provided stack protection"


Clang also supports stack protection AFAIK, so "GCC-provided" can be
removed here (or replaced by "compiler").


>  echo "  --disable-sdl            disable SDL"
>  echo "  --enable-sdl             enable SDL"
>  echo "  --disable-gtk            disable gtk UI"
> @@ -1298,9 +1304,15 @@ for flag in $gcc_flags; do
>      fi
>  done
>  
> -if compile_prog "-Werror -fstack-protector-all" "" ; then
> -    QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
> -    LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
> +if test "$stack_protector" != "no" ; then
> +  gcc_flags="-fstack-protector-strong -fstack-protector"
> +  for flag in $gcc_flags; do
> +    if compile_prog "-Werror $flag" "" ; then
> +      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
> +      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
> +      break
> +    fi
> +  done
>  fi
>  
>  # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
> 


Reviewed-by: Stefan Weil <sw@weilnetz.de>

I think this patch can be used as a base for further improvements (MinGW
specific settings, error handling when user's choice does not work).

Maybe you will have to resend the patch as a top level patch (don't use
the reply function of your mailer). As Paolo said, Anthony might
overwise be unable to pick it up with his scripts.

Regards
Stefan

Re: [Qemu-devel] [PATCH] configure: add option to disable -fstack-protector flags

[Noonan, Steven]

On Mon, Jan 13, 2014 at 09:27:42PM +0100, Stefan Weil wrote:
> Hi,
> 
> please see comments below.
> 
> Am 13.01.2014 21:00, schrieb Steven Noonan:
> > The -fstack-protector flag family is useful for ensuring safety and for
> > debugging, but has a performance impact. Here are some boot time comparisons of
> > the various versions of -fstack-protector using qemu-system-arm on an x86_64
> > host:
> > 
> >     # -fstack-protector-all
> >     Startup finished in 1.810s (kernel) + 12.331s (initrd) + 49.016s (userspace) = 1min 3.159s
> >     Startup finished in 1.801s (kernel) + 12.287s (initrd) + 47.925s (userspace) = 1min 2.013s
> >     Startup finished in 1.812s (kernel) + 12.302s (initrd) + 47.995s (userspace) = 1min 2.111s
> > 
> >     # -fstack-protector-strong
> >     Startup finished in 1.744s (kernel) + 11.223s (initrd) + 44.688s (userspace) = 57.657s
> >     Startup finished in 1.721s (kernel) + 11.222s (initrd) + 44.194s (userspace) = 57.138s
> >     Startup finished in 1.693s (kernel) + 11.250s (initrd) + 44.426s (userspace) = 57.370s
> > 
> >     # -fstack-protector
> >     Startup finished in 1.705s (kernel) + 11.409s (initrd) + 43.563s (userspace) = 56.677s
> >     Startup finished in 1.877s (kernel) + 11.137s (initrd) + 43.719s (userspace) = 56.734s
> >     Startup finished in 1.708s (kernel) + 11.141s (initrd) + 43.628s (userspace) = 56.478s
> > 
> >     # no stack protector
> >     Startup finished in 1.743s (kernel) + 11.190s (initrd) + 43.709s (userspace) = 56.643s
> >     Startup finished in 1.763s (kernel) + 11.216s (initrd) + 43.767s (userspace) = 56.747s
> >     Startup finished in 1.711s (kernel) + 11.283s (initrd) + 43.878s (userspace) = 56.873s
> > 
> > This patch introduces a configure option to disable the stack protector
> > entirely, and conditional stack protector flag selection (in order, based on
> > availability): -fstack-protector-strong, -fstack-protector, no stack protector.
> > 
> > Signed-off-by: Steven Noonan <snoonan@amazon.com>
> > ---
> >  configure | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/configure b/configure
> > index 07b6be3..c9c0b2c 100755
> > --- a/configure
> > +++ b/configure
> > @@ -147,6 +147,7 @@ audio_win_int=""
> >  cc_i386=i386-pc-linux-gnu-gcc
> >  libs_qga=""
> >  debug_info="yes"
> > +stack_protector=""
> >  
> >  # Don't accept a target_list environment variable.
> >  unset target_list
> > @@ -879,6 +880,10 @@ for opt do
> >    ;;
> >    --disable-werror) werror="no"
> >    ;;
> > +  --enable-stack-protector) stack_protector="yes"
> > +  ;;
> > +  --disable-stack-protector) stack_protector="no"
> > +  ;;
> >    --disable-curses) curses="no"
> >    ;;
> >    --enable-curses) curses="yes"
> > @@ -1117,6 +1122,7 @@ echo "  --enable-sparse          enable sparse checker"
> >  echo "  --disable-sparse         disable sparse checker (default)"
> >  echo "  --disable-strip          disable stripping binaries"
> >  echo "  --disable-werror         disable compilation abort on warning"
> > +echo "  --disable-stack-protector disable GCC-provided stack protection"
> 
> 
> Clang also supports stack protection AFAIK, so "GCC-provided" can be
> removed here (or replaced by "compiler").
> 

Fixed in v3 of the patch.

> 
> >  echo "  --disable-sdl            disable SDL"
> >  echo "  --enable-sdl             enable SDL"
> >  echo "  --disable-gtk            disable gtk UI"
> > @@ -1298,9 +1304,15 @@ for flag in $gcc_flags; do
> >      fi
> >  done
> >  
> > -if compile_prog "-Werror -fstack-protector-all" "" ; then
> > -    QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
> > -    LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
> > +if test "$stack_protector" != "no" ; then
> > +  gcc_flags="-fstack-protector-strong -fstack-protector"
> > +  for flag in $gcc_flags; do
> > +    if compile_prog "-Werror $flag" "" ; then
> > +      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
> > +      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
> > +      break
> > +    fi
> > +  done
> >  fi
> >  
> >  # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
> > 
> 
> 
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> 
> I think this patch can be used as a base for further improvements (MinGW
> specific settings, error handling when user's choice does not work).
> 
> Maybe you will have to resend the patch as a top level patch (don't use
> the reply function of your mailer). As Paolo said, Anthony might
> overwise be unable to pick it up with his scripts.

Thanks, I've done that now.

- Steven