On Sat, Nov 25, 2017 at 08:25:12PM -0800, Andy Lutomirski wrote:
> On Sat, Nov 25, 2017 at 6:40 PM, Josh Poimboeuf <jpoim...@redhat.com> 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:
Makes sense, maybe both are needed. Your patch deals with a bad SP at
the beginning and mine deals with a bad SP in the middle.
> > -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?
I think the side effects are ok, but maybe stack_access_ok() should be
renamed to make it clearer that it has side effects.
--
Josh
