On Wed, Jul 29, 2020 at 09:11:17PM +0100, Al Viro wrote:
> On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote:
>
> > +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> > +{
> > + char *p = d_path(path, buf, sz - 1);
> > + int len;
> > +
> > + if (IS_ERR(p)) {
> > + len = PTR_ERR(p);
> > + } else {
> > + len = strlen(p);
> > + if (len && p != buf)
> > + memmove(buf, p, len);
>
> *blink*
> What the hell do you need that strlen() for? d_path() copies into
> the end of buffer (well, starts there and prepends to it); all you
> really need is memmove(buf, p, buf + sz - p)
I used the code from some of the other users like
backing_dev_show
fsg_show_file
nice, looks like we could omit strlen call in perf mmap event call as well
>
>
> > + buf[len] = 0;
>
> Wait a minute... Why are you NUL-terminating it separately?
> You do rely upon having NUL in the damn thing (and d_path() does
> guarantee it there). Without that strlen() would've gone into
> the nasal demon country; you can't call it on non-NUL-terminated
> array. So you are guaranteed that p[len] will be '\0'; why bother
> copying the first len bytes and then separately deal with that
> NUL? Just memmove() the fucker and be done with that...
>
> If you are worried about stray NUL in the middle of the returned
> data... can't happen. Note the rename_lock use in fs/d_path.c;
> the names of everything involved are guaranteed to have been
> stable throughout the copying them into the buffer - if anything
> were to be renamed while we are doing that, we'd repeat the whole
> thing (with rename_lock taken exclusive the second time around).
>
> So make it simply
> if (IS_ERR(p))
> return PTR_ERR(p);
> len = buf + sz - p;
> memmove(buf, p, len);
> return len;
ok, will use this
> and be done with that. BTW, the odds of p == buf are pretty much
> nil - it would happen only if sz - 1 happened to be the exact length
> of pathname.
>
ok, great
thanks,
jirka
