[PATCH liburing v1] src/Makefile: Don't use stack protector for all builds by default

[PATCH liburing v1] src/Makefile: Don't use stack protector for all builds by default

[Ammar Faizi]

Stack protector adds extra mov, extra stack allocation and extra branch
to save and validate the stack canary. While this feature could be
useful to detect stack corruption in some scenarios, it is not really
needed for liburing which is simple enough to review.

Good code shouldn't corrupt the stack. We don't need this extra
checking at the moment. Just for comparison, let's take a hot function
__io_uring_get_cqe.

Before this patch:
```
0000000000002b80 <__io_uring_get_cqe>:
  2b80:   f3 0f 1e fa             endbr64
  2b84:   48 83 ec 28             sub    $0x28,%rsp
  2b88:   64 48 8b 04 25 28 00    mov    %fs:0x28,%rax
  2b8f:   00 00
  2b91:   48 89 44 24 18          mov    %rax,0x18(%rsp)
  2b96:   31 c0                   xor    %eax,%eax
  2b98:   89 14 24                mov    %edx,(%rsp)
  2b9b:   48 89 e2                mov    %rsp,%rdx
  2b9e:   48 b8 00 00 00 00 08    movabs $0x800000000,%rax
  2ba5:   00 00 00
  2ba8:   89 4c 24 04             mov    %ecx,0x4(%rsp)
  2bac:   48 89 44 24 08          mov    %rax,0x8(%rsp)
  2bb1:   4c 89 44 24 10          mov    %r8,0x10(%rsp)
  2bb6:   e8 45 fe ff ff          call   2a00 <_io_uring_get_cqe>
  2bbb:   48 8b 54 24 18          mov    0x18(%rsp),%rdx
  2bc0:   64 48 2b 14 25 28 00    sub    %fs:0x28,%rdx
  2bc7:   00 00
  2bc9:   75 05                   jne    2bd0 <__io_uring_get_cqe+0x50>
  2bcb:   48 83 c4 28             add    $0x28,%rsp
  2bcf:   c3                      ret
  2bd0:   e8 9b f5 ff ff          call   2170 <__stack_chk_fail@plt>
  2bd5:   66 66 2e 0f 1f 84 00    data16 cs nopw 0x0(%rax,%rax,1)
  2bdc:   00 00 00 00
```

After this patch:
```
0000000000002ab0 <__io_uring_get_cqe>:
  2ab0:   f3 0f 1e fa             endbr64
  2ab4:   48 b8 00 00 00 00 08    movabs $0x800000000,%rax
  2abb:   00 00 00
  2abe:   48 83 ec 28             sub    $0x28,%rsp
  2ac2:   89 14 24                mov    %edx,(%rsp)
  2ac5:   48 89 e2                mov    %rsp,%rdx
  2ac8:   89 4c 24 04             mov    %ecx,0x4(%rsp)
  2acc:   48 89 44 24 08          mov    %rax,0x8(%rsp)
  2ad1:   4c 89 44 24 10          mov    %r8,0x10(%rsp)
  2ad6:   e8 55 fe ff ff          call   2930 <_io_uring_get_cqe>
  2adb:   48 83 c4 28             add    $0x28,%rsp
  2adf:   c3                      ret
```

Previously, we only use `-fno-stack-protector` for nolibc build as the
stack protector needs to call `__stack_chk_fail@plt` function from the
libc. Now, we always use `-fno-stack-protector` for both nolibc and
libc builds to generate shorter Assembly code.

Cc: Nugra <richiisei@gmail.com>
Cc: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>
Cc: GNU/Weeb Mailing List <gwml@vger.gnuweeb.org>
Cc: Tea Inside Mailing List <timl@vger.teainside.org>
Cc: io-uring Mailing List <io-uring@vger.kernel.org>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 src/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/Makefile b/src/Makefile
index cc6c871..3e1192f 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -8,7 +8,7 @@ libdevdir ?= $(prefix)/lib
 CPPFLAGS ?=
 override CPPFLAGS += -D_GNU_SOURCE \
 	-Iinclude/ -include ../config-host.h
-CFLAGS ?= -g -fomit-frame-pointer -O2 -Wall -Wextra
+CFLAGS ?= -g -fomit-frame-pointer -O2 -Wall -Wextra -fno-stack-protector
 override CFLAGS += -Wno-unused-parameter -Wno-sign-compare -DLIBURING_INTERNAL
 SO_CFLAGS=-fPIC $(CFLAGS)
 L_CFLAGS=$(CFLAGS)
@@ -36,8 +36,8 @@ liburing_srcs := setup.c queue.c register.c
 
 ifeq ($(CONFIG_NOLIBC),y)
 	liburing_srcs += nolibc.c
-	override CFLAGS += -nostdlib -nodefaultlibs -ffreestanding -fno-stack-protector
-	override CPPFLAGS += -nostdlib -nodefaultlibs -ffreestanding -fno-stack-protector
+	override CFLAGS += -nostdlib -nodefaultlibs -ffreestanding
+	override CPPFLAGS += -nostdlib -nodefaultlibs -ffreestanding
 	override LINK_FLAGS += -nostdlib -nodefaultlibs
 else
 	liburing_srcs += syscall.c

base-commit: 896a1d3ab14a8777a45db6e7b67cf557a44923fb
-- 
2.32.0

Re: [PATCH liburing v1] src/Makefile: Don't use stack protector for all builds by default

[Alviro Iskandar Setiawan]

On Fri, Feb 25, 2022 at 5:25 AM Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote:
> Stack protector adds extra mov, extra stack allocation and extra branch
> to save and validate the stack canary. While this feature could be
> useful to detect stack corruption in some scenarios, it is not really
> needed for liburing which is simple enough to review.
>
> Good code shouldn't corrupt the stack. We don't need this extra
> checking at the moment. Just for comparison, let's take a hot function
> __io_uring_get_cqe.

Yes, I don't see any harm in removing the stack protector here.

Reviewed-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>

-- Viro

Re: [PATCH liburing v1] src/Makefile: Don't use stack protector for all builds by default

[Jens Axboe]

On Fri, 25 Feb 2022 05:24:27 +0700, Ammar Faizi wrote:
> Stack protector adds extra mov, extra stack allocation and extra branch
> to save and validate the stack canary. While this feature could be
> useful to detect stack corruption in some scenarios, it is not really
> needed for liburing which is simple enough to review.
> 
> Good code shouldn't corrupt the stack. We don't need this extra
> checking at the moment. Just for comparison, let's take a hot function
> __io_uring_get_cqe.
> 
> [...]

Applied, thanks!

[1/1] src/Makefile: Don't use stack protector for all builds by default
      (no commit info)

Best regards,
-- 
Jens Axboe