Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 04 Feb 2022 13:56:31 -0800 (PST)
Subject:     Re: [PATCH] riscv: eliminate unreliable __builtin_frame_address(1)
In-Reply-To: <20220117154433.3124-1-changbin.du@gmail.com>
CC:     Paul Walmsley <paul.walmsley@sifive.com>, aou@eecs.berkeley.edu,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        changbin.du@gmail.com
From:   Palmer Dabbelt <palmer@dabbelt.com>
To:     changbin.du@gmail.com
Message-ID: <mhng-43e63f81-4c9c-4ffa-b514-b418d21b69d4@palmer-ri-x1c9>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Mon, 17 Jan 2022 07:44:33 PST (-0800), changbin.du@gmail.com wrote:
> I tried different pieces of code which uses __builtin_frame_address(1)
> (with both gcc version 7.5.0 and 10.3.0) to verify whether it works as
> expected on riscv64. The result is negative.
>
> What the compiler had generated is as below:
> 31                      fp = (unsigned long)__builtin_frame_address(1);
>    0xffffffff80006024 <+200>:   ld      s1,0(s0)
>
> It takes '0(s0)' as the address of frame 1 (caller), but the actual address
> should be '-16(s0)'.
>
>           |       ...       | <-+
>           +-----------------+   |
>           | return address  |   |
>           | previous fp     |   |
>           | saved registers |   |
>           | local variables |   |
>   $fp --> |       ...       |   |
>           +-----------------+   |
>           | return address  |   |
>           | previous fp --------+
>           | saved registers |
>   $sp --> | local variables |
>           +-----------------+
>
> This leads the kernel can not dump the full stack trace on riscv.
>
> [    7.222126][    T1] Call Trace:
> [    7.222804][    T1] [<ffffffff80006058>] dump_backtrace+0x2c/0x3a
>
> This problem is not exposed on most riscv builds just because the '0(s0)'
> occasionally is the address frame 2 (caller's caller), if only ra and fp
> are stored in frame 1 (caller).
>
>           |       ...       | <-+
>           +-----------------+   |
>           | return address  |   |
>   $fp --> | previous fp     |   |
>           +-----------------+   |
>           | return address  |   |
>           | previous fp --------+
>           | saved registers |
>   $sp --> | local variables |
>           +-----------------+
>
> This could be a *bug* of gcc that should be fixed. But as noted in gcc
> manual "Calling this function with a nonzero argument can have
> unpredictable effects, including crashing the calling program.", let's
> remove the '__builtin_frame_address(1)' in backtrace code.
>
> With this fix now it can show full stack trace:
> [   10.444838][    T1] Call Trace:
> [   10.446199][    T1] [<ffffffff8000606c>] dump_backtrace+0x2c/0x3a
> [   10.447711][    T1] [<ffffffff800060ac>] show_stack+0x32/0x3e
> [   10.448710][    T1] [<ffffffff80a005c0>] dump_stack_lvl+0x58/0x7a
> [   10.449941][    T1] [<ffffffff80a005f6>] dump_stack+0x14/0x1c
> [   10.450929][    T1] [<ffffffff804c04ee>] ubsan_epilogue+0x10/0x5a
> [   10.451869][    T1] [<ffffffff804c092e>] __ubsan_handle_load_invalid_value+0x6c/0x78
> [   10.453049][    T1] [<ffffffff8018f834>] __pagevec_release+0x62/0x64
> [   10.455476][    T1] [<ffffffff80190830>] truncate_inode_pages_range+0x132/0x5be
> [   10.456798][    T1] [<ffffffff80190ce0>] truncate_inode_pages+0x24/0x30
> [   10.457853][    T1] [<ffffffff8045bb04>] kill_bdev+0x32/0x3c
> ...
>
> Signed-off-by: Changbin Du <changbin.du@gmail.com>
> ---
>  arch/riscv/kernel/stacktrace.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index 201ee206fb57..14d2b53ec322 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c
> @@ -22,15 +22,16 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  			     bool (*fn)(void *, unsigned long), void *arg)
>  {
>  	unsigned long fp, sp, pc;
> +	int level = 0;
>
>  	if (regs) {
>  		fp = frame_pointer(regs);
>  		sp = user_stack_pointer(regs);
>  		pc = instruction_pointer(regs);
>  	} else if (task == NULL || task == current) {
> -		fp = (unsigned long)__builtin_frame_address(1);
> -		sp = (unsigned long)__builtin_frame_address(0);
> -		pc = (unsigned long)__builtin_return_address(0);
> +		fp = (unsigned long)__builtin_frame_address(0);
> +		sp = sp_in_global;
> +		pc = (unsigned long)walk_stackframe;
>  	} else {
>  		/* task blocked in __switch_to */
>  		fp = task->thread.s[0];
> @@ -42,7 +43,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  		unsigned long low, high;
>  		struct stackframe *frame;
>
> -		if (unlikely(!__kernel_text_address(pc) || !fn(arg, pc)))
> +		if (unlikely(!__kernel_text_address(pc) || (level++ >= 1 && !fn(arg, pc))))
>  			break;
>
>  		/* Validate frame pointer */

Thanks, this is on fixes.