Re: [PATCH v8 bpf-next 09/13] bpf: Add d_path helper

[Jiri Olsa <jolsa@redhat.com>]

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