From: Stefan Metzmacher <metze@samba.org>
To: Simon Marchi <simon.marchi@polymtl.ca>,
Borislav Petkov <bp@alien8.de>,
Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Andy Lutomirski <luto@amacapital.net>,
Thomas Gleixner <tglx@linutronix.de>,
Jens Axboe <axboe@kernel.dk>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
io-uring <io-uring@vger.kernel.org>,
the arch/x86 maintainers <x86@kernel.org>,
linux-toolchains@vger.kernel.org
Subject: Re: [PATCH] io_thread/x86: don't reset 'cs', 'ss', 'ds' and 'es' registers for io_threads
Date: Thu, 6 May 2021 00:21:19 +0200 [thread overview]
Message-ID: <f15f0ccd-fe30-c3cf-9b01-df7ba462401f@samba.org> (raw)
In-Reply-To: <30e248aa-534d-37ff-2954-a70a454391fc@polymtl.ca>
Hi Simon,
> When you attach to PIDVAL (assuming that PIDVAL is a thread-group
> leader), GDB attaches to all the threads of that thread group. The
> inferior_ptid global variable is "the thread we are currently working
> with", and changes whenever GDB wants to deal with a different thread.
>
> After attaching to all threads, GDB wants to know more about that
> process' architecture (that read_description call mentioned in [1]).
> The way this is implemented varies from arch to arch, but as you found
> out, for x86-64 it consists of peeking into a thread's CS/DS to
> determine whether the process is x86-64, x32 or i386. The thread used
> for this - the inferior_ptid value - just happens to be the latest
> thread we switched inferior_ptid to (presumably because we iterated on
> the thread list to do something and that was the last thread in the
> list). And up to now, this was working under the assumption that
> picking any thread of the process would yield the same values for that
> purpose. I don't think it was intentionally done this way, but it
> worked and didn't cause any trouble until now.
>
> With io threads, that assumption doesn't hold anymore.
Yes, in 5.12
> From what I understand, your v1 patch made it so that the kernel puts the CS/DS
> values GDB expects in the io threads (even though they are never
> actually used otherwise). I don't understand how your v2 patch
> addresses the problem though.
v1 did clear everything and tried to keep some selected registers:
'memset(childregs, 0, sizeof(struct pt_regs));'
childregs->cs = currenttrgs->cs;
childregs->ss = currenttrgs->ss;
childregs->ds = currenttrgs->ds;
childregs->es = currenttrgs->es;
v2 copies everything and only clears 3 selected registers (I added the last two for
the PF_IO_WORKER case:
*childregs = *current_pt_regs();
childregs->ax = 0;
...
childregs->ip = 0;
childregs->sp = 0;
So regarding childregs->cs and childregs->ds the effect is the same.
(Also note that on x86_64 ds in handled by savesegment(ds, p->thread.ds);
already instead so the effective problem as only childregs->cs which
is cleared in 5.12, but will be kept with both of my patches.
> I don't think it would be a problem on the GDB-side to make sure to
> fetch these values from a "standard" thread. Most likely the thread
> group leader, like Tom has proposed in [1].
Ok.
Is it clear now?
metze
next prev parent reply other threads:[~2021-05-05 22:21 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <8735v3ex3h.ffs@nanos.tec.linutronix.de>
[not found] ` <3C41339D-29A2-4AB1-958F-19DB0A92D8D7@amacapital.net>
[not found] ` <CAHk-=wh0KoEZXPYMGkfkeVEerSCEF1AiCZSvz9TRrx=Kj74D+Q@mail.gmail.com>
2021-05-04 8:39 ` [PATCH] io_thread/x86: don't reset 'cs', 'ss', 'ds' and 'es' registers for io_threads Peter Zijlstra
2021-05-04 15:35 ` Borislav Petkov
2021-05-04 15:55 ` Simon Marchi
2021-05-05 11:29 ` Stefan Metzmacher
2021-05-05 21:59 ` Simon Marchi
2021-05-05 22:11 ` Andy Lutomirski
2021-05-05 23:12 ` Borislav Petkov
2021-05-05 23:22 ` Andy Lutomirski
2021-05-06 1:04 ` Simon Marchi
2021-05-06 15:11 ` Andy Lutomirski
2021-05-06 9:47 ` David Laight
2021-05-06 9:53 ` David Laight
2021-05-05 22:21 ` Stefan Metzmacher [this message]
2021-05-05 23:15 ` Simon Marchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f15f0ccd-fe30-c3cf-9b01-df7ba462401f@samba.org \
--to=metze@samba.org \
--cc=axboe@kernel.dk \
--cc=bp@alien8.de \
--cc=io-uring@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-toolchains@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=peterz@infradead.org \
--cc=simon.marchi@polymtl.ca \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).