Re: [PATCH 02/10] bpf: Add multi kprobe link

[Masami Hiramatsu <mhiramat@kernel.org>]

On Wed, 23 Feb 2022 18:44:33 +0100
Jiri Olsa <olsajiri@gmail.com> wrote:

> 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@kernel.org> 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?

Oops, indeed, I need to fix my code too!
Thank you! 

> 
> thanks,
> jirka


-- 
Masami Hiramatsu <mhiramat@kernel.org>