Re: [PATCH 2/3] bpf: Add d_path helper

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Apr 3, 2020 at 2:03 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Apr 02, 2020 at 04:02:55PM +0200, Florent Revest wrote:
> > On Wed, 2020-04-01 at 13:09 +0200, Jiri Olsa wrote:
> > > + * int bpf_d_path(struct path *path, char *buf, u32 sz)
> > > + * Description
> > > + *         Return full path for given 'struct path' object, which
> > > + *         needs to be the kernel BTF 'path' object. The path is
> > > + *         returned in buffer provided 'buf' of size 'sz'.
> > > + *
> > > + * Return
> > > + *         length of returned string on success, or a negative
> > > + *         error in case of failure
> > > + *
> >
> > You might want to add that d_path is ambiguous since it can add
> > " (deleted)" at the end of your path and you don't know whether this is
> > actually part of the file path or not. :)
>
> right
>
> >
> > > +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> > > +{
> > > +   char *p = d_path(path, buf, sz - 1);
> >
> > I am curious why you'd use sz - 1 here? In my experience, d_path's
> > output is 0 limited so you shouldn't need to keep an extra byte for
> > that (if that was the intention here).
> >
> > > +   int len;
> > > +
> > > +   if (IS_ERR(p)) {
> > > +           len = PTR_ERR(p);
> > > +   } else {
> > > +           len = strlen(p);
> > > +           if (len && p != buf) {
> > > +                   memmove(buf, p, len);
> >
> > Have you considered returning the offset within buf instead and let the
> > BPF program do pointer arithmetics to find the beginning of the string?
>
> we could do that.. I was following some other user of d_path,
> which I can't find at the moment ;-) I'll check

This would make it hard to support variable-length data encoding and
sending it over perf_buffer, because it would prevent back-to-back
"stitching" of multiple strings compactly in output buffer. So I think
current approach is preferable.

>
> >
> > > +                   buf[len] = 0;
> >
> > If my previous comment about sz - 1 is true, then this wouldn't be
> > necessary, you could just use memmove with len + 1.
>
> hum, you might be right, I'll check on this
>
> thanks,
> jirka
>
> >
> > > +           }
> > > +   }
> > > +
> > > +   return len;
> > > +}
> >
>