Re: [PATCH] arm64: break while loop if task had been rescheduled

From: Anshuman Khandual
Date: Wed May 22 2019 - 05:07:17 EST


On 05/21/2019 02:50 PM, Tengfei Fan wrote:
> While printing a task's backtrace and this task isn't
> current task, it is possible that task's fp and fp+8
> have the same value, so cannot break the while loop.
> This can break while loop if this task had been
> rescheduled during print this task's backtrace.

This is very confusing. IIUC it suggests that while printing
the backtrace for non-current tasks the do/while loop does not
exit because fp and fp+8 might have the same value ? When would
this happen ? Even in that case the commit message here does not
properly match the change in this patch.

This patch tries to stop printing the stack for non-current tasks
if their state change while there is one dump_backtrace() trying
to print back trace. Dont we have any lock preventing a task in
this situation (while dumping it's backtrace) from running again
or changing state.

>
> Signed-off-by: Tengfei Fan <tengfeif@xxxxxxxxxxxxxx>
> ---
> arch/arm64/kernel/traps.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 2975598..9df6e02 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -103,6 +103,9 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> {
> struct stackframe frame;
> int skip = 0;
> + long cur_state = 0;
> + unsigned long cur_sp = 0;
> + unsigned long cur_fp = 0;
>
> pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>
> @@ -127,6 +130,9 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> */
> frame.fp = thread_saved_fp(tsk);
> frame.pc = thread_saved_pc(tsk);
> + cur_state = tsk->state;
> + cur_sp = thread_saved_sp(tsk);
> + cur_fp = frame.fp;

Should 'saved_state|sp|fp' instead as its applicable to non-current
tasks only.

> }
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> frame.graph = 0;
> @@ -134,6 +140,23 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>
> printk("Call trace:\n");
> do {
> + if (tsk != current && (cur_state != tsk->state
> + /*
> + * We would not be printing backtrace for the task
> + * that has changed state from uninterruptible to
> + * running before hitting the do-while loop but after
> + * saving the current state. If task is in running

This does not check any explicit task states like 'un-interruptible' or
'running' but instead tracks change from any previously 'saved' state.


> + * state before saving the state, then we may print
> + * wrong call trace or end up in infinite while loop
> + * if *(fp) and *(fp+8) are same. While the situation

Then dump_backtrace() must detect it, should not save it and just abort.


> + * will stop print when that task schedule out.

Thats not a reliable solution. AFICS we should not proceed further if
there is a chance of an wrong trace or an infinite loop. Hoping that
the printing will stop when task gets scheduled out does not seem right.

> + */
> + || cur_sp != thread_saved_sp(tsk)
> + || cur_fp != thread_saved_fp(tsk))) {

Why does any of these three mismatches detect the problematic transition
not just the state ?