On Wed, Feb 23, 2022 at 02:58:40PM +0900, Masami Hiramatsu wrote:
> Hi Jiri,
>
> On Tue, 22 Feb 2022 18:05:52 +0100
> Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> [snip]
> > +
> > +static void
> > +kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > + struct pt_regs *regs)
> > +{
> > + unsigned long saved_ip = instruction_pointer(regs);
> > + struct bpf_kprobe_multi_link *link;
> > +
> > + /*
> > + * Because fprobe's regs->ip is set to the next instruction of
> > + * dynamic-ftrace instruction, correct entry ip must be set, so
> > + * that the bpf program can access entry address via regs as same
> > + * as kprobes.
> > + */
> > + instruction_pointer_set(regs, entry_ip);
>
> This is true for the entry_handler, but false for the exit_handler,
> because entry_ip points the probed function address, not the
> return address. Thus, when this is done in the exit_handler,
> the bpf prog seems to be called from the entry of the function,
> not return.
>
> If it is what you expected, please explictly comment it to
> avoid confusion. Or, make another handler function for exit
> probing.
yes we want the ip of the function we are tracing, so it's correct,
I'll adjust the comment
>
> > +
> > + link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> > + kprobe_multi_link_prog_run(link, regs);
> > +
> > + instruction_pointer_set(regs, saved_ip);
> > +}
> > +
> > +static int
> > +kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
> > + unsigned long *addrs)
> > +{
> > + unsigned long addr, size;
> > + const char **syms;
> > + int err = -ENOMEM;
> > + unsigned int i;
> > + char *func;
> > +
> > + size = cnt * sizeof(*syms);
> > + syms = kvzalloc(size, GFP_KERNEL);
> > + if (!syms)
> > + return -ENOMEM;
> > +
> > + func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> > + if (!func)
> > + goto error;
> > +
> > + if (copy_from_user(syms, usyms, size)) {
> > + err = -EFAULT;
> > + goto error;
> > + }
> > +
> > + for (i = 0; i < cnt; i++) {
> > + err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
> > + if (err == KSYM_NAME_LEN)
> > + err = -E2BIG;
> > + if (err < 0)
> > + goto error;
> > +
> > + err = -EINVAL;
> > + if (func[0] == '\0')
> > + goto error;
> > + addr = kallsyms_lookup_name(func);
> > + if (!addr)
> > + goto error;
> > + if (!kallsyms_lookup_size_offset(addr, &size, NULL))
> > + size = MCOUNT_INSN_SIZE;
>
> Note that this is good for x86, but may not be good for other arch
> which use some preparation instructions before mcount call. Maybe you
> can just reject it if kallsyms_lookup_size_offset() fails.
I 'borrowed' this from fprobe's get_ftrace_locations function,
and it still seems to match.. do you plan to change that?
thanks,
jirka
