Re: [PATCHv6 bpf-next 03/28] bpf: Add multi uprobe link

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Aug 04, 2023 at 02:55:29PM -0700, Yonghong Song wrote:

SNIP

> > +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_prog *prog = link->link.prog;
> > +	bool sleepable = prog->aux->sleepable;
> > +	struct bpf_run_ctx *old_run_ctx;
> > +	int err = 0;
> > +
> > +	might_fault();
> 
> Could you explain what you try to protect here
> with might_fault()?
> 
> In my opinion, might_fault() is unnecessary here
> since the calling context is process context and
> there is no mmap_lock held, so might_fault()
> won't capture anything.
> 
> might_fault() is used in iter.c and trampoline.c
> since their calling context is more complex
> than here and might_fault() may actually capture
> issues.

hum, I followed bpf_prog_run_array_sleepable, which is called
the same way.. will check

> 
> > +
> > +	migrate_disable();
> > +
> > +	if (sleepable)
> > +		rcu_read_lock_trace();
> > +	else
> > +		rcu_read_lock();
> 
> Looking at trampoline.c and iter.c, typical
> usage is
> 	rcu_read_lock_trace()/rcu_read_lock()
> 	migrate_disable()
> 
> Your above sequenence could be correct too. But it
> is great if we can keep consistency here.

ok, will switch that

SNIP

> > +	link->cnt = cnt;
> > +	link->uprobes = uprobes;
> > +	link->path = path;
> > +
> > +	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_free;
> > +
> > +	for (i = 0; i < cnt; i++) {
> > +		err = uprobe_register_refctr(d_real_inode(link->path.dentry),
> > +					     uprobes[i].offset,
> > +					     ref_ctr_offsets ? ref_ctr_offsets[i] : 0,
> > +					     &uprobes[i].consumer);
> > +		if (err) {
> > +			bpf_uprobe_unregister(&path, uprobes, i);
> > +			bpf_link_cleanup(&link_primer);
> > +			kvfree(ref_ctr_offsets);
> 
> Is it possible we may miss some of below 'error_free' cleanups?
> In my opinion, we should replace
> 			kvfree(ref_ctr_offsets);
> 			return err;
> with
> 			goto error_free;
> 
> Could you double check?

the problem here is that bpf_link_cleanup calls link's dealloc callback,
so it get's released in bpf_uprobe_multi_link_dealloc.. which is missing
task release :-\

I think we could init the link only after we create all the uprobes,
and have single release function dealloc callback and error path in here

thanks,
jirka

> 
> > +			return err;
> > +		}
> > +	}
> > +
> > +	kvfree(ref_ctr_offsets);
> > +	return bpf_link_settle(&link_primer);
> > +
> > +error_free:
> > +	kvfree(ref_ctr_offsets);
> > +	kvfree(uprobes);
> > +	kfree(link);
> > +error_path_put:
> > +	path_put(&path);
> > +	return err;
> > +}
> > +#else /* !CONFIG_UPROBES */
> > +int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +#endif /* CONFIG_UPROBES */
> [...]