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: <d26e3a82-8a2c-7354-d36b-cac945c208c7@kernel.dk>
Date:   Mon, 3 May 2021 15:26:10 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>
Cc:     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 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.

-- 
Jens Axboe