Re: [Xen-devel] [PATCH] x86: slightly improve stack trace on debug builds

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.12 at 17:48, Keir Fraser <keir@xxxxxxx> wrote:
> On 25/09/2012 16:07, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> +    addr = regs->eip;
>> +    while ( !is_kernel_text(addr) &&
>> +            (system_state > SYS_STATE_boot || !is_kernel_inittext(addr)) )
>> +    {
>> +        /* Special case when a bad pointer was called. */
>> +        addr ^= regs->eip ^ *ESP_BEFORE_EXCEPTION(regs);
>> +        if ( addr == regs->eip )
>> +            break;
>> +    }
> 
> Lol, how does your brain work this way? It took me 15 minutes to decode this
> to something like (also I added range checks on ESP_BEFORE_EXCEPTION(regs),
> what do you think?):

Sorry for this - just wanted to avoid having the condition evaluated
(and spelled out) twice (and it took me a minute at most to find this
iterate-at-most-twice solution). With the helper function ...

> bool_t is_current_kernel_text(unsigned long addr)
> {
>     return (is_kernel_text(addr) ||
>             (system_state == SYS_STATE_boot && is_kernel_inittext(addr)));
> }

... it at least would be reasonable to read at the source level.

So adding (and at once using at wherever else is appropriate)
that helper function is certainly worthwhile. And while I like the
way the rest was coded better than yours, I guess I ought to
change it for your and future readers' sake.

> /*
>  * If RIP is not valid hypervisor code then someone may have called into
>  * oblivion. Peek to see if they left a return address at top of stack.
>  */
> addr = (!is_current_kernel_text(regs->eip) &&
>         (ESP_BEFORE_EXCEPTION(regs) >= low) &&
>         (ESP_BEFORE_EXCEPTION(regs) < high) &&
>         is_current_kernel_text(*ESP_BEFORE_EXCEPTION(regs)))
>        ? *ESP_BEFORE_EXCEPTION(regs) : regs->eip;

Checking against "low" is pointless, as that one is guaranteed
lower than the stack pointer (unless the stack pointer was within
16 bytes from NULL). Checking against "high" is almost pointless
too, as there's only a very small range where %rsp could have
been to make that check fail (plus the 2 slots offset wouldn't be
right in this special case, i.e. we could get a false negative here),
and it wouldn't help preventing eventual #PF on the deref (since
"high" and the possible window above are on the same page).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel