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

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Nov 01, 2023 at 03:21:42PM -0700, Andrii Nakryiko wrote:
> On Fri, Oct 27, 2023 at 7:30 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > 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@kernel.org> wrote:
> > > [...]
> 
> [...]
> 
> > >
> > > > +               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
> 
> +1, was going to say the same
> 
> >
> > >
> > > > +               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
> >
> 
> hm... only offset is mandatory, and then we can have either cookie or
> ref_ctr_offset or both, so co-locating them in the same struct seems
> inconvenient and unnecessary

yes, using struct seems too complicated because of this

but during attach we could store offsets/ref_ctr_offsets/cookies
in separated arrays (instead of in bpf_uprobe) and just use single
copy_to_user call for each array in here

jirka