On Thu, Jan 24, 2019 at 04:19:39PM +0000, Christophe Leroy wrote:
> [text copied from commit 9bbd4c56b0b6
> ("arm64: prep stack walkers for THREAD_INFO_IN_TASK")]
>
> When CONFIG_THREAD_INFO_IN_TASK is selected, task stacks may be freed
> before a task is destroyed. To account for this, the stacks are
> refcounted, and when manipulating the stack of another task, it is
> necessary to get/put the stack to ensure it isn't freed and/or re-used
> while we do so.
>
> This patch reworks the powerpc stack walking code to account for this.
> When CONFIG_THREAD_INFO_IN_TASK is not selected these perform no
> refcounting, and this should only be a structural change that does not
> affect behaviour.
>
> Signed-off-by: Christophe Leroy <christophe.le...@c-s.fr>
I'm not familiar with the powerpc code, but AFAICT this is analagous to
the arm64 code, and I'm not aware of any special caveats. FWIW:
Acked-by: Mark Rutland <mark.rutl...@arm.com>
Thanks,
Mark.
> ---
> arch/powerpc/kernel/process.c | 23 +++++++++++++++++++++--
> arch/powerpc/kernel/stacktrace.c | 29 ++++++++++++++++++++++++++---
> 2 files changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index ce393df243aa..4ffbb677c9f5 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -2027,7 +2027,7 @@ int validate_sp(unsigned long sp, struct task_struct *p,
>
> EXPORT_SYMBOL(validate_sp);
>
> -unsigned long get_wchan(struct task_struct *p)
> +static unsigned long __get_wchan(struct task_struct *p)
> {
> unsigned long ip, sp;
> int count = 0;
> @@ -2053,6 +2053,20 @@ unsigned long get_wchan(struct task_struct *p)
> return 0;
> }
>
> +unsigned long get_wchan(struct task_struct *p)
> +{
> + unsigned long ret;
> +
> + if (!try_get_task_stack(p))
> + return 0;
> +
> + ret = __get_wchan(p);
> +
> + put_task_stack(p);
> +
> + return ret;
> +}
> +
> static int kstack_depth_to_print = CONFIG_PRINT_STACK_DEPTH;
>
> void show_stack(struct task_struct *tsk, unsigned long *stack)
> @@ -2067,6 +2081,9 @@ void show_stack(struct task_struct *tsk, unsigned long
> *stack)
> int curr_frame = 0;
> #endif
>
> + if (!try_get_task_stack(tsk))
> + return;
> +
> sp = (unsigned long) stack;
> if (tsk == NULL)
> tsk = current;
> @@ -2081,7 +2098,7 @@ void show_stack(struct task_struct *tsk, unsigned long
> *stack)
> printk("Call Trace:\n");
> do {
> if (!validate_sp(sp, tsk, STACK_FRAME_OVERHEAD))
> - return;
> + break;
>
> stack = (unsigned long *) sp;
> newsp = stack[0];
> @@ -2121,6 +2138,8 @@ void show_stack(struct task_struct *tsk, unsigned long
> *stack)
>
> sp = newsp;
> } while (count++ < kstack_depth_to_print);
> +
> + put_task_stack(tsk);
> }
>
> #ifdef CONFIG_PPC64
> diff --git a/arch/powerpc/kernel/stacktrace.c
> b/arch/powerpc/kernel/stacktrace.c
> index e2c50b55138f..a5745571e06e 100644
> --- a/arch/powerpc/kernel/stacktrace.c
> +++ b/arch/powerpc/kernel/stacktrace.c
> @@ -67,12 +67,17 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct
> stack_trace *trace)
> {
> unsigned long sp;
>
> + if (!try_get_task_stack(tsk))
> + return;
> +
> if (tsk == current)
> sp = current_stack_pointer();
> else
> sp = tsk->thread.ksp;
>
> save_context_stack(trace, sp, tsk, 0);
> +
> + put_task_stack(tsk);
> }
> EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
>
> @@ -84,9 +89,8 @@ save_stack_trace_regs(struct pt_regs *regs, struct
> stack_trace *trace)
> EXPORT_SYMBOL_GPL(save_stack_trace_regs);
>
> #ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
> -int
> -save_stack_trace_tsk_reliable(struct task_struct *tsk,
> - struct stack_trace *trace)
> +static int __save_stack_trace_tsk_reliable(struct task_struct *tsk,
> + struct stack_trace *trace)
> {
> unsigned long sp;
> unsigned long stack_page = (unsigned long)task_stack_page(tsk);
> @@ -193,6 +197,25 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk,
> }
> return 0;
> }
> +
> +int save_stack_trace_tsk_reliable(struct task_struct *tsk,
> + struct stack_trace *trace)
> +{
> + int ret;
> +
> + /*
> + * If the task doesn't have a stack (e.g., a zombie), the stack is
> + * "reliably" empty.
> + */
> + if (!try_get_task_stack(tsk))
> + return 0;
> +
> + ret = __save_stack_trace_tsk_reliable(trace, tsk);
> +
> + put_task_stack(tsk);
> +
> + return ret;
> +}
> EXPORT_SYMBOL_GPL(save_stack_trace_tsk_reliable);
> #endif /* CONFIG_HAVE_RELIABLE_STACKTRACE */
>
> --
> 2.13.3
>
