x86: Rewrite 64-bit syscall code

There aren't any yet, but there might be a few some day.

Signed-off-by Andy Lutomirski <[email protected]>

---
 tools/testing/selftests/x86/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

tools/testing/selftests/x86/Makefile +2 -1

index 389701f59940..a460fe7c5365 100644
@@ -8,8 +8,9 @@ TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs ldt_gdt syscall_nt ptr
 TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault sigreturn test_syscall_vdso unwind_vdso
 
 TARGETS_C_32BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_32BIT_ONLY)
+TARGETS_C_64BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_64BIT_ONLY)
 BINARIES_32 := $(TARGETS_C_32BIT_ALL:%=%_32)
-BINARIES_64 := $(TARGETS_C_BOTHBITS:%=%_64)
+BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64)
 
 CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
 

Quoted 23 lines · Andy Lutomirski

There aren't any yet, but there might be a few some day.

Signed-off-by Andy Lutomirski <[email protected]>

---
 tools/testing/selftests/x86/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

tools/testing/selftests/x86/Makefile +2 -1

index 389701f59940..a460fe7c5365 100644
@@ -8,8 +8,9 @@ TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs ldt_gdt syscall_nt ptr
 TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault sigreturn test_syscall_vdso unwind_vdso
 
 TARGETS_C_32BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_32BIT_ONLY)
+TARGETS_C_64BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_64BIT_ONLY)
 BINARIES_32 := $(TARGETS_C_32BIT_ALL:%=%_32)
-BINARIES_64 := $(TARGETS_C_BOTHBITS:%=%_64)
+BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64)
 
 CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
 

tools/testing/selftests/x86/Makefile clean:

Quoted 9 lines

tools/testing/selftests/x86/Makefile +1 -2

@@ -37,7 +38,7 @@ clean:
 $(TARGETS_C_32BIT_ALL:%=%_32): %_32: %.c
      $(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
 
-$(TARGETS_C_BOTHBITS:%=%_64): %_64: %.c
+$(TARGETS_C_64BIT_ALL:%=%_64): %_64: %.c
      $(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
 
 # x86_64 users should be encouraged to install 32-bit libraries
--

It doesn't build some of the tests here if I run make in the x86 dir. This is unrelated but maybe for the future we should add some feature testing like perf tool does to warn people if stuff is missing on the system...

$ cd tools/testing/selftests/x86
$ make
gcc -m32 -o single_step_syscall_32 -O2 -g -std=gnu99 -pthread -Wall  single_step_syscall.c -lrt -ldl -lm
gcc -m32 -o sysret_ss_attrs_32 -O2 -g -std=gnu99 -pthread -Wall  sysret_ss_attrs.c -lrt -ldl -lm
gcc -m32 -o ldt_gdt_32 -O2 -g -std=gnu99 -pthread -Wall  ldt_gdt.c -lrt -ldl -lm
gcc -m32 -o syscall_nt_32 -O2 -g -std=gnu99 -pthread -Wall  syscall_nt.c -lrt -ldl -lm
gcc -m32 -o ptrace_syscall_32 -O2 -g -std=gnu99 -pthread -Wall  ptrace_syscall.c raw_syscall_helper_32.S -lrt -ldl -lm
gcc -m32 -o entry_from_vm86_32 -O2 -g -std=gnu99 -pthread -Wall  entry_from_vm86.c -lrt -ldl -lm
gcc -m32 -o syscall_arg_fault_32 -O2 -g -std=gnu99 -pthread -Wall  syscall_arg_fault.c -lrt -ldl -lm

gcc -m32 -o sigreturn_32 -O2 -g -std=gnu99 -pthread -Wall  sigreturn.c -lrt -ldl -lm
gcc -m32 -o test_syscall_vdso_32 -O2 -g -std=gnu99 -pthread -Wall  test_syscall_vdso.c thunks_32.S -lrt -ldl -lm
In file included from ptrace_syscall.c:6:0:
/usr/include/sys/syscall.h:24:24: fatal error: asm/unistd.h: No such file or directory
compilation terminated.
In file included from entry_from_vm86.c:14:0:
/usr/include/sys/syscall.h:24:24: fatal error: asm/unistd.h: No such file or directory
compilation terminated.

There aren't any yet, but there might be a few some day.

Signed-off-by Andy Lutomirski <[email protected]>

---
 tools/testing/selftests/x86/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

tools/testing/selftests/x86/Makefile +3 -3

index 389701f59940..a460fe7c5365 100644
@@ -8,8 +8,9 @@ TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs ldt_gdt syscall_nt ptr
 TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault sigreturn test_syscall_vdso unwind_vdso
 
 TARGETS_C_32BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_32BIT_ONLY)
+TARGETS_C_64BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_64BIT_ONLY)
 BINARIES_32 := $(TARGETS_C_32BIT_ALL:%=%_32)
-BINARIES_64 := $(TARGETS_C_BOTHBITS:%=%_64)
+BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64)
 
 CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
 

@@ -37,7 +38,7 @@ clean:
 $(TARGETS_C_32BIT_ALL:%=%_32): %_32: %.c
        $(CC) -m32 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
 
-$(TARGETS_C_BOTHBITS:%=%_64): %_64: %.c
+$(TARGETS_C_64BIT_ALL:%=%_64): %_64: %.c
        $(CC) -m64 -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
 
 # x86_64 users should be encouraged to install 32-bit libraries
--

2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 1 line · Andy Lutomirski

There aren't any yet, but there might be a few some day.

Andy,

Hmm. I would think get_maintainer script should have included linux-api and as well as my email for this patch.

Anyway, I would like to see a better worded changelog. Something along the lines

Makefile changes to enable x86_64 tests.

Quoted 3 lines · Shuah Khan

Quoted 1 line · Andy Lutomirski

There aren't any yet, but there might be a few some day.

Quoted 4 lines

Andy,

Hmm. I would think get_maintainer script should have included linux-api and as well as my email for this patch.

Whoops, my bad.

Although... isn't it about time that selftests got its own list?

Quoted 5 lines

Anyway, I would like to see a better worded changelog. Something along the lines

Makefile changes to enable x86_64 tests.

I find that confusing. The Makefile already supports x86_64 tests. What's missing is support for tests that are x86_64 but not x86_32.

This checks that ELF binaries are started with an appropriately blank register state.

(There's currently a nasty special case in the entry asm to arrange
for this. I'm planning on removing the special case, and this will help make sure I don't break it.)

Signed-off-by Andy Lutomirski <[email protected]>

---
 tools/testing/selftests/x86/Makefile               |   8 +-
 .../selftests/x86/check_initial_reg_state.c        | 108 +++++++++++++++++++++
 2 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/x86/check_initial_reg_state.c

tools/testing/selftests/x86/Makefile +7 -1

index a460fe7c5365..b82409421fa6 100644
@@ -4,7 +4,7 @@ include ../lib.mk
 
 .PHONY: all all_32 all_64 warn_32bit_failure clean
 
-TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs ldt_gdt syscall_nt ptrace_syscall
+TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs ldt_gdt syscall_nt ptrace_syscall check_initial_reg_state
 TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault sigreturn test_syscall_vdso unwind_vdso
 
 TARGETS_C_32BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_32BIT_ONLY)

@@ -63,3 +63,9 @@ endif
 sysret_ss_attrs_64: thunks.S
 ptrace_syscall_32: raw_syscall_helper_32.S
 test_syscall_vdso_32: thunks_32.S
+
+# check_initial_reg_state is special: it needs a custom entry, and it
+# needs to be static so that its interpreter doesn't destroy its initial
+# state.
+check_initial_reg_state_32: CFLAGS += -Wl,-ereal_start -static
+check_initial_reg_state_64: CFLAGS += -Wl,-ereal_start -static

tools/testing/selftests/x86/check_initial_reg_state.c new file +12 -0

new file mode 100644
index 000000000000..0cb565f7786d
@@ -0,0 +1,108 @@
+/*
+ * check_initial_reg_state.c - check that execve sets the correct state
+ * Copyright (c) 2014-2015 Andrew Lutomirski
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.

Quoted 55 lines · Andy Lutomirski

This checks that ELF binaries are started with an appropriately blank register state.

(There's currently a nasty special case in the entry asm to arrange
for this. I'm planning on removing the special case, and this will help make sure I don't break it.)

Signed-off-by Andy Lutomirski <[email protected]>

---
 tools/testing/selftests/x86/Makefile               |   8 +-
 .../selftests/x86/check_initial_reg_state.c        | 108 +++++++++++++++++++++
 2 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/x86/check_initial_reg_state.c

tools/testing/selftests/x86/Makefile +7 -1

index a460fe7c5365..b82409421fa6 100644
@@ -4,7 +4,7 @@ include ../lib.mk
 
 .PHONY: all all_32 all_64 warn_32bit_failure clean
 
-TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs ldt_gdt syscall_nt ptrace_syscall
+TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs ldt_gdt syscall_nt ptrace_syscall check_initial_reg_state
 TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault sigreturn test_syscall_vdso unwind_vdso
 
 TARGETS_C_32BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_32BIT_ONLY)

@@ -63,3 +63,9 @@ endif
 sysret_ss_attrs_64: thunks.S
 ptrace_syscall_32: raw_syscall_helper_32.S
 test_syscall_vdso_32: thunks_32.S
+
+# check_initial_reg_state is special: it needs a custom entry, and it
+# needs to be static so that its interpreter doesn't destroy its initial
+# state.
+check_initial_reg_state_32: CFLAGS += -Wl,-ereal_start -static
+check_initial_reg_state_64: CFLAGS += -Wl,-ereal_start -static

tools/testing/selftests/x86/check_initial_reg_state.c new file +12 -0

new file mode 100644
index 000000000000..0cb565f7786d
@@ -0,0 +1,108 @@
+/*
+ * check_initial_reg_state.c - check that execve sets the correct state
+ * Copyright (c) 2014-2015 Andrew Lutomirski
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.

tools/testing/selftests/x86/check_initial_reg_state.c line 22

Quoted 10 lines

tools/testing/selftests/x86/check_initial_reg_state.c new file +10 -0

+ */
+
+#define _GNU_SOURCE
+
+#include <stdio.h>
+
+unsigned long ax, bx, cx, dx, si, di, bp, sp, flags;
+unsigned long r8, r9, r10, r11, r12, r13, r14, r15;
+
+asm (".pushsection .text\n\t"

WARNING: please, no spaces at the start of a line
#82: FILE: tools/testing/selftests/x86/check_initial_reg_state.c:23:
+ ".type real_start, @function\n\t"$

Something trampled over the preceding tabs in that whole asm().

That was intentional -- everything lines up with the top-level "asm(". checkpatch is confused because it understands "\t " at the front of a line but not just a space.

That being said, I could easily be convinced to switch to tabs there.

Quoted 5 lines · Andy Lutomirski

That was intentional -- everything lines up with the top-level "asm(". checkpatch is confused because it understands "\t " at the front of a line but not just a space.

That being said, I could easily be convinced to switch to tabs there.

I'd say kernel coding style is tabs of 8 chars...

emacs, the One True Reference Of Kernel Style, disagrees with you :)

64-bit syscalls currently have an optimization in which they are called with partial pt_regs. A small handful require full pt_regs.

In the 32-bit and compat cases, I cleaned this up by forcing full pt_regs for all syscalls. The performance hit doesn't really matter.

I want to clean up the 64-bit case as well, but I don't want to hurt fast path performance. To do that, I want to force the syscalls that use pt_regs onto the slow path. This will enable us to make slow path syscalls be real ABI-compliant C functions.

Use the new syscall entry qualification machinery for this. stub_clone is now stub_clone/ptregs.

The next patch will eliminate the stubs, and we'll just have sys_clone/ptregs.

Quoted 11 lines · Andy Lutomirski

64-bit syscalls currently have an optimization in which they are called with partial pt_regs. A small handful require full pt_regs.

In the 32-bit and compat cases, I cleaned this up by forcing full pt_regs for all syscalls. The performance hit doesn't really matter.

I want to clean up the 64-bit case as well, but I don't want to hurt fast path performance. To do that, I want to force the syscalls that use pt_regs onto the slow path. This will enable us to make slow path syscalls be real ABI-compliant C functions.

Quoted 5 lines

Use the new syscall entry qualification machinery for this. stub_clone is now stub_clone/ptregs.

The next patch will eliminate the stubs, and we'll just have sys_clone/ptregs.

Quoted 12 lines · Brian Gerst

Quoted 11 lines · Andy Lutomirski

64-bit syscalls currently have an optimization in which they are called with partial pt_regs. A small handful require full pt_regs.

In the 32-bit and compat cases, I cleaned this up by forcing full pt_regs for all syscalls. The performance hit doesn't really matter.

I want to clean up the 64-bit case as well, but I don't want to hurt fast path performance. To do that, I want to force the syscalls that use pt_regs onto the slow path. This will enable us to make slow path syscalls be real ABI-compliant C functions.

Quoted 5 lines

Quoted 5 lines

Use the new syscall entry qualification machinery for this. stub_clone is now stub_clone/ptregs.

The next patch will eliminate the stubs, and we'll just have sys_clone/ptregs.

[Resend after gmail web interface fail]

I've got an idea on how to do this without the duplicate syscall table.

ptregs_foo:
leaq sys_foo(%rip), %rax
jmp stub_ptregs_64

stub_ptregs_64:

    testl $TS_EXTRAREGS, <current->ti_status>
    jnz 1f
    SAVE_EXTRA_REGS
    call *%rax
    RESTORE_EXTRA_REGS
    ret

1:
call *%rax
ret

This makes sure that the extra regs don't get saved a second time if coming in from the slow path, but preserves the fast path if not tracing.

Quoted 34 lines · Brian Gerst

Quoted 12 lines · Brian Gerst

Quoted 11 lines · Andy Lutomirski

64-bit syscalls currently have an optimization in which they are called with partial pt_regs. A small handful require full pt_regs.

In the 32-bit and compat cases, I cleaned this up by forcing full pt_regs for all syscalls. The performance hit doesn't really matter.

I want to clean up the 64-bit case as well, but I don't want to hurt fast path performance. To do that, I want to force the syscalls that use pt_regs onto the slow path. This will enable us to make slow path syscalls be real ABI-compliant C functions.

Quoted 5 lines

Quoted 5 lines

Use the new syscall entry qualification machinery for this. stub_clone is now stub_clone/ptregs.

The next patch will eliminate the stubs, and we'll just have sys_clone/ptregs.

[Resend after gmail web interface fail]

I've got an idea on how to do this without the duplicate syscall table.

ptregs_foo:
leaq sys_foo(%rip), %rax
jmp stub_ptregs_64

stub_ptregs_64:

    testl $TS_EXTRAREGS, <current->ti_status>
    jnz 1f
    SAVE_EXTRA_REGS
    call *%rax
    RESTORE_EXTRA_REGS
    ret

Quoted 7 lines

1:
call *%rax
ret

This makes sure that the extra regs don't get saved a second time if coming in from the slow path, but preserves the fast path if not tracing.

I think there's value in having the entries in the table be genuine C ABI-compliant function pointers. In your example, it only barely works -- you can call them from C only if you have TS_EXTRAREGS set appropriately -- -otherwise you crash and burn. That will break the rest of the series.

We could adjust it a bit and check whether we're in C land (by checking rsp for ts) and jump into the slow path if we aren't, but I'm not sure this is a huge win. It does save some rodata space by avoiding duplicating the table.

Quoted 3 lines

We could adjust it a bit and check whether we're in C land (by checking rsp for ts) and jump into the slow path if we aren't, but I'm not sure this is a huge win. It does save some rodata space by avoiding duplicating the table.

The syscall table is huge. 545*8 bytes, over a full page. Duplicating it for just a few different entries is wasteful.

Quoted 6 lines · Brian Gerst

Quoted 3 lines

We could adjust it a bit and check whether we're in C land (by checking rsp for ts) and jump into the slow path if we aren't, but I'm not sure this is a huge win. It does save some rodata space by avoiding duplicating the table.

The syscall table is huge. 545*8 bytes, over a full page. Duplicating it for just a few different entries is wasteful.

Note that what matters more is cache footprint, not pure size: 1K of RAM overhead for something as fundamental as system calls is trivial cost.

So the questions to ask are along these lines:

- what is the typical locality of access (do syscall numbers cluster in time and space)

- how frequently would the two tables be accessed (is one accessed less frequently than the other?)

- subsequently how does the effective cache footprint change with the duplication?

it might still end up not being worth it - but it's not the RAM cost that is the main factor IMHO.

64-bit syscalls currently have an optimization in which they are called with partial pt_regs. A small handful require full pt_regs.

In the 32-bit and compat cases, I cleaned this up by forcing full pt_regs for all syscalls. The performance hit doesn't really matter.

I want to clean up the 64-bit case as well, but I don't want to hurt fast path performance. To do that, I want to force the syscalls that use pt_regs onto the slow path. This will enable us to make slow path syscalls be real ABI-compliant C functions.

Use the new syscall entry qualification machinery for this. stub_clone is now stub_clone/ptregs.

The next patch will eliminate the stubs, and we'll just have sys_clone/ptregs.

Signed-off-by Andy Lutomirski <[email protected]>

Quoted 11 lines · Andy Lutomirski

64-bit syscalls currently have an optimization in which they are called with partial pt_regs. A small handful require full pt_regs.

In the 32-bit and compat cases, I cleaned this up by forcing full pt_regs for all syscalls. The performance hit doesn't really matter.

I want to clean up the 64-bit case as well, but I don't want to hurt fast path performance. To do that, I want to force the syscalls that use pt_regs onto the slow path. This will enable us to make slow path syscalls be real ABI-compliant C functions.

Quoted 7 lines

Use the new syscall entry qualification machinery for this. stub_clone is now stub_clone/ptregs.

The next patch will eliminate the stubs, and we'll just have sys_clone/ptregs.

Signed-off-by Andy Lutomirski <[email protected]>

Fails to boot, bisected to this patch:

[   32.675319] kernel BUG at kernel/auditsc.c:1504!
[   32.675325] invalid opcode: 0000 [#65] SMP
[   32.675328] Modules linked in:
[   32.675333] CPU: 1 PID: 216 Comm: systemd-cgroups Tainted: G      D
        4.3.0-rc4+ #7
[   32.675336] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[   32.675339] task: ffff880000075340 ti: ffff880036520000 task.ti:
ffff880036520000
[   32.675350] RIP: 0010:[<ffffffff8113d9ed>]  [<ffffffff8113d9ed>]
__audit_syscall_entry+0xcd/0xf0
[   32.675353] RSP: 0018:ffff880036523ef0  EFLAGS: 00010202
[   32.675355] RAX: 000000000000000c RBX: ffff8800797b3000 RCX: 00007ffef8504e88
[   32.675357] RDX: 000056172f37cfd0 RSI: 0000000000000000 RDI: 000000000000000c
[   32.675359] RBP: ffff880036523f00 R08: 0000000000000001 R09: ffff880000075340
[   32.675361] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[   32.675363] R13: 00000000c000003e R14: 0000000000000001 R15: 0000000000001000
[   32.675380] FS:  00007f02b4ff48c0(0000) GS:ffff88007fc80000(0000)
knlGS:0000000000000000
[   32.675383] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   32.675385] CR2: 00007f93d47ea0e0 CR3: 0000000036aa9000 CR4: 00000000000006e0
[   32.675391] Stack:
[   32.675396]  ffff880036523f58 0000000000000000 ffff880036523f10
ffffffff8100321b
[   32.675401]  ffff880036523f48 ffffffff81003ad0 000056172f374040
00007f93d45c9990
[   32.675404]  0000000000000001 0000000000000001 0000000000001000
000000000000000a
[   32.675405] Call Trace:
[   32.675414]  [<ffffffff8100321b>] do_audit_syscall_entry+0x4b/0x70

[   32.675420]  [<ffffffff81003ad0>] syscall_trace_enter_phase2+0x110/0x1d0
[   32.675425]  [<ffffffff81761d94>] tracesys+0x3a/0x96
[   32.675464] Code: 00 00 00 00 e8 a5 e0 fc ff c7 43 04 01 00 00 00
48 89 43 18 48 89 53 20 44 89 63 0c c7 83 94 02 00 00 00 00 00 00 5b
41 5c 5d c3 <0f> 0b 48 c7 43 50 00 00 00 00 48 c7 c2 60 b4 c5 81 48 89
de 4c
[   32.675469] RIP  [<ffffffff8113d9ed>] __audit_syscall_entry+0xcd/0xf0
[   32.675471]  RSP <ffff880036523ef0>

Quoted 50 lines · Brian Gerst

Quoted 11 lines · Andy Lutomirski

64-bit syscalls currently have an optimization in which they are called with partial pt_regs. A small handful require full pt_regs.

In the 32-bit and compat cases, I cleaned this up by forcing full pt_regs for all syscalls. The performance hit doesn't really matter.

I want to clean up the 64-bit case as well, but I don't want to hurt fast path performance. To do that, I want to force the syscalls that use pt_regs onto the slow path. This will enable us to make slow path syscalls be real ABI-compliant C functions.

Quoted 7 lines

Use the new syscall entry qualification machinery for this. stub_clone is now stub_clone/ptregs.

The next patch will eliminate the stubs, and we'll just have sys_clone/ptregs.

Signed-off-by Andy Lutomirski <[email protected]>

Fails to boot, bisected to this patch:

[   32.675319] kernel BUG at kernel/auditsc.c:1504!
[   32.675325] invalid opcode: 0000 [#65] SMP
[   32.675328] Modules linked in:
[   32.675333] CPU: 1 PID: 216 Comm: systemd-cgroups Tainted: G      D
        4.3.0-rc4+ #7
[   32.675336] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[   32.675339] task: ffff880000075340 ti: ffff880036520000 task.ti:
ffff880036520000
[   32.675350] RIP: 0010:[<ffffffff8113d9ed>]  [<ffffffff8113d9ed>]
__audit_syscall_entry+0xcd/0xf0
[   32.675353] RSP: 0018:ffff880036523ef0  EFLAGS: 00010202
[   32.675355] RAX: 000000000000000c RBX: ffff8800797b3000 RCX: 00007ffef8504e88
[   32.675357] RDX: 000056172f37cfd0 RSI: 0000000000000000 RDI: 000000000000000c
[   32.675359] RBP: ffff880036523f00 R08: 0000000000000001 R09: ffff880000075340
[   32.675361] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[   32.675363] R13: 00000000c000003e R14: 0000000000000001 R15: 0000000000001000
[   32.675380] FS:  00007f02b4ff48c0(0000) GS:ffff88007fc80000(0000)
knlGS:0000000000000000
[   32.675383] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   32.675385] CR2: 00007f93d47ea0e0 CR3: 0000000036aa9000 CR4: 00000000000006e0
[   32.675391] Stack:
[   32.675396]  ffff880036523f58 0000000000000000 ffff880036523f10
ffffffff8100321b
[   32.675401]  ffff880036523f48 ffffffff81003ad0 000056172f374040
00007f93d45c9990
[   32.675404]  0000000000000001 0000000000000001 0000000000001000
000000000000000a
[   32.675405] Call Trace:
[   32.675414]  [<ffffffff8100321b>] do_audit_syscall_entry+0x4b/0x70

Quoted 8 lines

[   32.675420]  [<ffffffff81003ad0>] syscall_trace_enter_phase2+0x110/0x1d0
[   32.675425]  [<ffffffff81761d94>] tracesys+0x3a/0x96
[   32.675464] Code: 00 00 00 00 e8 a5 e0 fc ff c7 43 04 01 00 00 00
48 89 43 18 48 89 53 20 44 89 63 0c c7 83 94 02 00 00 00 00 00 00 5b
41 5c 5d c3 <0f> 0b 48 c7 43 50 00 00 00 00 48 c7 c2 60 b4 c5 81 48 89
de 4c
[   32.675469] RIP  [<ffffffff8113d9ed>] __audit_syscall_entry+0xcd/0xf0
[   32.675471]  RSP <ffff880036523ef0>

I'm not reproducing this, even with audit manually enabled. Can you send a .config?

Quoted 60 lines · Andy Lutomirski

Quoted 50 lines · Brian Gerst

Quoted 11 lines · Andy Lutomirski

64-bit syscalls currently have an optimization in which they are called with partial pt_regs. A small handful require full pt_regs.

In the 32-bit and compat cases, I cleaned this up by forcing full pt_regs for all syscalls. The performance hit doesn't really matter.

I want to clean up the 64-bit case as well, but I don't want to hurt fast path performance. To do that, I want to force the syscalls that use pt_regs onto the slow path. This will enable us to make slow path syscalls be real ABI-compliant C functions.

Quoted 7 lines

Use the new syscall entry qualification machinery for this. stub_clone is now stub_clone/ptregs.

The next patch will eliminate the stubs, and we'll just have sys_clone/ptregs.

Signed-off-by Andy Lutomirski <[email protected]>

Fails to boot, bisected to this patch:

[   32.675319] kernel BUG at kernel/auditsc.c:1504!
[   32.675325] invalid opcode: 0000 [#65] SMP
[   32.675328] Modules linked in:
[   32.675333] CPU: 1 PID: 216 Comm: systemd-cgroups Tainted: G      D
        4.3.0-rc4+ #7
[   32.675336] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[   32.675339] task: ffff880000075340 ti: ffff880036520000 task.ti:
ffff880036520000
[   32.675350] RIP: 0010:[<ffffffff8113d9ed>]  [<ffffffff8113d9ed>]
__audit_syscall_entry+0xcd/0xf0
[   32.675353] RSP: 0018:ffff880036523ef0  EFLAGS: 00010202
[   32.675355] RAX: 000000000000000c RBX: ffff8800797b3000 RCX: 00007ffef8504e88
[   32.675357] RDX: 000056172f37cfd0 RSI: 0000000000000000 RDI: 000000000000000c
[   32.675359] RBP: ffff880036523f00 R08: 0000000000000001 R09: ffff880000075340
[   32.675361] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[   32.675363] R13: 00000000c000003e R14: 0000000000000001 R15: 0000000000001000
[   32.675380] FS:  00007f02b4ff48c0(0000) GS:ffff88007fc80000(0000)
knlGS:0000000000000000
[   32.675383] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   32.675385] CR2: 00007f93d47ea0e0 CR3: 0000000036aa9000 CR4: 00000000000006e0
[   32.675391] Stack:
[   32.675396]  ffff880036523f58 0000000000000000 ffff880036523f10
ffffffff8100321b
[   32.675401]  ffff880036523f48 ffffffff81003ad0 000056172f374040
00007f93d45c9990
[   32.675404]  0000000000000001 0000000000000001 0000000000001000
000000000000000a
[   32.675405] Call Trace:
[   32.675414]  [<ffffffff8100321b>] do_audit_syscall_entry+0x4b/0x70

Quoted 8 lines

[   32.675420]  [<ffffffff81003ad0>] syscall_trace_enter_phase2+0x110/0x1d0
[   32.675425]  [<ffffffff81761d94>] tracesys+0x3a/0x96
[   32.675464] Code: 00 00 00 00 e8 a5 e0 fc ff c7 43 04 01 00 00 00
48 89 43 18 48 89 53 20 44 89 63 0c c7 83 94 02 00 00 00 00 00 00 5b
41 5c 5d c3 <0f> 0b 48 c7 43 50 00 00 00 00 48 c7 c2 60 b4 c5 81 48 89
de 4c
[   32.675469] RIP  [<ffffffff8113d9ed>] __audit_syscall_entry+0xcd/0xf0
[   32.675471]  RSP <ffff880036523ef0>

Quoted 2 lines

I'm not reproducing this, even with audit manually enabled. Can you send a .config?

Never mind, I found the bug by inspection. I'll send a fixed up series tomorrow.

Can you send the boot failure you got with the full series applied, though? I think that the bug I found is only triggerable part-way through the series -- I think I inadvertently fixed it later on.

Instead of using a duplicate syscall table for the fast path, create stubs for the syscalls that need pt_regs that save the extra registers if a flag for the slow path is not set.

Signed-off-by Brian Gerst <[email protected]>

To: Andy Lutomirski <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: the arch/x86 maintainers <[email protected]>
Cc: Linux Kernel Mailing List <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Frédéric Weisbecker <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Linus Torvalds <[email protected]>
---

Applies on top of Andy's syscall cleanup series.

arch/x86/entry/entry_64.S +1 -2

@@ -306,15 +306,37 @@ END(entry_SYSCALL_64)
 
 ENTRY(stub_ptregs_64)
        /*
-        * Syscalls marked as needing ptregs that go through the fast path
-        * land here.  We transfer to the slow path.
+        * Syscalls marked as needing ptregs land here.

Quoted 15 lines · Brian Gerst

Instead of using a duplicate syscall table for the fast path, create stubs for the syscalls that need pt_regs that save the extra registers if a flag for the slow path is not set.

Signed-off-by Brian Gerst <[email protected]>

To: Andy Lutomirski <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: the arch/x86 maintainers <[email protected]>
Cc: Linux Kernel Mailing List <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Frédéric Weisbecker <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Linus Torvalds <[email protected]>
---

Quoted 1 line

Applies on top of Andy's syscall cleanup series.

A couple questions:

Quoted 7 lines

arch/x86/entry/entry_64.S +1 -2

@@ -306,15 +306,37 @@ END(entry_SYSCALL_64)
 
 ENTRY(stub_ptregs_64)
        /*
-        * Syscalls marked as needing ptregs that go through the fast path
-        * land here.  We transfer to the slow path.
+        * Syscalls marked as needing ptregs land here.

arch/x86/entry/entry_64.S END(entry_SYSCALL_64) · line 315

Quoted 10 lines

arch/x86/entry/entry_64.S +5 -4

+        * If we are on the fast path, we need to save the extra regs.
+        * If we are on the slow path, the extra regs are already saved.
         */
-       DISABLE_INTERRUPTS(CLBR_NONE)
-       TRACE_IRQS_OFF
-       addq    $8, %rsp
-       jmp     entry_SYSCALL64_slow_path
+       movq    PER_CPU_VAR(cpu_current_top_of_stack), %r10
+       testl   $TS_SLOWPATH, ASM_THREAD_INFO(TI_status, %r10, 0)
+       jnz     1f

OK (but see below), but why not do:

addq $8, %rsp
jmp entry_SYSCALL64_slow_path

here instead of the stack munging below?

Quoted 10 lines

arch/x86/entry/entry_64.S +9 -0

+       subq    $SIZEOF_PTREGS, %r10
+       SAVE_EXTRA_REGS base=r10
+       movq    %r10, %rbx
+       call    *%rax
+       movq    %rbx, %r10
+       RESTORE_EXTRA_REGS base=r10
+       ret
+1:
+       jmp     *%rax
 END(stub_ptregs_64)

Also, can we not get away with keying off rip or rsp instead of ti->status? That should be faster and less magical IMO.

Quoted 40 lines · Andy Lutomirski

Quoted 15 lines · Brian Gerst

Instead of using a duplicate syscall table for the fast path, create stubs for the syscalls that need pt_regs that save the extra registers if a flag for the slow path is not set.

Signed-off-by Brian Gerst <[email protected]>

To: Andy Lutomirski <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: the arch/x86 maintainers <[email protected]>
Cc: Linux Kernel Mailing List <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Frédéric Weisbecker <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Linus Torvalds <[email protected]>
---

Quoted 1 line

Applies on top of Andy's syscall cleanup series.

A couple questions:

Quoted 7 lines

arch/x86/entry/entry_64.S +1 -2

@@ -306,15 +306,37 @@ END(entry_SYSCALL_64)
 
 ENTRY(stub_ptregs_64)
        /*
-        * Syscalls marked as needing ptregs that go through the fast path
-        * land here.  We transfer to the slow path.
+        * Syscalls marked as needing ptregs land here.

arch/x86/entry/entry_64.S END(entry_SYSCALL_64) · line 315

Quoted 10 lines

arch/x86/entry/entry_64.S +5 -4

+        * If we are on the fast path, we need to save the extra regs.
+        * If we are on the slow path, the extra regs are already saved.
         */
-       DISABLE_INTERRUPTS(CLBR_NONE)
-       TRACE_IRQS_OFF
-       addq    $8, %rsp
-       jmp     entry_SYSCALL64_slow_path
+       movq    PER_CPU_VAR(cpu_current_top_of_stack), %r10
+       testl   $TS_SLOWPATH, ASM_THREAD_INFO(TI_status, %r10, 0)
+       jnz     1f

OK (but see below), but why not do:

Quoted 2 lines

addq $8, %rsp
jmp entry_SYSCALL64_slow_path

I've always been adverse to doing things like that because it breaks call/return branch prediction.

Also, are there any side effects to calling enter_from_user_mode() more than once?

Quoted 13 lines

here instead of the stack munging below?

Quoted 10 lines

arch/x86/entry/entry_64.S +9 -0

+       subq    $SIZEOF_PTREGS, %r10
+       SAVE_EXTRA_REGS base=r10
+       movq    %r10, %rbx
+       call    *%rax
+       movq    %rbx, %r10
+       RESTORE_EXTRA_REGS base=r10
+       ret
+1:
+       jmp     *%rax
 END(stub_ptregs_64)

After some thought, that can be simplified. It's only executed on the fast path, so pt_regs is at 8(%rsp).

Quoted 2 lines

Also, can we not get away with keying off rip or rsp instead of ti->status? That should be faster and less magical IMO.

Checking if the return address is the instruction after the fast path dispatch would work.

Simplified version:
ENTRY(stub_ptregs_64)
cmpl $fast_path_return, (%rsp)

jne 1f
SAVE_EXTRA_REGS offset=8
call *%rax
RESTORE_EXTRA_REGS offset=8
ret
1:
jmp *%rax
END(stub_ptregs_64)

Quoted 3 lines · Brian Gerst

Simplified version:
ENTRY(stub_ptregs_64)
cmpl $fast_path_return, (%rsp)

Does that instruction actually work the way you want it to? (Does it link?) I think you might need to use leaq the way I did in my patch.

Quoted 8 lines

jne 1f
SAVE_EXTRA_REGS offset=8
call *%rax
RESTORE_EXTRA_REGS offset=8
ret
1:
jmp *%rax
END(stub_ptregs_64)

This'll work, I think, but I still think I prefer keeping as much complexity as possible in the slow path. I could be convinced otherwise, though -- this variant is reasonably clean.

Quoted 7 lines · Andy Lutomirski

Quoted 3 lines · Brian Gerst

Simplified version:
ENTRY(stub_ptregs_64)
cmpl $fast_path_return, (%rsp)

Does that instruction actually work the way you want it to? (Does it link?) I think you might need to use leaq the way I did in my patch.

Quoted 9 lines

Quoted 8 lines

jne 1f
SAVE_EXTRA_REGS offset=8
call *%rax
RESTORE_EXTRA_REGS offset=8
ret
1:
jmp *%rax
END(stub_ptregs_64)

Quoted 3 lines

This'll work, I think, but I still think I prefer keeping as much complexity as possible in the slow path. I could be convinced otherwise, though -- this variant is reasonably clean.

On further reflection, there's at least one functional difference. With my variant, modifying pt_regs from sys_foo/ptregs is safe. In your variant, it's unsafe unless force_iret() is called. I don't know whether we care.

Quoted 6 lines · Andy Lutomirski

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines · Brian Gerst

Simplified version:
ENTRY(stub_ptregs_64)
cmpl $fast_path_return, (%rsp)

Quoted 2 lines

Quoted 2 lines

Does that instruction actually work the way you want it to? (Does it link?) I think you might need to use leaq the way I did in my patch.

It should have been cmpq. leaq isn't necessary, since immediates are sign-extended to 64-bit.

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

jne 1f
SAVE_EXTRA_REGS offset=8
call *%rax
RESTORE_EXTRA_REGS offset=8
ret
1:
jmp *%rax
END(stub_ptregs_64)

This'll work, I think, but I still think I prefer keeping as much complexity as possible in the slow path. I could be convinced otherwise, though -- this variant is reasonably clean.

Quoted 4 lines

On further reflection, there's at least one functional difference. With my variant, modifying pt_regs from sys_foo/ptregs is safe. In your variant, it's unsafe unless force_iret() is called. I don't know whether we care.

I can go either way at this point. My main concern was getting rid of the duplicate table.

I want all of the syscall entries to run with interrupts off so that I can efficiently run context tracking before enabling interrupts.

This will regress int $0x80 performance on 32-bit kernels by a couple of cycles. This shouldn't matter much -- int $0x80 is not a fast path.

This effectively reverts 657c1eea0019 ("x86/entry/32: Fix entry_INT80_32() to expect interrupts to be on") and fixes the issue differently.

Signed-off-by Andy Lutomirski <[email protected]>

This is kind of like the 32-bit and compat code, except that I preserved the fast path this time. I was unable to measure any significant performance change on my laptop in the fast path.

What do you all think?

Quoted 4 lines · Andy Lutomirski

This is kind of like the 32-bit and compat code, except that I preserved the fast path this time. I was unable to measure any significant performance change on my laptop in the fast path.

Quoted 1 line

What do you all think?

For completeness, if I zap the fast path entirely (see attached), I lose 20 cycles (148 cycles vs 128 cycles) on Skylake. Switching between movq and pushq for stack setup makes no difference whatsoever, interestingly. I haven't tried to figure out exactly where those 20 cycles go.

Quoted 6 lines · Andy Lutomirski

Quoted 4 lines · Andy Lutomirski

This is kind of like the 32-bit and compat code, except that I preserved the fast path this time. I was unable to measure any significant performance change on my laptop in the fast path.

Quoted 6 lines

Quoted 1 line

What do you all think?

For completeness, if I zap the fast path entirely (see attached), I lose 20 cycles (148 cycles vs 128 cycles) on Skylake. Switching between movq and pushq for stack setup makes no difference whatsoever, interestingly. I haven't tried to figure out exactly where those 20 cycles go.

So I asked for this before, and I'll do so again: could you please stick the cycle granular system call performance test into a 'perf bench' variant so that:

1) More people can run it all on various pieces of hardware and help out quantify the patches.

2) We can keep an eye on not regressing base system call performance in the future, with a good in-tree testcase.

Quoted 19 lines · Ingo Molnar

Quoted 6 lines · Andy Lutomirski

Quoted 4 lines · Andy Lutomirski

This is kind of like the 32-bit and compat code, except that I preserved the fast path this time. I was unable to measure any significant performance change on my laptop in the fast path.

Quoted 6 lines

Quoted 1 line

What do you all think?

For completeness, if I zap the fast path entirely (see attached), I lose 20 cycles (148 cycles vs 128 cycles) on Skylake. Switching between movq and pushq for stack setup makes no difference whatsoever, interestingly. I haven't tried to figure out exactly where those 20 cycles go.

So I asked for this before, and I'll do so again: could you please stick the cycle granular system call performance test into a 'perf bench' variant so that:

Quoted 6 lines

1) More people can run it all on various pieces of hardware and help out quantify the patches.

2) We can keep an eye on not regressing base system call performance in the future, with a good in-tree testcase.

Is it okay if it's not particularly shiny or modular? [...]

[...] The tool I'm using is here:

https://git.kernel.org/cgit/linux/kernel/git/luto/misc-tests.git/tree/tight_loop/perf_self_monitor.c

and I can certainly stick it into 'perf bench' pretty easily. Can I leave making it into a proper library to some future contributor?

It's actually decently fancy. It allocates a perf self-monitoring instance that counts cycles, and then it takes a bunch of samples and discards any that flagged a context switch. It does some very rudimentary statistics on the rest. It's utterly devoid of a fancy UI, though.

It works very well on native, and it works better than I had expected under KVM. (KVM traps RDPMC because neither Intel nor AMD has seen fit to provide any sensible way to virtualize RDPMC without exiting.)