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@xxxxxxxxx> 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@xxxxxxxxxx> 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
