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

[David Mair]

On 9/8/20 7:25 PM, HAGIO KAZUHITO(萩尾 一仁) wrote:
> 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.

Thanks, yes, I thought of doing it using ulong casts. The language standard
offers uintptr_t for "portable" pointer arithmetic. Crash has no instances
but gdb 7.6 has nearly 300. However, it certainly doesn't matter to me which
is used and I'd expect ulong to achieve the same on supported architexctures
with the advantage of not appearing to do two type conversions (ulong through
uintptr_t from char *).

I'll have to rebuild and remind myself of the gcc specific error I see. I'll
post later with the specifics.

> The patch logic looks good and tested OK.

Thanks,
David Mair.