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

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Nov 22, 2023 at 10:50:06PM +0100, Jiri Olsa wrote:
> 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

hum, actually.. after checking d_path IIUC it copies into the end of buffer,
so I can't see this code copying more data to user buffer

jirka