public inbox for [email protected]
 help / color / mirror / Atom feed
From: Stefan Metzmacher <[email protected]>
To: Simon Marchi <[email protected]>,
	Borislav Petkov <[email protected]>,
	Peter Zijlstra <[email protected]>
Cc: Linus Torvalds <[email protected]>,
	Andy Lutomirski <[email protected]>,
	Thomas Gleixner <[email protected]>,
	Jens Axboe <[email protected]>,
	Linux Kernel Mailing List <[email protected]>,
	io-uring <[email protected]>,
	the arch/x86 maintainers <[email protected]>,
	[email protected]
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: <[email protected]> (raw)
In-Reply-To: <[email protected]>


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

  parent reply	other threads:[~2021-05-05 22:21 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <[email protected]>
2021-05-03 16:05 ` [PATCH] io_thread/x86: don't reset 'cs', 'ss', 'ds' and 'es' registers for io_threads Andy Lutomirski
2021-05-03 19:14   ` Linus Torvalds
2021-05-03 20:15     ` Andy Lutomirski
2021-05-03 20:21       ` Andy Lutomirski
2021-05-03 20:37       ` Linus Torvalds
2021-05-03 21:26         ` Jens Axboe
2021-05-03 21:49           ` Andy Lutomirski
2021-05-03 22:08             ` Linus Torvalds
2021-05-03 22:56               ` Thomas Gleixner
2021-05-03 23:15                 ` Andy Lutomirski
2021-05-03 23:16                 ` Linus Torvalds
2021-05-03 23:19                   ` Linus Torvalds
2021-05-03 23:27                   ` Stefan Metzmacher
2021-05-03 23:48                     ` Linus Torvalds
2021-05-04  2:50                       ` Jens Axboe
2021-05-04 11:39                         ` Stefan Metzmacher
2021-05-04 15:53                           ` Linus Torvalds
2021-05-12  4:24                         ` Olivier Langlois
2021-05-12 17:44                           ` Linus Torvalds
2021-05-12 20:55                             ` Jens Axboe
2021-05-20  4:13                               ` Olivier Langlois
2021-05-21  7:31                                 ` Olivier Langlois
2021-05-25 19:39                                   ` Olivier Langlois
2021-05-25 19:45                                     ` Olivier Langlois
2021-05-25 19:52                                     ` Jens Axboe
2021-05-25 20:23                                     ` Linus Torvalds
2021-05-04  8:22                       ` David Laight
2021-05-04  0:01                   ` Andy Lutomirski
2021-05-04  8:39     ` 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
2021-04-11 15:27 Stefan Metzmacher
2021-04-14 21:28 ` Stefan Metzmacher

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 \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /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