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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 May 2019 14:34:46 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Tengfei Fan <tengfeif@...eaurora.org>, catalin.marinas@....com,
        will.deacon@....com
Cc:     mark.rutland@....com, marc.zyngier@....com, andreyknvl@...gle.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        tengfei@...eaurora.org
Subject: Re: [PATCH] arm64: break while loop if task had been rescheduled

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@...eaurora.org>
> ---
>  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 ?