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]

Message-ID: <CALCETrWmhquicE2C=G2Hmwfj4VNypXVxY-K3CWOkyMe9Edv88A@mail.gmail.com>
Date:   Mon, 3 May 2021 14:49:16 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Jens Axboe <axboe@...nel.dk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        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 2:26 PM Jens Axboe <axboe@...nel.dk> wrote:
>
> On 5/3/21 2:37 PM, Linus Torvalds wrote:
> > On Mon, May 3, 2021 at 1:15 PM Andy Lutomirski <luto@...nel.org> wrote:
> >>
> >> On Mon, May 3, 2021 at 12:15 PM Linus Torvalds
> >> <torvalds@...ux-foundation.org> wrote:
> >>> So generally, the IO threads are now 100% normal threads - it's
> >>> literally just that they never return to user space because they are
> >>> always just doing the IO offload on the kernel side.
> >>>
> >>> That part is lovely, but part of the "100% IO threads" really is that
> >>> they share the signal struct too, which in turn means that they very
> >>> much show up as normal threads. Again, not a problem: they really
> >>> _are_ normal threads for all intents and purposes.
> >>
> >> I'm a bit confused, though.  All the ptrace register access (AFAICS)
> >> goes through ptrace_check_attach(), which should wait until the tracee
> >> is stopped.  Does the io_uring thread now stop in response to ptrace
> >> stop requests?
> >
> > Yup. They really are 100% regular threads. Things like ^Z and friends
> > also stop them now, and the freezer freezes them etc.
> >
> > And making PTRACE_ATTACH fail just causes gdb to fail.
> >
> >> Fair enough.  But I would really, really rather that gdb starts fixing
> >> its amazingly broken assumptions about bitness.
> >
> > "Preach it, Brother"
>
> That's actually what the original code did, and the "only" problem with
> it was that gdb shits itself and just go into an infinite loop trying to
> attach. And yes, that's most certainly a gdb bug, and we entertained a
> few options for making that work. One was hiding the threads, but nobody
> (myself included) liked that, because then we're special casing
> something again, and for no other reason than gdb is buggy.
>
> On principle, I think it's arguably the right change to just -EINVAL the
> attach. However, a part of me also finds it very annoying that anyone
> attempting to debug any program that uses io_uring will not be able to
> do so with a buggy gdb. That's regardless of whether or not you want to
> look at the io threads or not, or even if you don't care about debugging
> the io_uring side of things. And I'm assuming this will take a while to
> get fixed, and then even longer to make its way back to distros.
>
> So... You should just make the call. At least then I can just tell
> people that Linus made that decision :-)
>
> >>>    So I think Stefan's patch is reasonable, if not pretty. Literally
> >>> becasue of that "make these threads look even more normal"
> >>
> >> I think it's reasonable except for the bit about copying the segment
> >> regs.  Can we hardcode __USER_CS, etc, and, when gdb crashes or
> >> otherwise malfunctions for compat programs, we can say that gdb needs
> >> to stop sucking.
> >
> > So that was actually my initial suggestion. Stefan really does seem to
> > care about compat programs.
> >
> > Any "gdb breaks" would be good to motivate people to fix gdb, but the
> > thing is, presumably nobody actually wants to touch gdb with a ten
> > foot pole.
> >
> > And a "let's break gdb to encourage people to fix it" only works if
> > people actually _do_ fit it. Which doesn't seem to be happening.
> >
> > Two lines of kernel code seems to be the better option than hoping for
> > gdb to be fixed.
>
> As far as I'm concerned, gdb works "well enough" with io threads as it
> stands. Yes, it'll complain a bit, but nothing that prevents you from
> attaching to a progrem. If we -EINVAL, then gdb will become useless for
> debugging a program that uses io_uring. And I'm not holding my breath
> while someone fixes that.

To be clear, I'm suggesting that we -EINVAL the PTRACE_GETREGS calls
and such, not the ATTACH.  I have no idea what gdb will do if this
happens, though.

--Andy