On Sat, Nov 25, 2017 at 6:40 PM, Josh Poimboeuf <[email protected]> wrote:
> On Sat, Nov 25, 2017 at 04:16:23PM -0800, Andy Lutomirski wrote:
>> Can you send me whatever config and exact commit hash generated this?
>> I can try to figure out why it failed.
>
> Sorry, I've been traveling. I just got some time to take a look at
> this. I think there are at least two unwinder issues here:
>
> - It doesn't deal gracefully with the case where the stack overflows and
> the stack pointer itself isn't on a valid stack but the
> to-be-dereferenced data *is*.
>
> - The oops dump code doesn't know how to print partial pt_regs, for the
> case where if we get an interrupt/exception in *early* entry code
> before the full pt_regs have been saved.
>
> (Andy, I'm not quite sure about your patch, and whether it's still
> needed after these patches. I'll need to look at it later when I have
> more time.)
I haven't tested yet, but I think my patch is probably still needed.
The issue I fixed is that unwind_start() would bail out early if sp
was below the stack. Also:
> -static bool stack_access_ok(struct unwind_state *state, unsigned long addr,
> +static bool stack_access_ok(struct unwind_state *state, unsigned long _addr,
> size_t len)
> {
> struct stack_info *info = &state->stack_info;
> + void *addr = (void *)_addr;
>
> - /*
> - * If the address isn't on the current stack, switch to the next one.
> - *
> - * We may have to traverse multiple stacks to deal with the
> possibility
> - * that info->next_sp could point to an empty stack and the address
> - * could be on a subsequent stack.
> - */
> - while (!on_stack(info, (void *)addr, len))
> - if (get_stack_info(info->next_sp, state->task, info,
> - &state->stack_mask))
> - return false;
> + if (!on_stack(info, addr, len) &&
> + (get_stack_info(addr, state->task, info, &state->stack_mask)))
> + return false;
>
> return true;
> }
This looks odd to me before and after. Shouldn't this be side-effect
free? That is, shouldn't it create a copy of info and stack_mask and
point that to get_stack_info() rather than allowing get_stack_info()
to modify the unwind state?
