linux-kernel - Re: [PATCH] io_thread/x86: don't reset 'cs', 'ss', 'ds' and 'es' registers for io

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 May 2021 17:01:45 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        Jens Axboe <axboe@...nel.dk>,
        Stefan Metzmacher <metze@...ba.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

On Mon, May 3, 2021 at 4:16 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Mon, May 3, 2021 at 3:56 PM Thomas Gleixner <tglx@...utronix.de> wrote:
> >
> > It's all fine that we have lots of blurb about GDB, but there is no
> > reasoning why this does not affect regular kernel threads which take the
> > same code path.
>
> Actual kernel threads don't get attached to by ptrace.
>
> > This is a half setup user space thread which is assumed to behave like a
> > regular kernel thread, but is this assumption actually true?
>
> No, no.
>
> It's a *fully set up USER thread*.
>
> Those IO threads used to be kernel threads. That didn't work out for
> the reasons already mentioned earlier.
>
> These days they really are fully regular user threads, they just don't
> return to user space because they continue to do the IO work that they
> were created for.
>
> Maybe instead of Stefan's patch, we could do something like this:
>
>    diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>    index 43cbfc84153a..890f3992e781 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,17 @@ int copy_thread(unsigned long clone_flags,
> unsigned long sp, unsigned long arg,
>         if (sp)
>                 childregs->sp = sp;
>
>    +    /*
>    +     * An IO thread is a user space thread, but it doesn't
>    +     * return to ret_after_fork(), it does the same kernel
>    +     * frame setup to return to a kernel function that
>    +     * a kernel thread does.
>    +     */
>    +    if (unlikely(p->flags & PF_IO_WORKER)) {
>    +            kthread_frame_init(frame, sp, arg);
>    +            return 0;
>    +    }
>    +
>     #ifdef CONFIG_X86_32
>         task_user_gs(p) = get_user_gs(current_pt_regs());
>     #endif
>
> does that clarify things and make people happier?
>
> Maybe the compiler might even notice that the
>
>                 kthread_frame_init(frame, sp, arg);
>                 return 0;
>
> part is common code and then it will result in less generated code too.
>
> NOTE! The above is - as usual - COMPLETELY UNTESTED. It looks obvious
> enough, and it builds cleanly. But that's all I'm going to guarantee.
>
> It's whitespace-damaged on purpose.

I like this patch considerably more than I liked the previous patch.

FWIW, I have this fixlet sitting around:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/kentry&id=1eef07ae5b236112c9a0c5d880d7f9bb13e73761

Your patch fixes the same bug for the specific case of io_uring.