[PATCH v2 0/1] MR4683: ntdll: Restore fs32_sel only if pthread TEB was not set already in init_handler.
Fixes spurious crashes in Steam when downloading games.
The download causes a huge amount of SIGUSR1 signals, and it becomes very likely that one signal will be received while being inside the syscall or unix call dispatchers.
When this happens, it can be received within the small range of instructions where %fs has been restored but we have not yet switched to the syscall stack, or the other way around in the return path.
The signal handler then was restoring the 32bit %fs while returning to the syscall dispatcher, then we are entering a syscall with %fs set to the wrong value.
-- v2: ntdll: Avoid breaking leave_handler heuristics for fs32_sel restore.
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/ntdll/unix/signal_x86_64.c | 93 ++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 41 deletions(-)
diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 0c291633ac6..d69c0fbf78d 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -1596,14 +1596,14 @@ __ASM_GLOBAL_FUNC( call_user_mode_callback, "movl %r14d,0xb0(%rsp)\n\t" /* frame->syscall_flags */ "movq %r10,0xa0(%rsp)\n\t" /* frame->prev_frame */ "movq %rsp,0x328(%r8)\n\t" /* amd64_thread_data()->syscall_frame */ + /* switch to user stack */ + "movq %rdi,%rsp\n\t" /* user_rsp */ #ifdef __linux__ "testl $12,%r14d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */ "jz 1f\n\t" "movw 0x338(%r8),%fs\n" /* amd64_thread_data()->fs */ "1:\n\t" #endif - /* switch to user stack */ - "movq %rdi,%rsp\n\t" /* user_rsp */ "jmpq *%rcx" ) /* func */
@@ -2666,20 +2666,6 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, __ASM_CFI_REG_IS_AT1(r14, rbp, 0x48) __ASM_CFI_REG_IS_AT1(r15, rbp, 0x50) __ASM_CFI_REG_IS_AT1(rbp, rbp, 0x00) -#ifdef __linux__ - "testl $12,%r14d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */ - "jz 2f\n\t" - "movq %gs:0x320,%rsi\n\t" /* amd64_thread_data()->pthread_teb */ - "testl $8,%r14d\n\t" /* SYSCALL_HAVE_WRFSGSBASE */ - "jz 1f\n\t" - "wrfsbase %rsi\n\t" - "jmp 2f\n" - "1:\tmov $0x1002,%edi\n\t" /* ARCH_SET_FS */ - "mov $158,%eax\n\t" /* SYS_arch_prctl */ - "syscall\n\t" - "leaq -0x98(%rbp),%rcx\n" - "2:\n\t" -#endif "movq 0x28(%rsp),%r12\n\t" /* 5th argument */ "movq 0x30(%rsp),%r13\n\t" /* 6th argument */ "leaq 0x38(%rsp),%rsi\n\t" /* 7th argument */ @@ -2696,6 +2682,22 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, __ASM_CFI(".cfi_offset %r15,-0x38\n\t") __ASM_CFI(".cfi_undefined %rdi\n\t") __ASM_CFI(".cfi_undefined %rsi\n\t") +#ifdef __linux__ + "mov %rsi,%r15\n\t" + "testl $12,%r14d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */ + "jz 2f\n\t" + "movq %gs:0x320,%rsi\n\t" /* amd64_thread_data()->pthread_teb */ + "testl $8,%r14d\n\t" /* SYSCALL_HAVE_WRFSGSBASE */ + "jz 1f\n\t" + "wrfsbase %rsi\n\t" + "jmp 2f\n" + "1:\tmov $0x1002,%edi\n\t" /* ARCH_SET_FS */ + "mov $158,%eax\n\t" /* SYS_arch_prctl */ + "syscall\n\t" + "leaq -0x98(%rbp),%rcx\n" + "2:\n\t" + "mov %r15,%rsi\n\t" +#endif "movq 0x00(%rcx),%rax\n\t" "movq 0x18(%rcx),%r11\n\t" /* 2nd argument */ "movl %eax,%ebx\n\t" @@ -2732,12 +2734,6 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "leaq -0x98(%rbp),%rcx\n\t" __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_return") ":\n\t" "movl 0xb4(%rcx),%edx\n\t" /* frame->restore_flags */ -#ifdef __linux__ - "testl $12,%r14d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */ - "jz 1f\n\t" - "movw %gs:0x338,%fs\n" /* amd64_thread_data()->fs */ - "1:\n\t" -#endif "testl $0x48,%edx\n\t" /* CONTEXT_FLOATING_POINT | CONTEXT_XSTATE */ "jnz 2f\n\t" "movaps 0x1c0(%rcx),%xmm6\n\t" @@ -2792,6 +2788,13 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, __ASM_CFI(".cfi_same_value %r15\n\t") __ASM_CFI(".cfi_same_value %rdi\n\t") __ASM_CFI(".cfi_same_value %rsi\n\t") +#ifdef __linux__ + "movl 0xb0(%rcx),%r9d\n\t" /* frame->syscall_flags */ + "testl $12,%r9d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */ + "jz 2f\n\t" + "movw %gs:0x338,%fs\n" /* amd64_thread_data()->fs */ + "2:\n\t" +#endif "movq 0x70(%rcx),%rcx\n\t" /* frame->rip */ __ASM_CFI(".cfi_register rip, rcx\n\t") "pushq %rcx\n\t" @@ -2801,6 +2804,13 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, __ASM_CFI("\t.cfi_restore_state\n")
"1:\tleaq 0x70(%rcx),%rsp\n\t" +#ifdef __linux__ + "movl 0xb0(%rcx),%r9d\n\t" /* frame->syscall_flags */ + "testl $12,%r9d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */ + "jz 1f\n\t" + "movw %gs:0x338,%fs\n" /* amd64_thread_data()->fs */ + "1:\n\t" +#endif "testl $0x2,%edx\n\t" /* CONTEXT_INTEGER */ "jnz 1f\n\t" "movq 0x10(%rsp),%r11\n\t" /* frame->eflags */ @@ -2873,6 +2883,19 @@ __ASM_GLOBAL_FUNC( __wine_unix_call_dispatcher, "movdqa %xmm14,0x240(%rcx)\n\t" "movdqa %xmm15,0x250(%rcx)\n\t" "movl 0xb0(%rcx),%r14d\n\t" /* frame->syscall_flags */ + /* switch to kernel stack */ + "movq %rcx,%rsp\n" + /* we're now on the kernel stack, stitch unwind info with previous frame */ + __ASM_CFI_CFA_IS_AT2(rsp, 0xa8, 0x01) /* frame->syscall_cfa */ + __ASM_CFI(".cfi_offset %rip,-0x08\n\t") + __ASM_CFI(".cfi_offset %rbp,-0x10\n\t") + __ASM_CFI(".cfi_offset %rbx,-0x18\n\t") + __ASM_CFI(".cfi_offset %r12,-0x20\n\t") + __ASM_CFI(".cfi_offset %r13,-0x28\n\t") + __ASM_CFI(".cfi_offset %r14,-0x30\n\t") + __ASM_CFI(".cfi_offset %r15,-0x38\n\t") + __ASM_CFI(".cfi_undefined %rdi\n\t") + __ASM_CFI(".cfi_undefined %rsi\n\t") #ifdef __linux__ "testl $12,%r14d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */ "jz 2f\n\t" @@ -2888,19 +2911,6 @@ __ASM_GLOBAL_FUNC( __wine_unix_call_dispatcher, "mov %r9,%rcx\n\t" "2:\n\t" #endif - /* switch to kernel stack */ - "movq %rcx,%rsp\n" - /* we're now on the kernel stack, stitch unwind info with previous frame */ - __ASM_CFI_CFA_IS_AT2(rsp, 0xa8, 0x01) /* frame->syscall_cfa */ - __ASM_CFI(".cfi_offset %rip,-0x08\n\t") - __ASM_CFI(".cfi_offset %rbp,-0x10\n\t") - __ASM_CFI(".cfi_offset %rbx,-0x18\n\t") - __ASM_CFI(".cfi_offset %r12,-0x20\n\t") - __ASM_CFI(".cfi_offset %r13,-0x28\n\t") - __ASM_CFI(".cfi_offset %r14,-0x30\n\t") - __ASM_CFI(".cfi_offset %r15,-0x38\n\t") - __ASM_CFI(".cfi_undefined %rdi\n\t") - __ASM_CFI(".cfi_undefined %rsi\n\t") "movq %r8,%rdi\n\t" /* args */ "callq *(%r10,%rdx,8)\n\t" "movq %rsp,%rcx\n" @@ -2916,12 +2926,6 @@ __ASM_GLOBAL_FUNC( __wine_unix_call_dispatcher, "movdqa 0x250(%rcx),%xmm15\n\t" "testl $0xffff,0xb4(%rcx)\n\t" /* frame->restore_flags */ "jnz " __ASM_LOCAL_LABEL("__wine_syscall_dispatcher_return") "\n\t" -#ifdef __linux__ - "testl $12,%r14d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */ - "jz 1f\n\t" - "movw %gs:0x338,%fs\n" /* amd64_thread_data()->fs */ - "1:\n\t" -#endif "movq 0x60(%rcx),%r14\n\t" "movq 0x28(%rcx),%rdi\n\t" "movq 0x20(%rcx),%rsi\n\t" @@ -2937,6 +2941,13 @@ __ASM_GLOBAL_FUNC( __wine_unix_call_dispatcher, __ASM_CFI(".cfi_undefined %r15\n\t") __ASM_CFI(".cfi_same_value %rdi\n\t") __ASM_CFI(".cfi_same_value %rsi\n\t") +#ifdef __linux__ + "movl 0xb0(%rcx),%r9d\n\t" /* frame->syscall_flags */ + "testl $12,%r9d\n\t" /* SYSCALL_HAVE_PTHREAD_TEB | SYSCALL_HAVE_WRFSGSBASE */ + "jz 1f\n\t" + "movw %gs:0x338,%fs\n" /* amd64_thread_data()->fs */ + "1:\n\t" +#endif "pushq 0x70(%rcx)\n\t" /* frame->rip */ __ASM_CFI(".cfi_adjust_cfa_offset 8\n\t") "ret" )
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=141173
Your paranoid android.
=== debian11b (64 bit WoW report) ===
ntdll: exception.c:5444: Test failed: Expected r11 (0x602) to equal EFLAGS (0x646). exception.c:5454: Test failed: Expected r11 (0x602) to equal EFLAGS (0x646). exception.c:5461: Test failed: Expected r11 (0x602) to equal EFLAGS (0x646). exception.c:5474: Test failed: Expected r11 (0x602) to equal EFLAGS (0x646). exception.c:5480: Test failed: Expected r11 (0x602) to equal EFLAGS (0x646).
On Wed Dec 13 10:58:39 2023 +0000, Alexandre Julliard wrote:
The iret path moves %rsp above the syscall frame so it should still do the right thing.
Hmm okay, I think I did something like that, though assembly isn't my forte.
This merge request was closed by Rémi Bernon.
-
Marvin -
Rémi Bernon -
Rémi Bernon (＠rbernon)