Re: [RFC/PATCH bpf-next 01/20] bpf: Add multi uprobe link

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Apr 24, 2023 at 03:11:20PM -0700, Alexei Starovoitov wrote:
> On Mon, Apr 24, 2023 at 06:04:28PM +0200, Jiri Olsa wrote:
> > +
> > +static int uprobe_prog_run(struct bpf_uprobe *uprobe,
> > +			   unsigned long entry_ip,
> > +			   struct pt_regs *regs)
> > +{
> > +	struct bpf_uprobe_multi_link *link = uprobe->link;
> > +	struct bpf_uprobe_multi_run_ctx run_ctx = {
> > +		.entry_ip = entry_ip,
> > +	};
> > +	struct bpf_run_ctx *old_run_ctx;
> > +	int err;
> > +
> > +	preempt_disable();
> 
> preempt_disable? Which year is this? :)
> Let's allow sleepable from the start.
> See bpf_prog_run_array_sleepable.

ok, we should probably add also 'multi.uprobe.s' section so the program
gets loaded with the BPF_F_SLEEPABLE flag.. or maybe we can enable that
by default for 'multi.uprobe' section

> 
> Other than this the set looks great.
> 
> > +
> > +	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> > +		err = 0;
> > +		goto out;
> > +	}
> > +
> > +	rcu_read_lock();
> > +	old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
> > +	err = bpf_prog_run(link->link.prog, regs);
> > +	bpf_reset_run_ctx(old_run_ctx);
> > +	rcu_read_unlock();
> > +
> > + out:
> > +	__this_cpu_dec(bpf_prog_active);
> > +	preempt_enable();
> > +	return err;
> ...
> > +			struct path path;
> > +			char *name;
> > +
> > +			name = strndup_user(upath, PATH_MAX);
> > +			if (IS_ERR(name)) {
> > +				err = PTR_ERR(name);
> > +				goto error;
> > +			}
> > +			err = kern_path(name, LOOKUP_FOLLOW, &path);
> > +			kfree(name);
> > +			if (err)
> > +				goto error;
> > +			if (!d_is_reg(path.dentry)) {
> > +				err = -EINVAL;
> > +				path_put(&path);
> > +				goto error;
> > +			}
> > +			inode = d_real_inode(path.dentry);
> > +			path_put(&path);
> 
> inode won't disappear between here and its use in uprobe_register_refctr ?

ugh, that's bug.. will fix

thanks,
jirka

> 
> > +		}
> > +		old_upath = upath;
> > +
> > +		uprobes[i].inode = inode;
> > +		uprobes[i].offset = offset;
> > +		uprobes[i].ref_ctr_offset = ref_ctr_offset;
> > +		uprobes[i].link = link;
> > +
> > +		if (flags & BPF_F_UPROBE_MULTI_RETURN)
> > +			uprobes[i].consumer.ret_handler = uprobe_multi_link_ret_handler;
> > +		else
> > +			uprobes[i].consumer.handler = uprobe_multi_link_handler;
> > +	}
> > +
> > +	link->cnt = cnt;
> > +	link->uprobes = uprobes;
> > +
> > +	bpf_link_init(&link->link, BPF_LINK_TYPE_UPROBE_MULTI,
> > +		      &bpf_uprobe_multi_link_lops, prog);
> > +
> > +	err = bpf_link_prime(&link->link, &link_primer);
> > +	if (err)
> > +		goto error;
> > +
> > +	for (i = 0; i < cnt; i++) {
> > +		err = uprobe_register_refctr(uprobes[i].inode, uprobes[i].offset,
> > +					     uprobes[i].ref_ctr_offset,
> > +					     &uprobes[i].consumer);