[Crash-utility] [PATCH v3] x86_64_exception_frame only performs EFRAME_VERIFY if it is the only flag

[HAGIO KAZUHITO(萩尾 一仁)]

Hi David,

Thanks for the update.

> -----Original Message-----
> x86_64_exception_frame() called with combined flags including
> EFRAME_VERIFY does not perform the verify. It's only done when
> EFRAME_VERIFY is the only flag set.
> 
> Correct the condition to EFRAME_VERIFY if the flag is set. Verify
> requests are always performed. Fixes stack overrun "seek errors" seen on
> an x86_64 core when backtracing a PID at an IRQ stack where the
> interrupt handler doesn't save a pt_regs. Higher layers than the top
> frame on the IRQ stack were not displayed. Fixed by this change.
> 
> But it breaks bt -e and bt -E for exceptions on userspace stacks. Those
> use the constant 0 as the kvaddr argument to x86_64_exception_frame()
> and pass the userspace stack position in the local argument.
> x86_64_exception_frame() only verifies the kvaddr argument. Zero is not
> accessible and EFRAME_VERIFY always fails for those cases.
> 
> Modify the EFRAME_VERIFY block in x86_64_exception_frame() to choose
> kvaddr or local to verify using the same condition used to assign one of
> them to pt_regs_buf later in the same function. Add verify_addr to
> locals to hold the choice. Modify the accessible tests to use it instead
> of kvaddr. Type of the new variable is the same as the type of kvaddr.
> 
> If verifying local argument, translate to a kernel address range using
> the stackbuf and stackbase members of the bt argument the same way used
> for EFRAME_SEARCH later in x86_64_exception_frame(). local and
> bt->stackbuf are char *, the assignment destination and bt->stackbase
> are ulong. Cast the char * variables to uintptr_t for the assignment
> arithmetic using the local argument to prevent gcc 10.2 errors assigning
> char * to ulong...the sum is okay without casts in the uses for function
> arguments later.

sorry for nitpicking, but I may need some study, what errors do you see?
If you replace uintptr_t with ulong, what do you see?

I cannot see any errors with Fedora gcc 10.2 without the casts.
and uintptr_t looks same as ulong.  In the crash source code, we usually
use ulong for pointer value.

The patch logic looks good and tested OK.

Thanks,
Kazu

> 
> Signed-off-by: David Mair <dmair at suse.com>
> ---
> diff --git a/x86_64.c b/x86_64.c
> index fc05e8a..9f4b5c7 100644
> --- a/x86_64.c
> +++ b/x86_64.c
> @@ -4412,15 +4412,20 @@ x86_64_exception_frame(ulong flags, ulong kvaddr, char *local,
>          long r8, r9, r10, r11, r12, r13, r14, r15;
>  	struct machine_specific *ms;
>  	struct syment *sp;
> -	ulong offset;
> +	ulong offset, verify_addr;
>  	char *pt_regs_buf;
>  	long verified;
>  	long err;
>  	char buf[BUFSIZE];
> 
> -	if (flags == EFRAME_VERIFY) {
> -		if (!accessible(kvaddr) ||
> -		    !accessible(kvaddr + SIZE(pt_regs) - sizeof(long)))
> +	if (flags & EFRAME_VERIFY) {
> +		if (kvaddr)
> +			verify_addr = kvaddr;
> +		else
> +			verify_addr = ((uintptr_t)local - (uintptr_t)bt->stackbuf) + bt->stackbase;
> +
> +		if (!accessible(verify_addr) ||
> +		    !accessible(verify_addr + SIZE(pt_regs) - sizeof(long)))
>  			return FALSE;
>  	}
> 
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility