Re: [PATCH 08/12] x86/dumpstack: Pin the target stack in save_stack_trace_tsk()

[Andy Lutomirski]

On Thu, Sep 15, 2016 at 11:37 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> On Thu, Sep 15, 2016 at 11:04:47AM -0700, Andy Lutomirski wrote:
>> On Wed, Sep 14, 2016 at 11:35 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>> > On Wed, Sep 14, 2016 at 11:22:00AM -0700, Andy Lutomirski wrote:
>> >> On Wed, Sep 14, 2016 at 7:55 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>> >> > On Tue, Sep 13, 2016 at 02:29:28PM -0700, Andy Lutomirski wrote:
>> >> >> This will prevent a crash if the target task dies before or while
>> >> >> dumping its stack once we start freeing task stacks early.
>> >> >>
>> >> >> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
>> >> >
>> >> > Do we need a similar patch for show_stack()?
>> >>
>> >> Probably.  Shouldn't it go in show_stack_log_lvl() instead, though?
>> >
>> > Yeah, that would probably be better.
>>
>> This code is a colossal mess.  I really hope that, some day, we can
>> clarify which entry points are used only in dumpstack*.c and which are
>> used elsewhere.  Creating an arch/x86/kernel/dumpstack.h or just
>> merging the three files and removing all the intermediate crap from
>> the headers could help a lot.
>
> Agreed, it's a mess, though it's improving with some of my changes.
> dumpstack_32.c and dumpstack_64.c will be shrinking, with dump_trace()
> getting removed.  And they'll be shrinking even more with Linus's
> suggestion to remove show_stack_log_lvl().  Then maybe we can look at
> merging those two files into dumpstack.c with an #ifdef to separate the
> subarch differences.
>

I also wouldn't mind trying to do something to prevent ever dumping
the stack of an actively running task.  It's definitely safe to dump:

 - current

 - any task that's stopped via ptrace, etc

 - any task on the current CPU if running atomically enough that the
task can't migrate (which probably covers the nasty NMI cases, I hope)

What's *not* safe AFAIK is /proc/PID/stack.  I don't know if we can
somehow fix that short of actually sending an interrupt or NMI to
freeze the task if it's running.  I'm also not sure it's worth
worrying about it.

--Andy