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

[Yonghong Song <yonghong.song@linux.dev>]

On 11/23/23 4:20 AM, Jiri Olsa wrote:
> 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

Double checked as well. Indeed, the path is copied to the end of buffer,
so kmalloc() should be okay. Sorry for noise.