Message-ID: <CALCETrV9bCenqzzaW6Ra18tCvNP-my09decTjmLDVZZAQxR6VA@mail.gmail.com>
Date:   Mon, 3 May 2021 13:15:21 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Stefan Metzmacher <metze@...ba.org>,
        Jens Axboe <axboe@...nel.dk>,
        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 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?

>
> But then that (b) issue means that gdb gets confused by them. I
> personally think that's just a pure gdb mis-feature, but I also think
> that "hey, if we just make the register state look like the main
> thread, and unconfuse gdb that way, problem solved".
>
> So I'd actually rather not make these non-special threads any more
> special at all. And I strongly suspect that making ptrace() not work
> on them will just confuse gdb even more - so it would make them just
> unnecessarily special in the kernel, for no actual gain.
>
> Is the right thing to do to fix gdb to not look at irrelevant thread B
> when deciding whether thread A is 64-bit or not? Yeah, that seems like
> obviously the RightThing(tm) to me.
>
> But at the same time, this is arguably about "regression", although at
> the same time it's "gdb doesn't understand new user programs that use
> new features, film at 11", so I think that argument is partly bogus
> too.
>

Fair enough.  But I would really, really rather that gdb starts fixing
its amazingly broken assumptions about bitness.

> So my personal preference would be:
>
>  - make those threads look even more like user threads, even if that
> means giving them pointless user segment data that the threads
> themselves will never use
>
>    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.  In general, I think that piling a bitness hack in
here is a mess, and we're going to have to carry it forward forever
once we do it.

Meanwhile, I am going to put my foot down about one thing: NAK to this
patch until it comes with a selftest.  That selftest needs to test the
cs behavior, but it also needs to read the FPU state from an io_uring
thread, write some FPU state to that thread, and read it back.  And it
needs to not OOPS.  Not breaking ABI is nice and all, but even more
important is not breaking the kernel.

--Andy

Powered by blists - more mailing lists