Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core)

[Brian Gerst]

On Fri, Jun 24, 2016 at 2:17 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Jun 23, 2016 at 12:17 PM, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> With the goal being that I'm hoping that we can then actually get rid
>> of this (at least on x86-64, even if we leave it in some other
>> architectures) in 4.8.
>
> The context here was that we could almost get rid of thread-info
> entirely, at least for x86-64, by moving it into struct task_struct.
>
> It turns out that we're not *that* far off after the obvious cleanups
> I already committed, but I couldn't get things quite to work.
>
> I'm attaching a patch that I wrote today that doesn't boot, but "looks
> right". The reason I'm attaching it is because I'm hoping somebody
> wants to take a look and maybe see what else I missed, but mostly
> because I think the patch is interesting in a couple of cases where we
> just do incredibly ugly things.
>
> First off, some code that Andy wrote when he re-organized the entry path.
>
> Oh Gods, Andy. That pt_regs_to_thread_info() thing made me want to do
> unspeakable acts on a poor innocent wax figure that looked _exactly_
> like you.
>
> I just got rid of pt_regs_to_thread_info() entirely, and just replaced
> it with current_thread_info().  I'm not at all convinced that trying
> to be that clever was really a good idea.
>
> Secondly, the x86-64 ret_from_fork calling convention was documented
> wrongly. It says %rdi contains the previous task pointer. Yes it does,
> but it doesn't mention that %r8 is supposed to contain the new
> thread_info. That was fun to find.
>
> And thirdly, the stack size games that asm/kprobes.h plays are just
> disgusting. I stared at that code for much too long. I may in fact be
> going blind as a result.
>
> The rest was fairly straightforward, although since the end result
> doesn't actually work, that "straightforward" may be broken too. But
> the basic approach _looks_ sane.
>
> Comments? Anybody want to play with this and see where I went wrong?
>
> (Note - this patch was written on top of the two thread-info removal
> patches I committed in
>
>    da01e18a37a5 x86: avoid avoid passing around 'thread_info' in stack
> dumping code
>    6720a305df74 locking: avoid passing around 'thread_info' in mutex
> debugging code
>
> and depends on them, since "ti->task" no longer exists with
> CONFIG_THREAD_INFO_IN_TASK. "ti" and "task" will have the same value).
>
>                  Linus

  * A newly forked process directly context switches into this address.
  *
  * rdi: prev task we switched from
+ * rsi: task we're switching to
  */
 ENTRY(ret_from_fork)
-    LOCK ; btr $TIF_FORK, TI_flags(%r8)
+    LOCK ; btr $TIF_FORK, TI_flags(%rsi)    /* rsi: this newly forked task */

     call    schedule_tail            /* rdi: 'prev' task parameter */

I think you forgot GET_THREAD_INFO() here.  RSI is the task, not the
thread_info.  FYI, this goes away with my switch_to() rewrite, which
removes TIF_FORK.

--
Brian Gerst