Re: [PATCHv3 bpf-next 3/6] bpf: Add link_info support for uprobe multi link

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Nov 20, 2023 at 10:04:16AM -0800, Yonghong Song wrote:

SNIP

> > +static int bpf_uprobe_multi_link_fill_link_info(const struct bpf_link *link,
> > +						struct bpf_link_info *info)
> > +{
> > +	u64 __user *uref_ctr_offsets = u64_to_user_ptr(info->uprobe_multi.ref_ctr_offsets);
> > +	u64 __user *ucookies = u64_to_user_ptr(info->uprobe_multi.cookies);
> > +	u64 __user *uoffsets = u64_to_user_ptr(info->uprobe_multi.offsets);
> > +	u64 __user *upath = u64_to_user_ptr(info->uprobe_multi.path);
> > +	u32 upath_size = info->uprobe_multi.path_size;
> > +	struct bpf_uprobe_multi_link *umulti_link;
> > +	u32 ucount = info->uprobe_multi.count;
> > +	int err = 0, i;
> > +	long left;
> > +
> > +	if (!upath ^ !upath_size)
> > +		return -EINVAL;
> > +
> > +	if ((uoffsets || uref_ctr_offsets || ucookies) && !ucount)
> > +		return -EINVAL;
> > +
> > +	umulti_link = container_of(link, struct bpf_uprobe_multi_link, link);
> > +	info->uprobe_multi.count = umulti_link->cnt;
> > +	info->uprobe_multi.flags = umulti_link->flags;
> > +	info->uprobe_multi.pid = umulti_link->task ?
> > +				 task_pid_nr_ns(umulti_link->task, task_active_pid_ns(current)) : 0;
> > +
> > +	if (upath) {
> > +		char *p, *buf;
> > +
> > +		upath_size = min_t(u32, upath_size, PATH_MAX);
> > +
> > +		buf = kmalloc(upath_size, GFP_KERNEL);
> > +		if (!buf)
> > +			return -ENOMEM;
> > +		p = d_path(&umulti_link->path, buf, upath_size);
> > +		if (IS_ERR(p)) {
> > +			kfree(buf);
> > +			return -ENOSPC;
> 
> Should we just return PTR_ERR(p)? In d_path, it is possible that
> -ENAMETOOLONG is returned. But path->dentry->d_op->d_dname() might
> return a different error reason than  -ENAMETOOLONG or -ENOSPC?

true, will change

> 
> > +		}
> > +		upath_size = buf + upath_size - p;
> > +		left = copy_to_user(upath, p, upath_size);
> 
> Here, the data copied to user may contain more than
> actual path itself. I am okay with this since this
> is not in critical path. But early buf allocation is using
> kmalloc whose content could be arbitrary. Should we
> use kzalloc for the above 'buf' allocation?

good catch, will use kzalloc

thanks,
jirka