Re: [PATCH] x86/orc: Don't bail on stack overflow

[Andy Lutomirski]

On Sat, Nov 25, 2017 at 6:40 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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?