Re: [RFC PATCH 10/10] x86/unwind: add undwarf unwinder

From: Peter Zijlstra
Date: Thu Jun 01 2017 - 07:05:57 EST


On Thu, Jun 01, 2017 at 12:44:16AM -0500, Josh Poimboeuf wrote:

> +static struct undwarf *__undwarf_lookup(struct undwarf *undwarf,
> + unsigned int num, unsigned long ip)
> +{
> + struct undwarf *first = undwarf;
> + struct undwarf *last = undwarf + num - 1;
> + struct undwarf *mid;
> + unsigned long u_ip;
> +
> + while (first <= last) {
> + mid = first + ((last - first) / 2);
> + u_ip = undwarf_ip(mid);
> +
> + if (ip >= u_ip) {
> + if (ip < u_ip + mid->len)
> + return mid;
> + first = mid + 1;
> + } else
> + last = mid - 1;
> + }
> +
> + return NULL;
> +}

That's a bog standard binary search thing, don't we have a helper for
that someplace?

> +static struct undwarf *undwarf_lookup(unsigned long ip)
> +{
> + struct undwarf *undwarf;
> + struct module *mod;
> +
> + /* Look in vmlinux undwarf section: */
> + undwarf = __undwarf_lookup(__undwarf_start, __undwarf_end - __undwarf_start, ip);
> + if (undwarf)
> + return undwarf;
> +
> + /* Look in module undwarf sections: */
> + preempt_disable();
> + mod = __module_address(ip);
> + if (!mod || !mod->arch.undwarf)
> + goto module_out;
> + undwarf = __undwarf_lookup(mod->arch.undwarf, mod->arch.num_undwarves, ip);
> +
> +module_out:
> + preempt_enable();
> + return undwarf;
> +}

A few points here:

- that first lookup is entirely pointless if !core_kernel_text(ip)

- that preempt_{dis,en}able() muck is 'pointless', for while it shuts
up the warnings from __modules_address(), nothing preserves the
struct undwarf you get a pointer to after the preempt_enable().

- what about 'interesting' things like, ftrace_trampoline, kprobe insn
slots and bpf text?

> +static bool stack_access_ok(struct unwind_state *state, unsigned long addr,
> + size_t len)
> +{
> + struct stack_info *info = &state->stack_info;
> +
> + /*
> + * If the next bp 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 next bp
> + * 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;
}
> +
> + return true;
> +}