Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 23 Feb 2022 18:44:33 +0100
From:   Jiri Olsa <olsajiri@gmail.com>
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     Jiri Olsa <jolsa@kernel.org>, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>, netdev@vger.kernel.org,
        bpf@vger.kernel.org, lkml <linux-kernel@vger.kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH 02/10] bpf: Add multi kprobe link
Message-ID: <YhZygR9AMtJmo1mJ@krava>
References: <20220222170600.611515-1-jolsa@kernel.org>
 <20220222170600.611515-3-jolsa@kernel.org>
 <20220223145840.64f708ed2357c89039f55f07@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220223145840.64f708ed2357c89039f55f07@kernel.org>
Precedence: bulk

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?

thanks,
jirka