Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430023 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-23 19:50 +0200
    Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-23 20:00 +0200
      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Kees Cook <keescook@chromium.org> - 2016-06-23 20:10 +0200
        Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Peter Zijlstra <peterz@infradead.org> - 2016-06-23 21:00 +0200
      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Oleg Nesterov <oleg@redhat.com> - 2016-06-23 20:20 +0200
        Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Peter Zijlstra <peterz@infradead.org> - 2016-06-23 21:00 +0200
      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-23 20:50 +0200
        Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-23 21:10 +0200
      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Peter Zijlstra <peterz@infradead.org> - 2016-06-23 21:00 +0200
        Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-23 21:20 +0200
          Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-24 08:20 +0200
            Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Brian Gerst <brgerst@gmail.com> - 2016-06-24 14:30 +0200
              Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-24 19:30 +0200
                Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-24 19:50 +0200
                  Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-24 19:50 +0200
                    Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-24 20:00 +0200
                      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-24 20:40 +0200
                  Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-24 20:00 +0200
                    Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-24 20:20 +0200
                      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-24 23:00 +0200
                      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-24 23:00 +0200
                        Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-24 23:10 +0200
                          Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-24 23:30 +0200
                            Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-24 23:40 +0200
                              Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-24 23:40 +0200
                                Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-25 04:50 +0200
                                  Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-26 01:30 +0200
                                    Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-26 01:40 +0200
                                      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-26 03:30 +0200
                      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-24 23:10 +0200
        Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-23 21:20 +0200
          Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Peter Zijlstra <peterz@infradead.org> - 2016-06-23 21:20 +0200
    Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Oleg Nesterov <oleg@redhat.com> - 2016-06-23 21:00 +0200
      Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Michal Hocko <mhocko@kernel.org> - 2016-06-24 16:20 +0200
        Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Michal Hocko <mhocko@kernel.org> - 2016-06-24 17:10 +0200
          Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages  (x86, core) Oleg Nesterov <oleg@redhat.com> - 2016-06-24 22:30 +0200
            Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86,  core) Michal Hocko <mhocko@kernel.org> - 2016-06-27 12:40 +0200

Page 1 of 2  [1] 2  Next page →

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

On Thu, Jun 23, 2016 at 10:03 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Let me quote my previous email ;)
>
>         And we can't free/nullify it when the parent/debuger reaps a zombie,
>         say, mark_oom_victim() expects that get_task_struct() protects
>         thread_info as well.
>
> probably we can fix all such users though...

TIF_MEMDIE is indeed a potential problem, but I don't think
mark_oom_victim() is actually problematic.

mark_oom_victim() is called with either "current", or with a victim
that still has its mm and signal pointer (and the task is locked). So
the lifetime is already guaranteed - or that code is already very very
buggy, since it follows tsk->signal and tsk->mm

So as far as I can tell, that's all fine.

That said, by now it would actually in many ways be great if we could
get rid of thread_info entirely. The historical reasons for
thread_info have almost all been subsumed by the percpu area.

The reason for thread_info originally was

 - we used to find the task_struct by just masking the stack pointer
(long long ago). When the task struct grew too big, we kept just the
critical pieces and some arch-specific stuff and , called it
"thread_info", and moved the rest to an external allocation and added
the pointer to it.

 - the really crticial stuff we didn't want to follow a pointer for,
so things like preempt_count etc were in thread_info

 - but they were *so* critical that PeterZ (at my prodding) moved
those things to percpu caches that get updated at schedule time
instead

so these days, thread_info has almost nothing really critical in it
any more. There's the thread-local flags, yes, but they could stay or
easily be moved to the task_struct or get similar per-cpu fixup as
preempt_count did a couple of years ago. The only annoyance is the few
remaining entry code assembly sequences, but I suspect they would
actually become simpler with a per-cpu thing, and with Andy's cleanups
they are pretty insignificant these days. There seems to be exactly
two uses of ASM_THREAD_INFO(TI_flags,.. left.

So I suspect that it would

 (a) already be possible to just free the stack and thread info at
release time, because any rcu users will already be doing task_lock()
and check mm etc.

 (b) it probably would be a nice cleanup to try to make it even more
obviously safe by just shrinking thread_info more (or even getting rid
of it entirely, but that may be too painful because there are other
architectures that may depend on it more).

I dunno. Looking at what remains of thread_info, it really doesn't
seem very critical.

The thread_info->tsk pointer, that was one of the most critical issues
and the main raison d'être of the thread_info, has been replaced on
x86 by just using the per-cpu "current_task". Yes,.there are probably
more than a few "ti->task" users left for legacy reasons, harking back
to when the thread-info was cheaper to access, but it shouldn't be a
big deal.

                  Linus

[toc] | [next] | [standalone]

#1430029

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jun 23, 2016 at 10:44 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The thread_info->tsk pointer, that was one of the most critical issues
> and the main raison d'être of the thread_info, has been replaced on
> x86 by just using the per-cpu "current_task". Yes,.there are probably
> more than a few "ti->task" users left for legacy reasons, harking back
> to when the thread-info was cheaper to access, but it shouldn't be a
> big deal.

Ugh. Looking around at this, it turns out that a great example of this
kind of legacy issue is the debug_mutex stuff.

It uses "struct thread_info *" as the owner pointer, and there is _no_
existing reason for it. In fact, in every single place it actually
wants the task_struct, and it does task_thread_info(task) just to
convert it to the thread-info, and then converts it back with
"ti->task".

So the attached patch seems to be the right thing to do regardless of
this whole discussion.

                   Linus

[toc] | [prev] | [next] | [standalone]

#1430030

On Thu, Jun 23, 2016 at 10:52 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jun 23, 2016 at 10:44 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> The thread_info->tsk pointer, that was one of the most critical issues
>> and the main raison d'être of the thread_info, has been replaced on
>> x86 by just using the per-cpu "current_task". Yes,.there are probably
>> more than a few "ti->task" users left for legacy reasons, harking back
>> to when the thread-info was cheaper to access, but it shouldn't be a
>> big deal.
>
> Ugh. Looking around at this, it turns out that a great example of this
> kind of legacy issue is the debug_mutex stuff.
>
> It uses "struct thread_info *" as the owner pointer, and there is _no_
> existing reason for it. In fact, in every single place it actually
> wants the task_struct, and it does task_thread_info(task) just to
> convert it to the thread-info, and then converts it back with
> "ti->task".

Heh, yeah, that looks like a nice clean-up.

> So the attached patch seems to be the right thing to do regardless of
> this whole discussion.

Why does __mutex_lock_common() have "task" as a stack variable? It's
only assigned at the start, and is always "current". (I only noticed
from the patch changing "current_thread_info()" and
"task_thread_info(task)" both to "task".)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

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

On Thu, Jun 23, 2016 at 11:00:08AM -0700, Kees Cook wrote:
> 
> Why does __mutex_lock_common() have "task" as a stack variable?

That's actually a fairly common thing to do. The reason is that
'current' is far more expensive to evaluate than a local variable.

[toc] | [prev] | [next] | [standalone]

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

On 06/23, Linus Torvalds wrote:
>
> Ugh. Looking around at this, it turns out that a great example of this
> kind of legacy issue is the debug_mutex stuff.

Heh ;) I am looking at it too.

> It uses "struct thread_info *" as the owner pointer, and there is _no_
> existing reason for it. In fact, in every single place it actually
> wants the task_struct, and it does task_thread_info(task) just to
> convert it to the thread-info, and then converts it back with
> "ti->task".

Even worse, this task is always "current" afaics, so

> So the attached patch seems to be the right thing to do regardless of
> this whole discussion.

I think we should simply remove this argument.

And probably kill task_struct->blocked_on? I do not see the point of
this task->blocked_on != waiter check.

Oleg.

[toc] | [prev] | [next] | [standalone]

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

On Thu, Jun 23, 2016 at 08:12:16PM +0200, Oleg Nesterov wrote:
> 
> And probably kill task_struct->blocked_on? I do not see the point of
> this task->blocked_on != waiter check.

I think that came about because of PI and or deadlock detection. Of
course, the current mutex code doesn't have anything like that these
days, and rt_mutex has task_struct::pi_blocked_on.

[toc] | [prev] | [next] | [standalone]

#1430047

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jun 23, 2016 at 10:52 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Ugh. Looking around at this, it turns out that a great example of this
> kind of legacy issue is the debug_mutex stuff.

Interestingly, the *only* other user of ti->task for a full
allmodconfig build of x86-64 seems to be

  arch/x86/kernel/dumpstack.c

with the print_context_stack() -> print_ftrace_graph_addr() -> task =
tinfo->task chain.

And that doesn't really seem to want thread_info either. The callers
all have 'task', and have to generate thread_info from that anyway.

So this attached patch (which includes the previous one) seems to
build. I didn't actually boot it, but there should be no users left
unless there is some asm code that has hardcoded offsets..

                 Linus

[toc] | [prev] | [next] | [standalone]

#1430064

On Thu, Jun 23, 2016 at 11:46 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jun 23, 2016 at 10:52 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Ugh. Looking around at this, it turns out that a great example of this
>> kind of legacy issue is the debug_mutex stuff.
>
> Interestingly, the *only* other user of ti->task for a full
> allmodconfig build of x86-64 seems to be
>
>   arch/x86/kernel/dumpstack.c
>
> with the print_context_stack() -> print_ftrace_graph_addr() -> task =
> tinfo->task chain.
>
> And that doesn't really seem to want thread_info either. The callers
> all have 'task', and have to generate thread_info from that anyway.
>
> So this attached patch (which includes the previous one) seems to
> build. I didn't actually boot it, but there should be no users left
> unless there is some asm code that has hardcoded offsets..

I think you'll break some architectures when you remove the
initialization of ti->task.  That either needs to be pushed down into
arch code in unicore32, openrisc, microblaze, powerpx, xtensa, sparc,
parisc, arm, mips, s390, and whatever I missed, or you should leave
the field initialized and existing and wait for my patch to
conditionally remove/embed thread_info to get rid of the
initialization part.

On the C side, there's:

arm's contextidr_notifier (easily fixable)

sh's irqctx->tinfo.task = curctx->task; (probably useless) and
print_ftrace_graph_addr

cris's ugdb_trap_user (probably easily fixable)

sparc's arch_trigger_all_cpu_backtrace (possibly quite hard to fix)

sparc's flush_thread (trivial)

sparc's __save_stack_trace (not sure)

unicore's __die (probably easy)

metag's do_softirq_own_stack (not sure if it's useful)


I found these with this coccinelle script:

@@
struct thread_info *ti;
@@

* ti->task

$ spatch --sp-file titask.cocci --dir .

[toc] | [prev] | [next] | [standalone]

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

On Thu, Jun 23, 2016 at 10:52:58AM -0700, Linus Torvalds wrote:
> Ugh. Looking around at this, it turns out that a great example of this
> kind of legacy issue is the debug_mutex stuff.
> 
> It uses "struct thread_info *" as the owner pointer, and there is _no_
> existing reason for it. In fact, in every single place it actually
> wants the task_struct, and it does task_thread_info(task) just to
> convert it to the thread-info, and then converts it back with
> "ti->task".
> 
> So the attached patch seems to be the right thing to do regardless of
> this whole discussion.

Yeah, that looks fine. Want me to take it or will you just commit?

[toc] | [prev] | [next] | [standalone]

#1430068

On Thu, Jun 23, 2016 at 11:53 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> So the attached patch seems to be the right thing to do regardless of
>> this whole discussion.
>
> Yeah, that looks fine. Want me to take it or will you just commit?

I'm committing these trivial non-semantic patches, I'm actually
running the kernel without any ti->task pointer now (the previous
patch I sent out).

So I'll do the mutex debug patch and the stack dump patch as just he
obvious cleanup patches.

Those are the "purely legacy reasons for a bad calling convention",
and I'm ok with those during the rc series to make it easier for
people to play around with this.

With he 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.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1430391

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jun 23, 2016 at 12:17 PM, Linus Torvalds
<torvalds@linux-foundation.org> 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

[toc] | [prev] | [next] | [standalone]

#1430629

On Fri, Jun 24, 2016 at 2:17 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jun 23, 2016 at 12:17 PM, Linus Torvalds
> <torvalds@linux-foundation.org> 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

[toc] | [prev] | [next] | [standalone]

#1430828

On Fri, Jun 24, 2016 at 5:25 AM, Brian Gerst <brgerst@gmail.com> wrote:
>   *
>   * 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.

The point of that patch series is to make the thread_info and the
task_struct pointer have the same value on x86 - we hide the
thread_info inside the task_struct itself, and in fact at the
beginning of it.

That allows for the above kinds of simplification - use the task
struct pointer and thread info interchangably in the asm code.

But as mentioned, I must have missed something. There were a number of
places where the code used the task_stack_page() and
task_thread_info() interchangably, which used to work and is no longer
true. There might simply be cases I missed.

Or there might simply be cases I screwed up.

                Linus

[toc] | [prev] | [next] | [standalone]

#1430833

On Fri, Jun 24, 2016 at 10:21 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But as mentioned, I must have missed something. There were a number of
> places where the code used the task_stack_page() and
> task_thread_info() interchangably, which used to work and is no longer
> true. There might simply be cases I missed.

.. and immediately as I wrote that, I went "Duh".

One place I missed was free_thread_info(), which should now free the
stack, not the ti pointer. But it does

        struct page *page = virt_to_page(ti);

and frees that, which is bogus. It turns out that we do do

        free_thread_info(tsk->stack);

which is bogus too, and undoes it, but I think I have a few new places
to look at..

                Linus

[toc] | [prev] | [next] | [standalone]

#1430839

On Fri, Jun 24, 2016 at 10:40 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jun 24, 2016 at 10:21 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> But as mentioned, I must have missed something. There were a number of
>> places where the code used the task_stack_page() and
>> task_thread_info() interchangably, which used to work and is no longer
>> true. There might simply be cases I missed.
>
> .. and immediately as I wrote that, I went "Duh".
>
> One place I missed was free_thread_info(), which should now free the
> stack, not the ti pointer. But it does
>
>         struct page *page = virt_to_page(ti);
>
> and frees that, which is bogus. It turns out that we do do
>
>         free_thread_info(tsk->stack);
>
> which is bogus too, and undoes it, but I think I have a few new places
> to look at..

Try patching in this thing, which cleans up a bunch of that core crap:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=x86/vmap_stack&id=7ca9fb6bbf2838cc94b2af41e94854d02649c58c

It might not apply without the rest of my series, though.

FWIW, your patch is much more lenient than my approach: I was planning
prohibiting architectures from supplying their own struct thread_info
if they put it in task_struct.  To make that work, I have patches to
remove everything but cpu, flags, and task from x86's thread_info
first.  I'm planning on tidying them up and sending them out after the
vmap stack stuff lands in -tip -- I don't want to have big series that
depend on each other flying around by email at the same time, because
everyone will go nuts trying to figure out what applies where.

--Andy

[toc] | [prev] | [next] | [standalone]

#1430845

On Fri, Jun 24, 2016 at 10:47 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> FWIW, your patch is much more lenient than my approach.

I hate big flag-days - they cause so much pain for everybody. The
people who get it to work and can test it, can't test all the other
cases (whether they be drivers or other architectures), so I'd much
rather implement something that allows a gradual per-architecture
change from having the thread_info on the stack into having the
thread_info in the task_struct.

Bit "let's just change everything at once" patches are fine (and, in
fact, preferable) when you can test everything in one go. So something
that can be statically verified (ie "patch makes no semantic
difference, but changes calling convention or naming, so if it
compiles it is fine") I much prefer just getting the pain over and
done with rather than some lingering thing.

But when it's something where "oops, I broke every other architecture,
and I can't even test it", I'd much rather do it in a way where each
architecture can move over to the new model one by one.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1430878

On Fri, Jun 24, 2016 at 10:56 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jun 24, 2016 at 10:47 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> FWIW, your patch is much more lenient than my approach.
>
> I hate big flag-days - they cause so much pain for everybody. The
> people who get it to work and can test it, can't test all the other
> cases (whether they be drivers or other architectures), so I'd much
> rather implement something that allows a gradual per-architecture
> change from having the thread_info on the stack into having the
> thread_info in the task_struct.
>
> Bit "let's just change everything at once" patches are fine (and, in
> fact, preferable) when you can test everything in one go. So something
> that can be statically verified (ie "patch makes no semantic
> difference, but changes calling convention or naming, so if it
> compiles it is fine") I much prefer just getting the pain over and
> done with rather than some lingering thing.
>
> But when it's something where "oops, I broke every other architecture,
> and I can't even test it", I'd much rather do it in a way where each
> architecture can move over to the new model one by one.

Agreed.

To clarify, though: I wasn't planning on changing all arches at once.
 I'm just saying that, for arches that switch over, they get a single
core definition of thread_info.  That way, when someone (probably
named Peter) decides down the road to move, say, thread_info::cpu into
task_struct proper to optimize cache line layout, they won't need to
do it for every architecture.

Also, I want to give people an incentive to finally move their crap
out of struct thread_info and into struct thread_struct.

--Andy

[toc] | [prev] | [next] | [standalone]

#1430847

On Fri, Jun 24, 2016 at 10:40 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think I have a few new places to look at..

Yeah, making the 'stack' pointer be 'unsigned long *' rather than
'void *' shows a number of places where we were very confused about
things.

And in particular, the init_task stack initialization initialized it
to the init_thread pointer. Which was definitely deadly.

Let's see if that was it..

                  Linus

[toc] | [prev] | [next] | [standalone]

#1430856

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jun 24, 2016 at 10:51 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And in particular, the init_task stack initialization initialized it
> to the init_thread pointer. Which was definitely deadly.
>
> Let's see if that was it..

No, it's still broken. But it's *less* broken, so here's a new version
of the patch that at least gets some of the stack setup right, in my
hope that somebody will bother to look at this, and being less broken
might mean that somebody sees what else I missed..

                   Linus

[toc] | [prev] | [next] | [standalone]

#1430949

On Fri, Jun 24, 2016 at 1:51 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Jun 24, 2016 at 03:25:30PM -0500, Josh Poimboeuf wrote:
>> On Fri, Jun 24, 2016 at 11:11:47AM -0700, Linus Torvalds wrote:
>> > On Fri, Jun 24, 2016 at 10:51 AM, Linus Torvalds
>> > <torvalds@linux-foundation.org> wrote:
>> > >
>> > > And in particular, the init_task stack initialization initialized it
>> > > to the init_thread pointer. Which was definitely deadly.
>> > >
>> > > Let's see if that was it..
>> >
>> > No, it's still broken. But it's *less* broken, so here's a new version
>> > of the patch that at least gets some of the stack setup right, in my
>> > hope that somebody will bother to look at this, and being less broken
>> > might mean that somebody sees what else I missed..
>>
>> I found at least one bug.  The changing of task->stack from a "void *" to an
>> "unsigned long *":
>>
>> > -   void *stack;
>> > +   unsigned long *stack;
>>
>> That subtly changes the pointer arithmetic in do_boot_cpu():
>>
>>
>>       idle->thread.sp = (unsigned long) (((struct pt_regs *)
>>                         (THREAD_SIZE +  task_stack_page(idle))) - 1);
>>
>>
>> That ends up adding 128k to the stack page bottom instead of 16k.
>>
>> But fixing that doesn't seem to fix this:
>>
>> [18446743832.576241] ------------[ cut here ]------------
>> [18446743832.576241] WARNING: CPU: 1 PID: 0 at /home/jpoimboe/git/linux/arch/x86/kernel/cpu/common.c:1434 cpu_init+0x34b/0x440
>> [18446743832.576241] Modules linked in:
>> [18446743832.576241] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.7.0-rc4+ #47
>> [18446743832.576241] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
>> [18446743832.576241]  0000000000000086 574e5e6c6855ace9 ffff88007c553e88 ffffffff8143cb83
>> [18446743832.576241]  0000000000000000 0000000000000000 ffff88007c553ec8 ffffffff810b0e7b
>> [18446743832.576241]  0000059a00000000 0000000000000000 0000000000000000 0000000000000000
>> [18446743832.576241] Call Trace:
>> [18446743832.576241]  [<ffffffff8143cb83>] dump_stack+0x85/0xc2
>> [18446743832.576241]  [<ffffffff810b0e7b>] __warn+0xcb/0xf0
>> [18446743832.576241]  [<ffffffff810b0fad>] warn_slowpath_null+0x1d/0x20
>> [18446743832.576241]  [<ffffffff810491bb>] cpu_init+0x34b/0x440
>> [18446743832.576241]  [<ffffffff8105ab7c>] start_secondary+0x1c/0x1a0
>> [18446743832.576241] ---[ end trace 924d57afbaca0720 ]---
>>
>> So there's at least another bug lurking..
>
> Found another bug:
>
> #define stack_smp_processor_id()                                        \
> ({                                                              \
>         struct thread_info *ti;                                         \
>         __asm__("andq %%rsp,%0; ":"=r" (ti) : "0" (CURRENT_MASK));      \
>         ti->cpu;                                                        \
> })
>
> That macro is obviously no longer valid.
>
> That seems to cause the above warning.  When trying to boot CPU 1,
> cpu_init() calls the above macro which incorrectly returns 0.

Fixed in my queue by removing the function:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=x86/vmap_stack&id=01b1a4b6fd629820625b64ca6e17c987f2ee8c09



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web