On Thu, Oct 26, 2023 at 10:55:35AM -0700, Song Liu wrote:
> On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> [...]
> > __u64 missed;
> > } kprobe_multi;
> > + struct {
> > + __aligned_u64 path;
> > + __aligned_u64 offsets;
> > + __aligned_u64 ref_ctr_offsets;
> > + __aligned_u64 cookies;
> > + __u32 path_max; /* in/out: uprobe_multi path size */
>
> I don't think we use path_max for output. Did I miss something?
it's used by user to specify the size of the buffer
for storing the path, so you're right just 'in'
>
> > + __u32 count; /* in/out: uprobe_multi offsets/ref_ctr_offsets/cookies count */
> > + __u32 flags;
> > + __u32 pid;
> > + } uprobe_multi;
> > struct {
> > __u32 type; /* enum bpf_perf_event_type */
> > __u32 :32;
>
> [...]
>
> > +
> > + 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(umulti_link->task) : (u32) -1;
>
> I think we can just use 0 here (instead of (u32)-1)?
ok
>
> > +
> > + if (upath) {
>
> nit: we are only using buf and p in this {}. It is cleaner to define them here.
ok
>
> > + if (upath_max > PATH_MAX)
> > + return -E2BIG;
>
> I don't think we need to fail here. How about we simply do
>
> upath_max = min_ut(u32, upath_max, PATH_MAX);
ok
>
> > + buf = kmalloc(upath_max, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > + p = d_path(&umulti_link->path, buf, upath_max);
> > + if (IS_ERR(p)) {
> > + kfree(buf);
> > + return -ENOSPC;
> > + }
> > + left = copy_to_user(upath, p, buf + upath_max - p);
> > + kfree(buf);
> > + if (left)
> > + return -EFAULT;
> > + }
> > +
> > + if (!uoffsets)
> > + return 0;
> > +
> > + if (ucount < umulti_link->cnt)
> > + err = -ENOSPC;
> > + else
> > + ucount = umulti_link->cnt;
> > +
> > + for (i = 0; i < ucount; i++) {
> > + if (put_user(umulti_link->uprobes[i].offset, uoffsets + i))
> > + return -EFAULT;
> > + if (uref_ctr_offsets &&
> > + put_user(umulti_link->uprobes[i].ref_ctr_offset, uref_ctr_offsets + i))
> > + return -EFAULT;
> > + if (ucookies &&
> > + put_user(umulti_link->uprobes[i].cookie, ucookies + i))
> > + return -EFAULT;
>
> It feels expensive to put_user() 3x in a loop. Maybe we need a new struct
> with offset, ref_ctr_offset, and cookie?
good idea, I think we could store offsets/uref_ctr_offsets/cookies
together both in kernel and user sapce and use just single put_user
call... will check
thanks,
jirka
>
> Thanks,
> Song
>
> > + }
> > +
> > + return err;
> > +}
> > +
