| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 4 May 2021 13:39:35 +0200
From: Stefan Metzmacher <metze@...ba.org>
To: Jens Axboe <axboe@...nel.dk>,
Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Andy Lutomirski <luto@...nel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
io-uring <io-uring@...r.kernel.org>,
the arch/x86 maintainers <x86@...nel.org>
Subject: Re: [PATCH] io_thread/x86: don't reset 'cs', 'ss', 'ds' and 'es'
registers for io_threads
Am 04.05.21 um 04:50 schrieb Jens Axboe:
> On 5/3/21 5:48 PM, Linus Torvalds wrote:
>> On Mon, May 3, 2021 at 4:27 PM Stefan Metzmacher <metze@...ba.org> wrote:
>>>
>>> If I remember correctly gdb showed bogus addresses for the backtraces of the io_threads,
>>> as some regs where not cleared.
>>
>> Yeah, so that patch will make the IO thread have the user stack
>> pointer point to the original user stack, but that stack will
>> obviously be used by the original thread which means that it will
>> contain random stuff on it.
>>
>> Doing a
>>
>> childregs->sp = 0;
>>
>> is probably a good idea for that PF_IO_WORKER case, since it really
>> doesn't have - or need - a user stack.
>>
>> Of course, it doesn't really have - or need - any of the other user
>> registers either, but once you fill in the segment stuff to make gdb
>> happy, you might as well fill it all in using the same code that the
>> regular case does.
>
> I tested the below, which is the two combined, with a case that
> deliberately has two types of io threads - one for SQPOLL submission,
> and one that was created due to async work being needed. gdb attaches
> just fine to the creator, with a slight complaint:
>
> Attaching to process 370
> [New LWP 371]
> [New LWP 372]
> Error while reading shared library symbols for /usr/lib/libpthread.so.0:
> Cannot find user-level thread for LWP 372: generic error
> 0x00007f1a74675125 in clock_nanosleep@...BC_2.2.5 () from /usr/lib/libc.so.6
> (gdb) info threads
> Id Target Id Frame
> * 1 LWP 370 "io_uring" 0x00007f1a74675125 in clock_nanosleep@...BC_2.2.5 ()
> from /usr/lib/libc.so.6
> 2 LWP 371 "iou-sqp-370" 0x00007f1a746a7a9d in syscall () from /usr/lib/libc.so.6
> 3 LWP 372 "io_uring" 0x00007f1a74675125 in clock_nanosleep@...BC_2.2.5 ()
> from /usr/lib/libc.so.6
>
> (gdb) thread 2
> [Switching to thread 2 (LWP 371)]
> #0 0x00007f1a746a7a9d in syscall () from /usr/lib/libc.so.6
> (gdb) bt
> #0 0x00007f1a746a7a9d in syscall () from /usr/lib/libc.so.6
> Backtrace stopped: Cannot access memory at address 0x0
>
> (gdb) thread 1
> [Switching to thread 1 (LWP 370)]
> #0 0x00007f1a74675125 in clock_nanosleep@...BC_2.2.5 () from /usr/lib/libc.so.6
> (gdb) bt
> #0 0x00007f1a74675125 in clock_nanosleep@...BC_2.2.5 () from /usr/lib/libc.so.6
> #1 0x00007f1a7467a357 in nanosleep () from /usr/lib/libc.so.6
> #2 0x00007f1a7467a28e in sleep () from /usr/lib/libc.so.6
> #3 0x000055bd41e929ba in main (argc=<optimized out>, argv=<optimized out>)
> at t/io_uring.c:658
>
> which looks very reasonable to me - no backtraces for the io threads, and
> no arch complaints.
>
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 43cbfc84153a..58987bce90e2 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -156,7 +156,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,
> #endif
>
> /* Kernel thread ? */
> - if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
> + if (unlikely(p->flags & PF_KTHREAD)) {
> memset(childregs, 0, sizeof(struct pt_regs));
> kthread_frame_init(frame, sp, arg);
> return 0;
> @@ -168,6 +168,12 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,
> if (sp)
> childregs->sp = sp;
>
> + if (unlikely(p->flags & PF_IO_WORKER)) {
> + childregs->sp = 0;
> + kthread_frame_init(frame, sp, arg);
> + return 0;
> + }
> +
> #ifdef CONFIG_X86_32
> task_user_gs(p) = get_user_gs(current_pt_regs());
> #endif
I'm currently testing this (moving things to the end and resetting ->ip = 0 too)
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -161,7 +161,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,
#endif
/* Kernel thread ? */
- if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
+ if (unlikely(p->flags & PF_KTHREAD)) {
memset(childregs, 0, sizeof(struct pt_regs));
kthread_frame_init(frame, sp, arg);
return 0;
@@ -184,6 +184,23 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,
if (!ret && unlikely(test_tsk_thread_flag(current, TIF_IO_BITMAP)))
io_bitmap_share(p);
+ /*
+ * An IO thread is a user space thread, but it doesn't
+ * return to ret_after_fork().
+ *
+ * In order to indicate that to tools like gdb,
+ * we reset the stack and instruction pointers.
+ *
+ * It does the same kernel frame setup to return to a kernel
+ * function that a kernel thread does.
+ */
+ if (!ret && unlikely(p->flags & PF_IO_WORKER)) {
+ childregs->sp = 0;
+ childregs->ip = 0;
+ kthread_frame_init(frame, sp, arg);
+ return 0;
+ }
+
return ret;
}
which means the output looks like this:
(gdb) info threads
Id Target Id Frame
* 1 LWP 4863 "io_uring-cp-for" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
2 LWP 4864 "iou-mgr-4863" 0x0000000000000000 in ?? ()
3 LWP 4865 "iou-wrk-4863" 0x0000000000000000 in ?? ()
(gdb) thread 3
[Switching to thread 3 (LWP 4865)]
#0 0x0000000000000000 in ?? ()
(gdb) bt
#0 0x0000000000000000 in ?? ()
Backtrace stopped: Cannot access memory at address 0x0
I think "0x0000000000000000 in ?? ()" is a relative sane indication that this thread
will never return to userspace. I'd prefer this over:
> (gdb) thread 2
> [Switching to thread 2 (LWP 371)]
> #0 0x00007f1a746a7a9d in syscall () from /usr/lib/libc.so.6
> (gdb) bt
> #0 0x00007f1a746a7a9d in syscall () from /usr/lib/libc.so.6
> Backtrace stopped: Cannot access memory at address 0x0
which seem to indicate that the syscall returns eventually.
What do you think? Should I post that as v2 if my final testing doesn't find any problem?
Thanks!
metze
Powered by blists - more mailing lists